v0.10.1 & turn into multi-output recipe to account for optional installs #14

h-vetinari · 2021-03-05T10:02:03Z

While updating the dependencies for #13, I noticed that there are new optional installs (and that this recipe hasn't done a good job in keeping in sync with the dependency changes...)

In order to make the core install less heavy, I propose the following mapping of installs

`pip install`	`conda install`	Comment
`modin[all]`	`modin-all`
`modin[dask]`	`modin-dask`
`modin[ray]`	`modin-ray`
`modin[remote]`	`modin-remote`	Currently disabled, `rpyc` not in conda-forge yet
`modin[spreadsheet]`	`modin-spreadsheet`	Currently disabled, `modin-spreadsheet` not in conda-forge yet; Note: this name would currently conflict between the modin-optional and the separate package!
`modin`	`modin`	Needed for compatibility with previous installs. Should probably replace what I've currently named `modin-core`, but could also point to `modin-all`

Finally, there's a problem with the re-rendering, that conda-smithy reintroduces the 3.6 job, but that's unrelated to this discussion.

~~Builds on #13 (which should be merged first) and incorporates those commits~~

Closes #17
Closes #18

CC @devin-petersohn

conda-forge-linter · 2021-03-05T10:02:09Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

devin-petersohn · 2021-03-05T14:46:47Z

@h-vetinari Your plan looks great. I agree that conda install -c conda-forge modin should probably point to modin-core. Ideally conda install would only add the minimum requirements.

We can skip the spreadsheet and remote as those are very early releases and probably not ready for mass adoption.

h-vetinari · 2021-03-05T14:54:38Z

Your plan looks great.

Happy to hear it! :)
Over several packages with many optionals, I've found this to be an approach that works well. I'd need your help a bit how to write meaningful tests for the various subpackages; feel free to commit in this PR if you want.

I agree that conda install -c conda-forge modin should probably point to modin-core. Ideally conda install would only add the minimum requirements.

I've changed my mind on this slightly, because conda install modin and then python -c "import modin" fails, since there's no engine. In the meantime, I'm pointing modin to require modin-dask (since ray is a more heavy-weight dependency and not available on osx yet).

We can skip the spreadsheet and remote as those are very early releases and probably not ready for mass adoption.

That's fine, they're currently skipped in the recipe. The only thing I'd like to catch early is if we can avoid the naming clash between modin-spreadsheet (the PyPI package) and modin-spreadsheet==modin[spreadsheet] (the modin-optional). One solution would be to not package the PyPI package for conda-forge independently, but only package it here in this feedstock for the output modin-spreadsheet (which then still is the functional equivalent of modin[spreadsheet]). WDYT?

devin-petersohn · 2021-03-05T15:08:41Z

One solution would be to not package the PyPI package for conda-forge independently, but only package it here in this feedstock for the output modin-spreadsheet (which then still is the functional equivalent of modin[spreadsheet]). WDYT?

I think that makes sense, we could also have modin-spreadsheet under a different name (modin-spreadsheet-core) in conda forge. We aren't anticipating anyone directly installing it without Modin (it wont work without Modin anyway), but want it to make sure that it can evolve quickly and independently since it's quite early.

devin-petersohn · 2021-03-16T13:32:17Z

@h-vetinari What do you need from me on this?

h-vetinari

@devin-petersohn: What do you need from me on this?

I left a couple of comments where I could use some inputs. :)

h-vetinari · 2021-03-16T20:53:56Z

recipe/meta.yaml

+    test:
+      commands:
+        # cannot import modin because it needs an engine (dask or ray)
+        # TODO: add core-specific tests!
+        - exit 0


Could use some advice for core-specific tests here

For core specific, we could just import modin. We could also add a pytest test file with a with pytest.raises block in it to ensure it does raise an error.

Related question: do we need modin-core on conda forge? We have it in pypi to be as lightweight as possible because often a user will have one of ray/dask installed already. Do you think it will help to have it here too (even just to mirror pypi)?

The thing is, import modin doesn't work for core, because there's no engine, and the import fails on that.

modin-core makes a lot of sense IMO, even if it's not intended for general usage, because it allows us to have a common base for the people to either choose dask and/or ray as an engine.

import modin should work on the most recent releases.

import modin # doesn't touch engine code import modin.pandas

This will fail:

import modin from modin.config import Engine Engine.get() # triggers check for engine

h-vetinari · 2021-03-16T20:54:27Z

recipe/meta.yaml

+    test:
+      imports:
+        # are there more specific imports?!
+        - modin


