-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Conversation
This code has moved to joblib itself This commits removes all relevant code and tests, and raises an informative error message instead.
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. |
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. |
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. |
Thanks! |
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. |
Probably safe to remove the last vestiges now. ( #3040 ) |
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