Add Modin backends to dev requirements #366
Conversation
zmbc
requested review from
albrja,
hussain-jafari,
mattkappel,
ramittal,
rmudambi and
stevebachmeier
as code owners
setup.py
Outdated
| @@ -104,7 +104,8 @@ | |||
| "dev": doc_requirements | |||
| + test_requirements | |||
| + interactive_requirements | |||
| + modin_requirements, | |||
| + modin_requirements | |||
| + ["ray", "dask"], | |||
There was a problem hiding this comment.
Shouldn't these be added to the modin_requirements list?
There was a problem hiding this comment.
Modin has multiple backends (more even than these two) and they are all optional, since you can install any one of them and it will work. We don't want to force our users to install any particular backend, but for development I've been using these two (and I think we will test on these two, once we have automated tests for Modin).
There was a problem hiding this comment.
We shouldn't have naked requirements declared here. Sounds like we need a separate variable to contain these requirements like modin_test_requirements. If we have automated tests for these, they should at least be within the "test" extra otherwise tests will fail if you only install .[test].
There was a problem hiding this comment.
@rmudambi I've updated this; let me know if it is better now. We don't actually have the automated tests yet, but we will want them before we merge the Modin epic.
| test_requirements = [ | ||
| "pytest", | ||
| "pytest-mock", | ||
| ] | ||
| ] + modin_test_engines_requirements |
There was a problem hiding this comment.
I'm a bit confused. Is there a situation in which we would want to be able to install our test requirements but not the modin test requirements? If so, we aren't able to as you have it, but if we wouldn't, why not just include ray and dask in the test-requirements directly?
There was a problem hiding this comment.
I could include ray and dask in the test_requirements directly; the idea here was just to make it easier to see the different types of requirements. But if you think it adds confusion, I'll change it.
There was a problem hiding this comment.
I think it should be bundled unless we want to enable a person to install only test without dask/ray and be able to run the tests that don't use them. That seems like an unlikely use case?
Add Modin backends to dev requirements
Description
For development, we'll want to have some actual backends for Modin installed.
Testing
None