MAINT: add distributed as a dependency

[stsievert]

Fixes #464

[stsievert]

This is a work in progress while I figure out the testing.

[TomAugspurger]

These are generally hard to test in CI, due to module caching.

I'd recommend a simple change at the top of the file with a

try:
    import distributed
except ImportError:
    raise ImportError(nice message here)

and just a manual test locally in an environment without distributed.

[jrbourbeau]

Do we want to add distributed as a dependency? Ref discussion going on in #463

[TomAugspurger]

I have a slight preference for keeping this an optional dependency for now.

…

On Tue, Feb 19, 2019 at 3:12 PM James Bourbeau ***@***.***> wrote: Do we want to add distributed as a dependency? Ref discussion going on in #463 <#463> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#466 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHInlfLpCChyfvhEWKUkoIlfNq9n1_ks5vPGirgaJpZM4bDyhe> .

[jrbourbeau]

Sounds good

[TomAugspurger]

Ahh sorry @stsievert I may have led you astray... I see now why you started with your dummy class, since we need to provide something to model_selection/__init__.py to import, so that it would raise when the class is instantiated.

So, given the headaches of working around that, lets just add a dependency on distributed. Did you want to work on that change here?

[stsievert]

So, given the headaches of working around that, lets just add a dependency on distributed. Did you want to work on that change here?

This solution is a lot easier.

I made that change here, which is a one-line change to setup.py. Is that all that's required?

Ahh sorry @stsievert I may have led you astray... I see now why you started with your dummy class

No worries – I didn't waste too much time on this. This solution is a lot cleaner, and resolves the conflict between requiring an optional dependency.

[TomAugspurger]

Yeah, the change to setup.py should be all we need now.

I'll take a look at the failing CI in a bit.

[stsievert]

Should a minimum version of distributed be required?

[TomAugspurger]

I was just looking to see what a good version would be. It seems like distributed 1.25.0 coincides with Dask 1.0, so let's go with that.

[stsievert]

Sounds good. I've added distributed>=1.25.0. I'll leave leave #461 to make Dask 1.0 required

[TomAugspurger]

CI should be fixed now, if you could merge master and push.

[stsievert]

Master merged and CI passing.

[TomAugspurger]

Thanks.