Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Joblib Dask Backend from codebase #2298

Merged
merged 3 commits into from
Oct 8, 2018

Conversation

mrocklin
Copy link
Member

@mrocklin mrocklin commented Oct 7, 2018

This code has moved to joblib itself

This commits removes all relevant code and tests, and raises an
informative error message instead.

Recent changes in the joblib version of this code was causing our tests to fail. I figure we should probably stop including a copy here. I'm fine with requiring users who want to use this feature to depend on the latest release. cc @TomAugspurger and @ogrisel

This code has moved to joblib itself

This commits removes all relevant code and tests, and raises an
informative error message instead.
@TomAugspurger
Copy link
Member

TomAugspurger commented Oct 8, 2018

Did you consider just printing out a warning instead of raising an error? I was going to ask for that, but upon further consideration the only adjustment people need to make is to delete an import.

If you've thought this through already, then +1 to just removing.

@mrocklin mrocklin mentioned this pull request Oct 8, 2018
@mrocklin
Copy link
Member Author

mrocklin commented Oct 8, 2018

It's a newish feature without much adoption. I'm inclined to be more strict. Happy either way though. Merging shortly if there is no response.

@dhirschfeld
Copy link
Contributor

dhirschfeld commented Oct 8, 2018

As a user of this feature I say rip it out!

The longer a bad/incorrect/broken feature remains in the harder it is to extricate IMHE.

Early adopters are likely sophisticated enough to not be fazed at all given the clear and explicit error message.

@TomAugspurger TomAugspurger merged commit a2d3fff into dask:master Oct 8, 2018
@TomAugspurger
Copy link
Member

Thanks!

@mrocklin mrocklin deleted the remove-joblib branch October 8, 2018 14:37
@jakirkham
Copy link
Member

For context/posterity PR ( joblib/joblib#722 ) added the Distributed backend to Joblib and shows up in 0.12.2. This version of Joblib was also vendored in scikit-learn with PR ( scikit-learn/scikit-learn#11741 ), which is included in scikit-learn 0.20.0.

@jakirkham
Copy link
Member

Probably safe to remove the last vestiges now. ( #3040 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants