Repository: joblib/joblib

Issue:
Memory.__reduce__ is broken since cachedir deprecation
Memory can't be pickled/unpickled.

It defines `__reduce__` to exclude a timestamp from the pickle.

But `__reduce__` includes:

```
        return (self.__class__, (location, self.backend, self.mmap_mode,
                                 compress, self._verbose))
```

where `__init__` is:

```
    def __init__(self, location=None, backend='local', cachedir=None,
                 mmap_mode=None, compress=False, verbose=1, bytes_limit=None,
                 backend_options={})
```

`cachedir`, not `mmap_mode` is now the third positional argument.  Perhaps it should be the last, and then `__reduce__` need not be changed. Or `__reduce__` can be fixed and changed again when the deprecation is completed.

Discussion:
lesteve: @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[bot]: # [Codecov](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=h1) Report
> Merging [#746](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=desc) into [master](https://codecov.io/gh/joblib/joblib/commit/aaba4f0ce048b1c39fde202a826ea09e4d4fadba?src=pr&el=desc) will **decrease** coverage by `4.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/joblib/joblib/pull/746/graphs/tree.svg?width=650&height=150&src=pr&token=gA6LF5DGTW)](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
- Coverage   95.28%   91.26%   -4.02%     
==========================================
  Files          42       42              
  Lines        6128     6171      +43     
==========================================
- Hits         5839     5632     -207     
- Misses        289      539     +250
```


| [Impacted Files](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [joblib/test/test\_memory.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9tZW1vcnkucHk=) | `97.61% <100%> (+0.12%)` | :arrow_up: |
| [joblib/memory.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL21lbW9yeS5weQ==) | `94.98% <100%> (-0.39%)` | :arrow_down: |
| [joblib/\_memory\_helpers.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL19tZW1vcnlfaGVscGVycy5weQ==) | `3.07% <0%> (-70.77%)` | :arrow_down: |
| [joblib/backports.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL2JhY2twb3J0cy5weQ==) | `33.33% <0%> (-62.51%)` | :arrow_down: |
| [joblib/test/test\_dask.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9kYXNrLnB5) | `83.01% <0%> (-15.1%)` | :arrow_down: |
| [joblib/format\_stack.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL2Zvcm1hdF9zdGFjay5weQ==) | `71.77% <0%> (-11.49%)` | :arrow_down: |
| [joblib/\_multiprocessing\_helpers.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL19tdWx0aXByb2Nlc3NpbmdfaGVscGVycy5weQ==) | `66.66% <0%> (-9.53%)` | :arrow_down: |
| [joblib/my\_exceptions.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL215X2V4Y2VwdGlvbnMucHk=) | `88.67% <0%> (-9.44%)` | :arrow_down: |
| [joblib/\_compat.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL19jb21wYXQucHk=) | `90.9% <0%> (-9.1%)` | :arrow_down: |
| [joblib/pool.py](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree#diff-am9ibGliL3Bvb2wucHk=) | `83.62% <0%> (-7.76%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/joblib/joblib/pull/746/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=footer). Last update [aaba4f0...a31ca64](https://codecov.io/gh/joblib/joblib/pull/746?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lesteve: > 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:
```py
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:
```py
memory = Memory(location='/my/location', verbose=10)
memory_copy = copy.deepcopy(memory)
assert memory_copy.verbose == 10  # this seems like a very reasonable expectation
```
The assert will fail unless you have `verbose` in `Memory.__reduce__`.
lesteve: > 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!

ogrisel: 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'"
```
aabadie: 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)
ogrisel: 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?
aabadie: @lesteve, just in case #752 was merged and this one needs rebase now.
lesteve: Rebased.
aabadie: Adding an entry in the changelog would be great.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 35f50e2..b048c67 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -1,13 +1,17 @@
 Latest changes
 ===============
 
-Release 0.12.3
---------------
+master
+------
 
 Alexandre Abadie
 
     Fix MemorizedResult not picklable (#747).
 
+Loïc Estève
+
+    Fix Memory, MemorizedFunc and MemorizedResult round-trip pickling +
+    unpickling (#746).
 
 Release 0.12.2
 --------------
diff --git a/examples/compressors_comparison.py b/examples/compressors_comparison.py
index 5dee5a7..bc10f42 100644
--- a/examples/compressors_comparison.py
+++ b/examples/compressors_comparison.py
@@ -33,7 +33,7 @@ names = ("duration, protocol_type, service, flag, src_bytes, "
          "root_shell, su_attempted, num_root, "
          "num_file_creations, ").split(', ')
 
-data = pd.read_csv(url, names=names, nrows=1000)
+data = pd.read_csv(url, names=names, nrows=1e6)
 
 ###############################################################################
 # Dump and load the dataset without compression
@@ -196,7 +196,7 @@ p2 = plt.bar(ind, load_durations, width, bottom=dump_durations)
 plt.ylabel('Time in seconds')
 plt.title('Dump and load durations')
 plt.xticks(ind, ('Raw', 'LZ4', 'Zlib', 'LZMA'))
-plt.yticks(np.arange(0, lzma_load_duration + lzma_dump_duration, 0.01))
+plt.yticks(np.arange(0, lzma_load_duration + lzma_dump_duration))
 plt.legend((p1[0], p2[0]), ('Dump duration', 'Load duration'))
 
 ###############################################################################
diff --git a/joblib/memory.py b/joblib/memory.py
index 1cebe63..8243b79 100644
--- a/joblib/memory.py
+++ b/joblib/memory.py
@@ -262,10 +262,11 @@ class MemorizedResult(Logger):
                         func=self.func,
                         args_id=self.args_id
                         ))
-    def __reduce__(self):
-        return (self.__class__,
-                (self.store_backend, self.func, self.args_id),
-                {'mmap_mode': self.mmap_mode})
+
+    def __getstate__(self):
+        state = self.__dict__.copy()
+        state['timestamp'] = None
+        return state
 
 
 class NotMemorizedResult(object):
@@ -330,9 +331,6 @@ class NotMemorizedFunc(object):
     def call_and_shelve(self, *args, **kwargs):
         return NotMemorizedResult(self.func(*args, **kwargs))
 
-    def __reduce__(self):
-        return (self.__class__, (self.func,))
-
     def __repr__(self):
         return '{0}(func={1})'.format(self.__class__.__name__, self.func)
 
@@ -392,6 +390,7 @@ class MemorizedFunc(Logger):
         self.mmap_mode = mmap_mode
         self.compress = compress
         self.func = func
+
         if ignore is None:
             ignore = []
         self.ignore = ignore
@@ -516,13 +515,13 @@ class MemorizedFunc(Logger):
     def __call__(self, *args, **kwargs):
         return self._cached_call(args, kwargs)[0]
 
-    def __reduce__(self):
+    def __getstate__(self):
         """ We don't store the timestamp when pickling, to avoid the hash
             depending from it.
-            In addition, when unpickling, we run the __init__
         """
-        return (self.__class__, (self.func, self.store_backend, self.ignore,
-                self.mmap_mode, self.compress, self._verbose))
+        state = self.__dict__.copy()
+        state['timestamp'] = None
+        return state
 
     # ------------------------------------------------------------------------
     # Private interface
@@ -823,6 +822,7 @@ class Memory(Logger):
         self.timestamp = time.time()
         self.bytes_limit = bytes_limit
         self.backend = backend
+        self.compress = compress
         if backend_options is None:
             backend_options = {}
         self.backend_options = backend_options
@@ -906,9 +906,11 @@ class Memory(Logger):
             mmap_mode = self.mmap_mode
         if isinstance(func, MemorizedFunc):
             func = func.func
-        return MemorizedFunc(func, self.store_backend, mmap_mode=mmap_mode,
-                             ignore=ignore, verbose=verbose,
-                             timestamp=self.timestamp)
+        return MemorizedFunc(func, location=self.store_backend,
+                             backend=self.backend,
+                             ignore=ignore, mmap_mode=mmap_mode,
+                             compress=self.compress,
+                             verbose=verbose, timestamp=self.timestamp)
 
     def clear(self, warn=True):
         """ Erase the complete cache directory.
@@ -945,15 +947,10 @@ class Memory(Logger):
             self.__class__.__name__, (repr(None) if self.store_backend is None
                                       else repr(self.store_backend)))
 
-    def __reduce__(self):
+    def __getstate__(self):
         """ We don't store the timestamp when pickling, to avoid the hash
             depending from it.
-            In addition, when unpickling, we run the __init__
         """
-        # We need to remove 'joblib' from the end of cachedir
-        location = (repr(self.store_backend)[:-7]
-                    if self.store_backend is not None else None)
-        compress = self.store_backend.compress \
-            if self.store_backend is not None else False
-        return (self.__class__, (location, self.backend, self.mmap_mode,
-                                 compress, self._verbose))
+        state = self.__dict__.copy()
+        state['timestamp'] = None
+        return state


Test patch:
diff --git a/joblib/test/test_memory.py b/joblib/test/test_memory.py
index 4690b5b..b333a48 100644
--- a/joblib/test/test_memory.py
+++ b/joblib/test/test_memory.py
@@ -14,6 +14,8 @@ import sys
 import time
 import datetime
 
+import pytest
+
 from joblib.memory import Memory
 from joblib.memory import MemorizedFunc, NotMemorizedFunc
 from joblib.memory import MemorizedResult, NotMemorizedResult
@@ -27,6 +29,7 @@ from joblib.test.common import with_numpy, np
 from joblib.test.common import with_multiprocessing
 from joblib.testing import parametrize, raises, warns
 from joblib._compat import PY3_OR_LATER
+from joblib.hashing import hash
 
 
 ###############################################################################
@@ -986,3 +989,52 @@ def test_memorized_result_pickle(tmpdir):
     assert memorized_result.func == memorized_result_loads.func
     assert memorized_result.args_id == memorized_result_loads.args_id
     assert str(memorized_result) == str(memorized_result_loads)
+
+
+def compare(left, right, ignored_attrs=None):
+    if ignored_attrs is None:
+        ignored_attrs = []
+
+    left_vars = vars(left)
+    right_vars = vars(right)
+    assert set(left_vars.keys()) == set(right_vars.keys())
+    for attr in left_vars.keys():
+        if attr in ignored_attrs:
+            continue
+        assert left_vars[attr] == right_vars[attr]
+
+
+@pytest.mark.parametrize('memory_kwargs',
+                         [{'compress': 3, 'verbose': 2},
+                          {'mmap_mode': 'r', 'verbose': 5, 'bytes_limit': 1e6,
+                           'backend_options': {'parameter': 'unused'}}])
+def test_memory_pickle_dump_load(tmpdir, memory_kwargs):
+    memory = Memory(location=tmpdir.strpath, **memory_kwargs)
+
+    memory_reloaded = pickle.loads(pickle.dumps(memory))
+
+    # Compare Memory instance before and after pickle roundtrip
+    compare(memory.store_backend, memory_reloaded.store_backend)
+    compare(memory, memory_reloaded,
+            ignored_attrs=set(['store_backend', 'timestamp']))
+    assert hash(memory) == hash(memory_reloaded)
+
+    func_cached = memory.cache(f)
+
+    func_cached_reloaded = pickle.loads(pickle.dumps(func_cached))
+
+    # Compare MemorizedFunc instance before/after pickle roundtrip
+    compare(func_cached.store_backend, func_cached_reloaded.store_backend)
+    compare(func_cached, func_cached_reloaded,
+            ignored_attrs=set(['store_backend', 'timestamp']))
+    assert hash(func_cached) == hash(func_cached_reloaded)
+
+    # Compare MemorizedResult instance before/after pickle roundtrip
+    memorized_result = func_cached.call_and_shelve(1)
+    memorized_result_reloaded = pickle.loads(pickle.dumps(memorized_result))
+
+    compare(memorized_result.store_backend,
+            memorized_result_reloaded.store_backend)
+    compare(memorized_result, memorized_result_reloaded,
+            ignored_attrs=set(['store_backend', 'timestamp']))
+    assert hash(memorized_result) == hash(memorized_result_reloaded)
diff --git a/joblib/test/test_parallel.py b/joblib/test/test_parallel.py
index fa0c69b..b5073b3 100644
--- a/joblib/test/test_parallel.py
+++ b/joblib/test/test_parallel.py
@@ -241,6 +241,17 @@ def test_nested_loop(parent_backend, child_backend):
         delayed(nested_loop)(child_backend) for _ in range(2))
 
 
+def raise_exception(backend):
+    raise ValueError
+
+
+def test_nested_loop_with_exception_with_loky():
+    with raises(ValueError):
+        with Parallel(n_jobs=2, backend="loky") as parallel:
+            parallel([delayed(nested_loop)("loky"),
+                      delayed(raise_exception)("loky")])
+
+
 def test_mutate_input_with_threads():
     """Input is mutable when using the threading backend"""
     q = Queue(maxsize=5)