Are there modin[dask]-specific things to test?

Testing the imports should be sufficient.

h-vetinari · 2021-03-16T20:54:46Z

recipe/meta.yaml

+    test:
+      imports:
+        # are there more specific imports?!
+        - modin


Are there modin[ray]-specific things to test?

Same here, testing import should be sufficient

h-vetinari · 2021-03-16T20:56:52Z

recipe/meta.yaml

+  # TODO: resolve conflict modin[spreadsheet] vs. modin-spreadsheet
+  - name: modin-spreadsheet


Based on your comment, my understanding now would be:

leave this as modin-spreadsheet in this package

when bringing in the modin-spreadsheet from PyPI to conda-forge, name it something else, like modin-spreadsheet-core

Does this match your view?

Yes, this matches my view

Alright, let's remember that whenever we create that staged-recipes PR.

I'm a little late to the party, but I don't think a separate package is needed - all modin-spreadsheet would do would be add a dependency to modin-spreadsheet-core, what's the point of having two separate packages? I think the other solution (make modin-spreadsheet subpackage include modin-spreadsheet from PyPI) makes more sense to me.

Come to think of it more, I think we definitely need a separate feedstock, because modin-spreadsheet (separate package from PyPI) has its own separate versioning.

Yeah, we just need to make sure that the separate feedstock is named modin-spreadsheet-core...

devin-petersohn · 2021-03-19T20:05:10Z

@h-vetinari Sorry it's been a hectic couple of weeks

I left a couple of comments where I could use some inputs. :)

I missed them before :/. I will respond

h-vetinari

Sorry it's been a hectic couple of weeks

No need to apologise. Here are my answers.

h-vetinari · 2021-03-20T20:50:55Z

recipe/meta.yaml

+    test:
+      commands:
+        # cannot import modin because it needs an engine (dask or ray)
+        # TODO: add core-specific tests!
+        - exit 0


The thing is, import modin doesn't work for core, because there's no engine, and the import fails on that.

modin-core makes a lot of sense IMO, even if it's not intended for general usage, because it allows us to have a common base for the people to either choose dask and/or ray as an engine.

h-vetinari · 2021-03-20T20:51:15Z

recipe/meta.yaml

+    test:
+      imports:
+        # are there more specific imports?!
+        - modin


h-vetinari · 2021-03-20T20:51:22Z

recipe/meta.yaml

+    test:
+      imports:
+        # are there more specific imports?!
+        - modin


h-vetinari · 2021-03-20T20:51:56Z

recipe/meta.yaml

+  # TODO: resolve conflict modin[spreadsheet] vs. modin-spreadsheet
+  - name: modin-spreadsheet


Alright, let's remember that whenever we create that staged-recipes PR.

vnlitvinov · 2021-07-13T08:25:32Z

recipe/meta.yaml

+
+  - name: modin-remote
+    build:
+      # dependency rpyc is not in conda-forge yet


do note that we also require ray-autoscaler for modin-remote, and autoscaler is also not on conda-forge now anymore because a lot of its dependencies are way out of date...

Could you open an issue on the ray-feedstock and maybe on the feedstocks of the missing dependencies?

…da-forge-pinning 2021.03.05.06.59.08

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

conda-forge-linter · 2021-07-13T09:23:08Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

It looks like the 'modin-omnisci' output doesn't have any tests.

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

recipe/meta.yaml

Also add placeholder for modin-sql Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

h-vetinari · 2021-07-13T13:48:29Z

