@aabadie if you are around and have some spare bandwidth, I would definitely be interested to have your opinion on this!

I may have overlooked some important reasons about why __reduce__ functions were implemented this way.

Codecov Report

Merging #746 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
- Coverage    95.4%   95.36%   -0.05%     
==========================================
  Files          43       43              
  Lines        6160     6191      +31     
==========================================
+ Hits         5877     5904      +27     
- Misses        283      287       +4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ce40a7...c24775e. Read the comment docs.

it feels like there is some tension between hashing and cloning in the implementation of __reduce__.

Just to elaborate on this, let's take the example of verbose in Memory:

• argument for hashing. If you have a snippet like this:

memory = Memory(location='/my/location')
pipe = Pipeline(memory=memory, ...)

func_cached = memory.cache(func)

func_cached(pipe)

you don't want the hash of pipe to change if you change memory.verbose because that could lead to time-consuming recomputations. In this case, you don't want verbose in Memory.__reduce__.

• argument for cloning. You have a snippet like this:

memory = Memory(location='/my/location', verbose=10)
memory_copy = copy.deepcopy(memory)
assert memory_copy.verbose == 10  # this seems like a very reasonable expectation

You need verbose in Memory.__reduce__ if you want the assert to pass: if verbose is not in Memory.__reduce__, memory_copy.verbose will be the verbose default value.

I think this is more complex than it should be. I don't think we need to introduce _reconstruction_from_parameters_dict.

I tried using __getstate__ to make it simpler. Looking at the code in 0.11, it feels like all the __reduce__ was trying to do is ignore timestamp (for hashing purposes)

I don't think we should change the stored attributes on the objects just to make pickling easier to manage.

I have reduced attribute changes to a minimum. The only thing I have kept is compress because it is needed to be passed properly to MemorizedFunc

And I wonder if the tests are too complicated to read: what are the practical differences between this and my tests?

IIRC you were only testing Memory, whereas because I found problems with MemorizedFunc and MemorizedResult so I decided to test them too. More than happy to make the test easier to read if you have suggestions!

This does not solve the problem of piclkle in 0.11 and load in 0.12 though:

>>> import pickle
>>> pickle
pickle.loads(b'\x80\x03cjoblib.memory\nMemory\nq\x00(X\t\x00\x00\x00/tmp/testq\x01N\x89K\x01tq\x02Rq\x03.')
Traceback (most recent call last):
  File "<ipython-input-3-1cde88ec75ed>", line 2, in <module>
    pickle.loads(b'\x80\x03cjoblib.memory\nMemory\nq\x00(X\t\x00\x00\x00/tmp/testq\x01N\x89K\x01tq\x02Rq\x03.')
  File "/volatile/ogrisel/code/joblib/joblib/memory.py", line 837, in __init__
    location, cachedir))
ValueError: You set both "location='/tmp/test' and "cachedir=False". 'cachedir' has been deprecated in version 0.12 and will be removed in version 0.14.
Please only set "location='/tmp/test'"

@lesteve restoring the __init__ args as follows restores the compatibility with 0.11:

diff --git a/joblib/memory.py b/joblib/memory.py
index 016554a..eac15b7 100644
--- a/joblib/memory.py
+++ b/joblib/memory.py
@@ -809,9 +809,9 @@ class Memory(Logger):
     # Public interface
     # ------------------------------------------------------------------------
 
-    def __init__(self, location=None, backend='local', cachedir=None,
-                 mmap_mode=None, compress=False, verbose=1, bytes_limit=None,
-                 backend_options=None):
+    def __init__(self, location=None, mmap_mode=None, compress=False,
+                 verbose=1, bytes_limit=None, backend='local',
+                 backend_options=None, cachedir=None):
         # XXX: Bad explanation of the None value of cachedir
         Logger.__init__(self)
         self._verbose = verbose

and all the test still pass along with the scikit-learn example as explained in #743 (comment).

Thanks for providing a fix @lesteve. Using set_state looks more robust than reduce.

I may have overlooked some important reasons about why reduce functions were implemented this way.

The changes here may impact related projects that already provide custom store backends, like joblib-s3 or joblib-hadoop. They will probably need an update (not verified yet)

Thanks for the fix @lesteve!

@jnothman I think I prefer the __getstate__ solution to yours and plan to merge this PR so as to be able to release joblib 0.12.2 with this fix before scikit-learn 0.20 final. Any final review/comment prior to merging?

@lesteve, just in case #752 was merged and this one needs rebase now.

Rebased.

Adding an entry in the changelog would be great.

I added an entry in CHANGES.rst. Feel free to improve it!

Ok merged. Thank you very much @lesteve!