Test failures still

This sounds good to me. It needs mention in the dev docs, though, right?

I was 100% sure I replied to this... I think it should be somewhere with set_config which is currently in doc/modules/computational_performance.rst. Maybe we should have a section on configuration in the user guide? There's no explicit section on set_config and get_config right now :-/
I would also add it to the joblib entry in the glossary maybe?

I think we can merge 6 and 7 but I don't understand your new TOC.
Would it make sense to distinguish RAM saving strategies from speeding up strategies from parallelization strategies?

I'm not sure I understand the Model Reshaping section. Is that feature selection?

There seems to be a bunch of model specific stuff in 7. like "sparsify" which might be better housed in the linear models section maybe?

Also 6 and 7 are a bit hard to find imho :-/

Sure, go for it.

I've reorganized the docs and updated the glossary. This is ready for review and merge.

Sounds reasonable, but can also be changed (on or off) after release if we want it or don't.

OK let's do that later.

Thanks

This pull request introduces 2 alerts when merging d5de52c into f97b515 - view on LGTM.com

new alerts:

• 2 for Unused import

Comment posted by LGTM.com

This has broken CircleCI on master, which is trying, if I understand correctly, to load pickles which reference sklearn.externals.joblib.numpy_pickle, resulting in an ImportError.

Firstly, I presume to fix Circle, we just need to clear a cache somewhere (but I've not worked out where).

Secondly, are we unnecessarily breaking pickles from previous versions? Should we see if we can find a way to make this function importable when loading a joblib pickle?

Weird, here is the first CircleCI failed build in master. It only seems to be a problem in Python 2.

Firstly, I presume to fix Circle, we just need to clear a cache somewhere (but I've not worked out where).

I can probably find the way to clear the CircleCI cache let me look for this.

Secondly, are we unnecessarily breaking pickles from previous versions? Should we see if we can find a way to make this function importable when loading a joblib pickle?

Breaking pickles is not something we should do lightly. We should definitely double check that and see whether this can be fixed.

Well it's easily fixed if we move sklearn.externals._joblib back to sklearn.externals.joblib, and import internally from ._joblib or something confusing like that.

One hack is to use sys.modules['sklearn.externals.joblib'] = joblib and same for all its sub-modules, when SKLEARN_SITE_JOBLIB is set ...

A more "correct" approach might be to modify sys.path_hooks.

I can probably find the way to clear the CircleCI cache let me look for this.

So it looks like the only way to clear the cache is to change the cache key in circle/config.yml, see this.

I think clearing the cache would just hide the problem though. At the moment on master we can not import as we would do from joblib:

On master (5916d57):

import sklearn.externals.joblib.numpy_pickle

Python 3:

ModuleNotFoundError: No module named 'sklearn.externals.joblib.numpy_pickle'; 'sklearn.externals.joblib' is not a package

Python 2:

ImportError: No module named numpy_pickle

On 526aede (i.e. before this PR was merged) the same snippet runs fine.

I am attempting to fix the problem in #11403. I don't know if the approach will work, but if it does, it's a simple fix.

I am attempting to fix the problem in #11403. I don't know if the approach will work, but if it does, it's a simple fix.

Thanks for this! I can reproduce the backward compatibility problem locally (on Python 2.7) and I don't think #11403 fixes it. When loading the pickle sklearn.externals.joblib.numpy_pickle is imported and this import fails even with your fix:

Here is what the pickle contains for completeness (a joblib pickle of np.array([1., 2., 3.]) using 526aede):

In [3]: open('/tmp/test.joblib', 'rb').read()
Out[3]: '\x80\x02csklearn.externals.joblib.numpy_pickle\nNumpyArrayWrapper\nq\x00)\x81q\x01}q\x02(U\x05dtypeq\x03cnumpy\ndtype\nq\x04U\x02f8q\x05K\x00K\x01\x87q\x06Rq\x07(K\x03U\x01<q\x08NNNJ\xff\xff\xff\xffJ\xff\xff\xff\xffK\x00tq\tbU\x05shapeq\nK\x03\x85q\x0bU\x05orderq\x0cU\x01Cq\rU\x08subclassq\x0ecnumpy\nndarray\nq\x0fU\nallow_mmapq\x10\x88ub\x00\x00\x00\x00\x00\x00\xf0?\x00\x00\x00\x00\x00\x00\x00@\x00\x00\x00\x00\x00\x00\x08@.'

Maybe a bit hacky, but what about having only joblib/__init__.py that does this:

import sys
import os

if os.environ.get('SKLEARN_SITE_JOBLIB', False):
    import joblib
    module = sys.modules['joblib']
else:
    import sklearn.externals._joblib
    module = sys.modules['sklearn.externals._joblib']

sys.modules[__name__] = module

I did not test this extensively besides checking that import sklearn.externals.joblib.numpy_pickle works in both cases and looking that sklearn.externals.joblib.numpy_pickle.__file__ makes sense.

I tried a bit more and the sys.modules hack does not seem to solve all the pickling problems ... in particular there seems to be some problems with the compressed pickles that I haven't fully understood.

While I am at it, a less hacky way of doing it may be to use sys.meta_path. It does not seem that easy, but maybe good examples to look at would be python-future and/or six.

Yeah, but how likely is it that it would still work with some other packages that use sys.module / sys.meta_path hacks (for instance pyinstaller/pyinstaller#2246 )?

It seems that our only option is to invert _joblib and joblib. It seems
the wrong API, but I don't see another way of doing it without breaking
backward compat.

If the documentations repeats often enough that everything under sklearn.externals is private and should not be used in user code maybe that would be enough?

Another way of looking at this, could be to raise a warning if the bundled joblib (e.g. sklearn.externals.joblib) is imported from outside of sklearn. That would allow keeping just one joblib module and remove the issue with the public/private naming convention, but I'm not not sure if it's technically feasible (might also involve some sys hacks) and even if it is, desirable.