Contributing: distributed not installed with [dev] extras_require

[n8henrie]

Hello,

I just followed the contributing guide, which recommends a sensible python -m pip install -e ".[dev]", but unfortunately my first step was to ensure that existing tests pass before making any changes, and pytest tests/ raised an ImportError that I assume is related to dask.distributed.

ImportError while loading conftest '/home/n8henrie/git/dask-ml/tests/conftest.py'.
tests/conftest.py:7: in <module>
    from distributed.utils_test import cluster
E   ModuleNotFoundError: No module named 'distributed'

I was able to work around by also installing .[complete] (though in retrospect it looks like I could have just installed distributed alone, I thought there could be a handful of missing dependencies that I'd have to discover one by one, so I went with complete to make sure it was taken care of in a single install).

It seems like the recommended setup in the contributing guide should lead to a point where one can at least run the tests -- I recommend either a change to the contributing guide to additionally recommend a manual installation of this dependency (in a quick glance I didn't see how to add a PR to the page), or possibly by adding distributed to the dev dependencies, or by including the complete dependencies to the dev dependencies.

Happy to make this (super minor) change into a PR if one of these is thought to be a best course of action.

[TomAugspurger]

Adding distributed to the dev dependencies sounds best.

…

On Mon, Feb 18, 2019 at 7:10 PM Nathan Henrie ***@***.***> wrote: Hello, I just followed the contributing guide <https://ml.dask.org/contributing.html#building-dask-ml>, which recommends a sensible python -m pip install -e ".[dev]", but unfortunately my first step was to ensure that existing tests pass before making any changes, and pytest tests/ raised an ImportError that I assume is related to dask.distributed. ImportError while loading conftest '/home/n8henrie/git/dask-ml/tests/conftest.py'.tests/conftest.py:7: in <module> from distributed.utils_test import cluster E ModuleNotFoundError: No module named 'distributed' I was able to work around by also installing .[complete] (though in retrospect it looks like I could have just installed distributed alone, I thought there could be a handful of missing dependencies that I'd have to discover one by one, so I went with complete to make sure it was taken care of in a single install). It seems like the recommended setup in the contributing guide should lead to a point where one can at least run the tests -- I recommend either a change to the contributing guide to additionally recommend a manual installation of this dependency (in a quick glance I didn't see how to add a PR to the page), or possibly by adding distributed to the dev dependencies, or by including the complete dependencies to the dev dependencies. Happy to make this (super minor) change into a PR if one of these is thought to be a best course of action. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#463>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIqo3_psSezKO2--zaDqFWwy4AMErks5vO07_gaJpZM4bB9ji> .

[jrbourbeau]

Perhaps we could update dask[array]>=0.18.2 in the setup.py install_requires to dask[complete]>=0.18.2? It looks like we make use of distributed in dask_ml/model_selection/_incremental.py

[n8henrie]

I think either sounds reasonable. For reference: the current dask setup.py.

[jrbourbeau]

Based on #466 (comment), we're going to keep distributed an optional dependency for now. So adding it to the dev dependencies should work here!

[TomAugspurger]

Sorry for they run-around here, but I think that
#46 is adding distributed as a required dependency, so we shouldn't need to add it to the optional dev dependencies any more.

[n8henrie]

@TomAugspurger Are you sure that's the right PR? I don't see any changes to the setup.py in the diff at https://github.com/dask/dask-ml/pull/46/files -- only changes are to CI files.

[stsievert]

I think @TomAugspurger meant #466.

[n8henrie]

Gotcha, thanks!