Should this be backported to 0.20.1 ?

+1

So my comments above are not exactly on the right lines :) but the general point is that I think we should not expose public attributes sklearn.utils.{joblib,joblib_version}. What we do in sklearn.utils._joblib and import it from there directly internally is fine..

Ok I tried to remove all direct import of joblib, which is now only available in _joblib.

Note that I also exposed dump and load in sklearn.utils to avoid putting private API import in the doc. Is that okay or should they be renamed?

Hmm, right joblib.{dump, load} is also used in examples in which case joblib.dump is still better than dump.

I don't know anymore, other opinions on this would be welcome @jnothman @ogrisel @amueller

It depends on how you think the interaction between scikit-learn and joblib will evolve in the future.

In 0.20.0 we have exposed sklearn.utils.{cpu_count, Parallel, Memory, delayed, hash, parallel_backend, register_parallel_backend}. Now it turns out that this is not enough and some at least dump/load are necessary. The sklearn.utils is namespace is progressively getting invaded by joblib and I'm not convinced it's a good thing. For internal code, I guess importing from sklearn.utils import _joblib could have been enough, but then the question is what to do for examples. One thing I find important is that it should be clear for users that the parallel capability is coming from a separate project called joblib, and in that sense in examples,

from sklearn.externals.joblib import Parallel
# or
from sklearn.utils._joblib import Parallel

is better IMO than

from sklearn.utils import Parallel

so that bugs are reported to the right issue tracker and joblib gets the recognition it deserves.

Maybe we could have exposed just sklearn.utils.joblib instead of the half dozen of current methods, but I guess we can't do much now that 0.20.0 is out.

What happens if say in v0.15 joblib deprecates or renames joblib.hash (or some other method). Will we have to do a deprecation cycle on sklearn.utils.hash, or do we consider those not part of the public API (in which case we might as well do something about it now)?

the public/private state of utils is still a bit unclear.
I didn't actually realize we exposed sklearn.utils.Parallel otherwise I would have vetoed that :(
I think we should unvendor as soon as possible, and having these names defined is very weird in that case. If we deprecated sklearn.externals.joblib and told people to use sklearn.utils.X then we shouldn't be removing those. But we can backpaddle and deprecate them in 0.20.1

Either way I feel like we need to resolve this for 0.20.1.
Can someone point me to the original discussion for this? What was the reason for moving to utils? (I know the reason for defining _joblib was to optionally unvendor).

(FYI I also had vetoed vendoring loky inside vendoring joblib very explicitly and that was ignored...)

For reference:

• [MRG] MAINT: option to unvendor joblib #11166 added the optional unvendoring of joblib
• [MRG+1] Restructure access to vendored/site Joblib #11471 restructured the access to joblib in sklearn.utils.

hm... I'd still vote deprecate everything and unvendor. That will probably break all pickles in two versions but will remove some major headaches going forward.
It "only" raises the questions which versions of joblib each sklearn should support.

The advantage of vendoring (or pinning) is that, unless the user does something very explicit, we know which version of joblib they're using when handling issues around a particular version. I assume @GaelVaroquaux would have an opinion here.

I don't think we need to provide load and dump in utils. If we do, we can prefix them by joblib_. Yes, I agree there are open questions about API stability for public vendored components... but we had already been expecting users to write code that depended on this vendoring of joblib, really.

Whether we vendor joblib, and how much of its API we are exposing as a public API of scikit-learn are two somewhat different/unrelated concerns. Let's move this discussion to a separate issue so it doesn't get lost in this PR, I'll try to open one later today with a summary of different points of view.

@rth I don't think they are entirely separate, unless you would consider providing an interface without vendoring. Which we could do - but it would seem pretty strange to me.
And we as long as we vendor, we need to provide a public API, right?

I don't think we need to provide load and dump in utils.

Can you elaborate? How else would you fix the problem this PR addresses?

Well we never use sklearn.externals.joblib.load in our codebase... so we don't strictly need to support it. But we can.

It is used in the datasets. I added sklearn.utils.dump and sklearn.utils.load because they are presented in the datasets and imported for instance here.

Also, it is mentioned in the documentation for instance here

Tried to write some summary on the joblib integration in #12447

I guess in the interest of getting 0.20.1 out we should probably table the larger discussion and finish this up in some way for now?

I am not sure I got the general concensus here.

I can rename dump and load to joblib_* and finish this PR like that, with joblib objects being available in sklearn.utils.

Or I add an extra layer for all joblib-related import such that everything is imported under sklearn.utils.joblib. I think this solution is better as it makes it clear that the tools used are defined in the joblib library and should not be considered as part of the sklearn API.

I can wrap this up tomorrow once given the green-light on the chosen solution.

I don't have a strong preference between the two. Having a more clear namespace for joblib seems desirable, but that would also require moving all the other functions and objects again, right?

I will move all the exposed functions in sklearn.utils.joblib if nobody has a strong opposition against it.

@ogrisel?

Done.

@jnothman I updated module/classes.rst, let me know if this is the right way to do it.
I also added a more detailed entry in what's new.

The changes to classes.rst look fine

@jnothman are you ok with doing it this way? You seemed a bit concerned about the change earlier. It seems the easiest way forward to me.

Thanks @tomMoral

yay!

Recently I changed all my code to use sklearn.externals.joblib instead of joblib alone. What should I use that will be compatible in the future? Would I be able still use the joblib from sklearn or will this be deprecated in future versions? Thanks.