Skip to content

Remove Joblib Dask Backend from codebase#2298

Merged
TomAugspurger merged 3 commits intodask:masterfrom
mrocklin:remove-joblib
Oct 8, 2018
Merged

Remove Joblib Dask Backend from codebase#2298
TomAugspurger merged 3 commits intodask:masterfrom
mrocklin:remove-joblib

Conversation

@mrocklin
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Thanks!

@mrocklin mrocklin deleted the remove-joblib branch October 8, 2018 14:37
@jakirkham
Copy link
Copy Markdown
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
Copy Markdown
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