Disabling python 3.9 should not be necessary. Something in the dependencies of pyomniscidbe depends on glibc 2.17, so we just need to follow suit there (potentially need to add - sysroot_linux-64 2.17 # [linux64] as a dependency)

vnlitvinov · 2021-07-13T13:50:47Z

I have only disabled modin-omnisci on Python 3.9 because there are no packages for pyomniscidbe for 3.9 🙄 refs: https://anaconda.org/conda-forge/pyomniscidbe/files and https://github.com/conda-forge/omniscidb-feedstock/blob/master/recipe/meta.yaml#L360

h-vetinari · 2021-07-13T13:53:09Z

Aha, I had looked at https://anaconda.org/conda-forge/pyomniscidb/files, and there are some py39 builds. What's the difference between having the "e" at the end or not?

h-vetinari · 2021-07-13T13:55:59Z

Nevermind, I can see there are different outputs in the recipe. In any case, that comment you linked is not current anymore, there are builds for tbb4py: https://anaconda.org/conda-forge/tbb4py/files

If you want you can force-push your changes again, but I'd prefer building pyomniscidbe for py39 before merging this.

h-vetinari · 2021-07-13T18:39:04Z

Waiting for the builds from conda-forge/omniscidb-feedstock#46 (should be ~3 more hours), thanks @vnlitvinov!

h-vetinari · 2021-07-13T19:19:16Z

PS. Sorry for messing up your commits, I had forgotten to update before I force-pushed. I managed to recover them using https://objectpartners.com/2014/02/11/recovering-a-commit-from-githubs-reflog/ 🙃

h-vetinari · 2021-07-13T23:17:31Z

@devin-petersohn
I cannot approve my own PR, but after the help hand of @vnlitvinov, I'd be happy to merge this now. Any final comments/concerns?

vnlitvinov · 2021-07-14T08:24:44Z

Aha, I had looked at https://anaconda.org/conda-forge/pyomniscidb/files, and there are some py39 builds. What's the difference between having the "e" at the end or not?

All difference in the world, or really bad naming (IMHO). The version with "e" is "Python bindings for SQL being run as library" (somewhat similar to sqlite but much more powerful), while without "e" is the Python interface to a SQL server run as a separate application. They serve different purposes.

P.S. I've written that yesterday but for some reason didn't publish, so publishing now.

vnlitvinov

LGTM (not that it counts now, but still)

vnlitvinov · 2021-07-14T08:31:56Z

recipe/meta.yaml

  version: {{ version }}

 source:
-  url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz
-  sha256: 88ce4def4a1145d31d207be55b9c3ba914e084ccfebace1d52b03507df258fd2
+  url: https://github.com/modin-project/modin/archive/refs/tags/{{ version }}.tar.gz


might be worth adding a comment explaining why we take github instead of pypi

You can if you want. It might just be this release where the sources are different - for the feedstocks I maintain, I consider PyPI & GH sources to be completely equivalent (actually with a slight preference to GH), and not really worth commenting about. But YMMV.

recipe/meta.yaml

vnlitvinov · 2021-07-14T08:36:08Z

recipe/meta.yaml

+  # TODO: resolve conflict modin[spreadsheet] vs. modin-spreadsheet
+  - name: modin-spreadsheet


Come to think of it more, I think we definitely need a separate feedstock, because modin-spreadsheet (separate package from PyPI) has its own separate versioning.

h-vetinari · 2021-07-14T10:08:51Z

recipe/meta.yaml

+      # Also requires ray-autoscaler which is now absent because
+      # its dependencies on conda-forge are too ancient.


@vnlitvinov, I don't see the dependency on ray[autoscaler] in setup.py for modin[remote]. Where does this come from? Again a conda-only dependency?

No, just knowledge of how things are implemented inside (as I was the one who implemented them 😄).
That said, ray does not have a ray[autoscale] target yet, ref: ray-project/ray#15725

vnlitvinov

LGTM

h-vetinari · 2021-07-14T19:09:40Z

@devin-petersohn
I cannot approve my own PR, but after the help hand of @vnlitvinov, I'd be happy to merge this now. Any final comments/concerns?

Last call (not that I think it's critical...) - will merge in about 24h unless there are objections.

vnlitvinov · 2021-07-14T20:32:02Z

I think Devin is out this week.

h-vetinari · 2021-07-14T20:45:58Z

I think Devin is out this week.

OK. Do you think we should wait or merge?

vnlitvinov · 2021-07-15T14:16:34Z

IIRC he should be back on Monday or Tuesday, we could wait for him, no big deal.

devin-petersohn

Thanks for letting me take a look before merging. LGTM!

h-vetinari requested a review from devin-petersohn as a code owner March 5, 2021 10:02

h-vetinari force-pushed the multi branch 4 times, most recently from 0bd97c2 to 98c11e5 Compare March 5, 2021 10:22

h-vetinari mentioned this pull request Mar 5, 2021

Skipping python 3.6 still creates CI job in multi-output recipe conda-forge/conda-smithy#1460

Open

h-vetinari force-pushed the multi branch from e60f7c3 to fdd72b3 Compare March 5, 2021 15:55

h-vetinari mentioned this pull request Mar 5, 2021

Global skip does not work in multi-output recipe conda/conda-build#4206

Open

h-vetinari force-pushed the multi branch from 6c9790d to 3a5fcd7 Compare March 6, 2021 21:36

h-vetinari force-pushed the multi branch from 3a5fcd7 to b7bd691 Compare March 16, 2021 20:53

h-vetinari commented Mar 16, 2021

View reviewed changes

h-vetinari commented Mar 20, 2021

View reviewed changes

h-vetinari mentioned this pull request Jul 13, 2021

modin v0.10.1 #18

Closed

3 tasks

vnlitvinov reviewed Jul 13, 2021

View reviewed changes

h-vetinari and others added 6 commits July 13, 2021 11:48

turn into multi-output recipe to handle upstream optional installs

8555e0d

MNT: Re-rendered with conda-build 3.21.4, conda-smithy 3.9.0, and con…

af7afbe

…da-forge-pinning 2021.03.05.06.59.08

bump build number

da7d681

make modin depend on modin-dask by default; shift tests

2cbc394

unpin pyarrow on py39

5d2ae01

Adapt the package to 0.10.1 release

08daca7

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

vnlitvinov force-pushed the multi branch from b7bd691 to 08daca7 Compare July 13, 2021 09:23

Fix tests specification for modin-omnisci

10eeab4

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

vnlitvinov force-pushed the multi branch from 57d36ff to 10eeab4 Compare July 13, 2021 09:28

take sources from github instead of PyPI

786b9c6

h-vetinari force-pushed the multi branch from 7871d28 to 786b9c6 Compare July 13, 2021 12:14

h-vetinari commented Jul 13, 2021

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

vnlitvinov added 2 commits July 13, 2021 15:38

Fix modin-omnisci test

5720257

Also add placeholder for modin-sql Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

Add comment about modin-omnisci source

e0c8c1f

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>

vnlitvinov force-pushed the multi branch from 6ecf993 to d0c67f2 Compare July 13, 2021 13:42

h-vetinari force-pushed the multi branch from d0c67f2 to 588174d Compare July 13, 2021 13:46

h-vetinari force-pushed the multi branch from 588174d to 786b9c6 Compare July 13, 2021 18:39

h-vetinari changed the title ~~RFC: turn into multi-output recipe to account for optional installs~~ v0.10.1 & turn into multi-output recipe to account for optional installs Jul 13, 2021

vnlitvinov approved these changes Jul 14, 2021

View reviewed changes

h-vetinari commented Jul 14, 2021

View reviewed changes

h-vetinari added 2 commits July 14, 2021 12:09

remove unnecessary restriction for pyarrow

3049725

reflect discussion about modin-spreadsheet subpackage vs. PyPI package

9b264ff

vnlitvinov approved these changes Jul 14, 2021

View reviewed changes

devin-petersohn approved these changes Jul 19, 2021

View reviewed changes

devin-petersohn merged commit e19affc into conda-forge:master Jul 19, 2021

h-vetinari deleted the multi branch July 20, 2021 04:37

YarShev mentioned this pull request Jul 3, 2023

add modin-spreadsheet in conda-forge modin-project/modin#6331

Closed

		# TODO: resolve conflict modin[spreadsheet] vs. modin-spreadsheet
		- name: modin-spreadsheet

		# Also requires ray-autoscaler which is now absent because
		# its dependencies on conda-forge are too ancient.

v0.10.1 & turn into multi-output recipe to account for optional installs #14

v0.10.1 & turn into multi-output recipe to account for optional installs #14

Conversation

h-vetinari commented Mar 5, 2021 • edited Loading

conda-forge-linter commented Mar 5, 2021

devin-petersohn commented Mar 5, 2021

h-vetinari commented Mar 5, 2021 • edited Loading

devin-petersohn commented Mar 5, 2021

devin-petersohn commented Mar 16, 2021

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devin-petersohn commented Mar 19, 2021

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conda-forge-linter commented Jul 13, 2021

h-vetinari commented Jul 13, 2021 • edited Loading

vnlitvinov commented Jul 13, 2021

h-vetinari commented Jul 13, 2021

h-vetinari commented Jul 13, 2021

h-vetinari commented Jul 13, 2021

h-vetinari commented Jul 13, 2021

h-vetinari commented Jul 13, 2021

vnlitvinov commented Jul 14, 2021

vnlitvinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov left a comment

Choose a reason for hiding this comment

h-vetinari commented Jul 14, 2021

vnlitvinov commented Jul 14, 2021

h-vetinari commented Jul 14, 2021

vnlitvinov commented Jul 15, 2021

devin-petersohn left a comment

Choose a reason for hiding this comment

h-vetinari commented Mar 5, 2021 •

edited

Loading

h-vetinari commented Mar 5, 2021 •

edited

Loading

h-vetinari commented Jul 13, 2021 •

edited

Loading