Repository: joblib/joblib
Pull request #1359 (2022-11-08 16:28:45)

Do not swallow PicklingError
Currently, when caching fails because the function output can't be pickled, there is no warning, but the output will not be cached. This surprises users, who expect their output to be cached.

This is related to https://github.com/joblib/joblib/issues/952 and https://github.com/joblib/joblib/issues/896 . It does not solve the underlying race condition in https://github.com/joblib/joblib/issues/896, but it does implement the suggestion made by @tomMoral [in this comment](https://github.com/joblib/joblib/issues/952#issuecomment-546370327): it just raises an error when the caching fails due to pickling.

codecov[bot]: # [Codecov](https://codecov.io/gh/joblib/joblib/pull/1359?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib) Report
Base: **94.02**% // Head: **70.75**% // Decreases project coverage by **`-23.26%`** :warning:
> Coverage data is based on head [(`6bfd045`)](https://codecov.io/gh/joblib/joblib/pull/1359?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib) compared to base [(`854458e`)](https://codecov.io/gh/joblib/joblib/commit/854458ee53301e4366f4ece2d4292d81dfe348e2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib).
> Patch coverage: 33.33% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           master    #1359       +/-   ##
===========================================
- Coverage   94.02%   70.75%   -23.27%     
===========================================
  Files          52       52               
  Lines        7292     7108      -184     
===========================================
- Hits         6856     5029     -1827     
- Misses        436     2079     +1643     
```


| [Impacted Files](https://codecov.io/gh/joblib/joblib/pull/1359?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib) | Coverage Δ | |
|---|---|---|
| [joblib/\_store\_backends.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL19zdG9yZV9iYWNrZW5kcy5weQ==) | `89.58% <33.33%> (-1.70%)` | :arrow_down: |
| [joblib/test/test\_dask.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL3Rlc3QvdGVzdF9kYXNrLnB5) | `3.92% <0.00%> (-87.27%)` | :arrow_down: |
| [joblib/executor.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL2V4ZWN1dG9yLnB5) | `30.95% <0.00%> (-69.05%)` | :arrow_down: |
| [joblib/\_dask.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL19kYXNrLnB5) | `24.01% <0.00%> (-68.67%)` | :arrow_down: |
| [joblib/test/testutils.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL3Rlc3QvdGVzdHV0aWxzLnB5) | `33.33% <0.00%> (-66.67%)` | :arrow_down: |
| [joblib/test/test\_memmapping.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL3Rlc3QvdGVzdF9tZW1tYXBwaW5nLnB5) | `34.33% <0.00%> (-64.90%)` | :arrow_down: |
| [joblib/pool.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL3Bvb2wucHk=) | `27.04% <0.00%> (-61.57%)` | :arrow_down: |
| [joblib/\_multiprocessing\_helpers.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL19tdWx0aXByb2Nlc3NpbmdfaGVscGVycy5weQ==) | `26.92% <0.00%> (-61.54%)` | :arrow_down: |
| [joblib/\_memmapping\_reducer.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL19tZW1tYXBwaW5nX3JlZHVjZXIucHk=) | `38.34% <0.00%> (-57.54%)` | :arrow_down: |
| [joblib/test/test\_module.py](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib#diff-am9ibGliL3Rlc3QvdGVzdF9tb2R1bGUucHk=) | `52.94% <0.00%> (-47.06%)` | :arrow_down: |
| ... and [28 more](https://codecov.io/gh/joblib/joblib/pull/1359/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/joblib/joblib/pull/1359?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=joblib).

Nielius: Hi,

Thanks for the quick feedback @tomMoral !

I've addressed your feedback by:

- moving the try/except block
- changing to a warning instead of raising an exception
- adding a unit test to test that a warning is being logged

In addition, I've also changed the catch-all except block to log a warning. When I was writing the test, I noticed how frustrating it was that it literally swallows every kind of error. I think the `warnings.warn( ...)` is better than just doing nothing --- do you agree?
ogrisel: We could even have a constructor parameter on the `Memory` object: `error="ignore"`, `error="warn"`, or `error="raise"`.

I would be in favor of using `error="warn"` by default but users who prefer other options are free to switch easily.
tomMoral: This feels a bit YAGNI to me. There are already many parameters that users don't understand, I would avoid adding some if possible.
The rational of swallowing error linked to dumping was that caching should not create extra problems due to OS (memory/ disk space/collision). So thinking back, I would say that raising a warning for any error except pickling one is the right behavior.
That way, users can implement the three behaviors simply with warning filters? WDYT?
ogrisel: Alright then.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 72e983e..6f2dd8a 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -22,6 +22,11 @@ In development
   in May 2022) that it should be safe to remove this.
   https://github.com/joblib/joblib/pull/1361
 
+- A warning is raised when a pickling error occurs during caching operations.
+  In version 1.5, this warning will be turned into an error. For all other
+  errors, a new warning has been introduced: `joblib.memory.CacheWarning`.
+  https://github.com/joblib/joblib/pull/1359
+
 Release 1.2.0
 -------------
 
diff --git a/joblib/_store_backends.py b/joblib/_store_backends.py
index e96f306..21a4958 100644
--- a/joblib/_store_backends.py
+++ b/joblib/_store_backends.py
@@ -1,5 +1,6 @@
 """Storage providers backends for Memory caching."""
 
+from pickle import PicklingError
 import re
 import os
 import os.path
@@ -20,6 +21,11 @@ CacheItemInfo = collections.namedtuple('CacheItemInfo',
                                        'path size last_access')
 
 
+class CacheWarning(Warning):
+    """Warning to capture dump failures except for PicklingError."""
+    pass
+
+
 def concurrency_safe_write(object_to_write, filename, write_func):
     """Writes an object into a unique file in a concurrency-safe way."""
     thread_id = id(threading.current_thread())
@@ -185,12 +191,24 @@ class StoreBackendMixin(object):
 
             def write_func(to_write, dest_filename):
                 with self._open_item(dest_filename, "wb") as f:
-                    numpy_pickle.dump(to_write, f,
-                                      compress=self.compress)
+                    try:
+                        numpy_pickle.dump(to_write, f, compress=self.compress)
+                    except PicklingError as e:
+                        # TODO(1.5) turn into error
+                        warnings.warn(
+                            "Unable to cache to disk: failed to pickle "
+                            "output. In version 1.5 this will raise an "
+                            f"exception. Exception: {e}.",
+                            FutureWarning
+                        )
 
             self._concurrency_safe_write(item, filename, write_func)
-        except:  # noqa: E722
-            " Race condition in the creation of the directory "
+        except Exception as e:  # noqa: E722
+            warnings.warn(
+                "Unable to cache to disk. Possibly a race condition in the "
+                f"creation of the directory. Exception: {e}.",
+                CacheWarning
+            )
 
     def clear_item(self, path):
         """Clear the item at the path, given as a list of strings."""
diff --git a/joblib/memory.py b/joblib/memory.py
index dc95d2b..b27c1fc 100644
--- a/joblib/memory.py
+++ b/joblib/memory.py
@@ -30,6 +30,7 @@ from .func_inspect import format_call
 from .func_inspect import format_signature
 from .logger import Logger, format_time, pformat
 from ._store_backends import StoreBackendBase, FileSystemStoreBackend
+from ._store_backends import CacheWarning  # noqa
 
 
 FIRST_LINE_TEXT = "# first line:"


Test patch:
diff --git a/joblib/test/test_store_backends.py b/joblib/test/test_store_backends.py
index 2c6198f..e5db167 100644
--- a/joblib/test/test_store_backends.py
+++ b/joblib/test/test_store_backends.py
@@ -1,3 +1,4 @@
+
 try:
     # Python 2.7: use the C pickle to speed up
     # test_concurrency_safe_write which pickles big python objects
@@ -5,13 +6,20 @@ try:
 except ImportError:
     import pickle as cpickle
 import functools
+from pickle import PicklingError
 import time
 
+import pytest
+
 from joblib.testing import parametrize, timeout
 from joblib.test.common import with_multiprocessing
 from joblib.backports import concurrency_safe_rename
 from joblib import Parallel, delayed
-from joblib._store_backends import concurrency_safe_write
+from joblib._store_backends import (
+    concurrency_safe_write,
+    FileSystemStoreBackend,
+    CacheWarning,
+)
 
 
 def write_func(output, filename):
@@ -54,3 +62,33 @@ def test_concurrency_safe_write(tmpdir, backend):
              if i % 3 != 2 else load_func for i in range(12)]
     Parallel(n_jobs=2, backend=backend)(
         delayed(func)(obj, filename) for func in funcs)
+
+
+def test_warning_on_dump_failure(tmpdir):
+    # Check that a warning is raised when the dump fails for any reason but
+    # a PicklingError.
+    class UnpicklableObject(object):
+        def __reduce__(self):
+            raise RuntimeError("some exception")
+
+    backend = FileSystemStoreBackend()
+    backend.location = tmpdir.join('test_warning_on_pickling_error').strpath
+    backend.compress = None
+
+    with pytest.warns(CacheWarning, match="some exception"):
+        backend.dump_item("testpath", UnpicklableObject())
+
+
+def test_warning_on_pickling_error(tmpdir):
+    # This is separate from test_warning_on_dump_failure because in the
+    # future we will turn this into an exception.
+    class UnpicklableObject(object):
+        def __reduce__(self):
+            raise PicklingError("not picklable")
+
+    backend = FileSystemStoreBackend()
+    backend.location = tmpdir.join('test_warning_on_pickling_error').strpath
+    backend.compress = None
+
+    with pytest.warns(FutureWarning, match="not picklable"):
+        backend.dump_item("testpath", UnpicklableObject())