This pull request introduces 2 alerts and fixes 2 when merging 7093fc1 into a2b5645 - view on LGTM.com

new alerts:

• 2 for Unused import

fixed alerts:

• 2 for Unused import

Comment posted by LGTM.com

This pull request introduces 2 alerts and fixes 2 when merging 314daa1 into a2b5645 - view on LGTM.com

new alerts:

• 2 for Unused import

fixed alerts:

• 2 for Unused import

Comment posted by LGTM.com

This pull request fixes 2 alerts when merging e567b8f into a2b5645 - view on LGTM.com

fixed alerts:

• 2 for Unused import

Comment posted by LGTM.com

This is looking green. Ready for review.

This pull request fixes 2 alerts when merging 7785ab6 into a2b5645 - view on LGTM.com

fixed alerts:

• 2 for Unused import

Comment posted by LGTM.com

+1 as well, it sounds like a safe way to handle the switch.

Should we add in the travis matrix a job that tests with SKLEARN_SITE_JOBLIB=1 (this should probably be done in a separate PR).

It's already tested in the cron job that checks the master branch of everything.

We could also update one on the existing entries to use the site joblib. No need to extend the size of the build matrix for this.

LGTM

it works.

I will update the PR.

I added another test and found a bug: cab9e0a

Tests now pass both with and without SKLEARN_SITE_JOBLIB on my laptop. Let's wait for travis. I plan to merge without waiting for appveyor.

A few things we may want to think about:

• is sklearn.utils the right place for joblib stuff?
• we are making only a few joblib things available in sklearn.utils (Memory, Parallel, delayed, paralell_backend, cpu_count). Does that cover most of what people do with sklearn.externals.joblib?

we are making only a few joblib things available in sklearn.utils (Memory, Parallel, delayed, paralell_backend, cpu_count). Does that cover most of what people do with sklearn.externals.joblib?

Indeed. If we accept one more level of nesting we could have:

sklearn/utils/joblib.py where we have:

# An environment variable to use the site joblib
if _os.environ.get('SKLEARN_SITE_JOBLIB', False):
    from joblib import __all__
    from joblib import *
    from joblib import __version__
    from joblib import logger
else:
    from ..externals.joblib import __all__
    from ..externals.joblib import *
    from ..externals.joblib import __version__
    from ..externals.joblib import logger

to expose the full joblib namespace.

+1 for having this in sklearn.utils.joblib

@jnothman my last commit put the "public" joblib symbols under sklearn.utils.joblib instead of flattening them into the sklearn.utils level.

My reasoning for using sklearn.utils rather than sklearn.utils.joblib went something like:

• In practice we only use a few things from joblib and only need to expose those to sklearn and users' libraries.
• Indeed, site and vendored joblib might expose different names. We need to make sure we're importing get_cpu_count, parallel_backend, etc for scikit-learn to work. And we need to make sure that developers never import sklearn.utils.joblib.newfangled_thing
• Importing from sklearn.utils.joblib looks a lot like importing from sklearn.externals.joblib which may confuse users, and may be easily missed by reviewers. from sklearn.utils import Memory gives a clean break in the paradigm of joblib usage.

from sklearn.utils import Memory gives a clean break in the paradigm of joblib usage.

I understand that we want to fix the parent issue but I was under impression that in the long run, we also wanted people to stop importing Parallel etc from from scikit-learn altogether and make them use joblib directly if they do require parallel functionality for their code? Otherwise issues get reported to the wrong repo etc.

sklearn.utils is semi-public, so does it mean that technically we have to write an API changes entry in scikit-learn if something changes in joblib? Why not use e.g.sklearn.utils._joblib internally and tell users to use joblib for user applications? Or am I missing something?

I agree with you @jnothman. However, I also have the feeling that if in the future joblib introduces a new set of toplevel symbols that we want to use in scikit-learn, we will start to crowd the sklearn.utils namespace quite heavily with possible (however unlikely) name conflicts.

Also I am wondering if it might not be useful to do:

>>> from sklearn.utils import joblib

to get a natural hold on the full joblib namespace as sklearn sees it in test scripts / interactive sessions.

This will still be possible with the later patterns though:

>>> from sklearn.utils import _joblib as joblib

so maybe this later point is not a big problem in the end. I am still worried about the first point though.

I understand that we want to fix the parent issue but I was under impression that in the long run, we also wanted people to stop importing Parallel etc from from scikit-learn altogether and make them use joblib directly if they do require parallel functionality for their code? Otherwise issues get reported to the wrong repo etc.

Parallel is different to Memory here. We ask users to sometimes pass a Memory object as a parameter. Should they have to separately install joblib to do so?

Also, does with parallel_backend... work if parallel_backend is site joblib.parallel_backend, but Parallel is the vendored sklearn.externals.joblib.parallel_backend? Surely the user code needs to be using the same joblib instance as we are internally.

we will start to crowd the sklearn.utils namespace quite heavily with possible (however unlikely) name conflicts.

The namespace is crowded already... I am not persuaded by this as a sufficient counter-argument. But shrug. It's not that important.

Also, does with parallel_backend... work if parallel_backend is site joblib.parallel_backend, but Parallel is the vendored sklearn.externals.joblib.parallel_backend? Surely the user code needs to be using the same joblib instance as we are internally.

Indeed, the user needs to use the parallel_backend that matches the active implementation of joblib.

Alright, I'll revert my last commit.

I plan to merge this without waiting for appveyor if travis is green.

PEP8 failure :(. You'll need "# noqa" on some lines.

We don't care no? Let me wait for the real tests to pass.

OK. I tend not to like pyflakes warnings.

Alright pushed.

Merged.