Do not swallow PicklingError #1359
Conversation
Codecov ReportBase: 93.32% // Head: 93.96% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1359 +/- ##
==========================================
+ Coverage 93.32% 93.96% +0.63%
==========================================
Files 52 52
Lines 7300 7328 +28
==========================================
+ Hits 6813 6886 +73
+ Misses 487 442 -45
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Hello,
Thanks a lot for this PR :)
It looks simple enough but this change could break existing code, so we should do the change in two steps:
- First raise a warning (instead of swallowing the error).
- Then in a later version change to raise the
PicklingError.
Could you change this to raise a warning instead? Also could you please move the try/except in the write_func?
This might be worth it to add a test, to make sure the error is correctly raised.
|
Hi, Thanks for the quick feedback @tomMoral ! I've addressed your feedback by:
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 |
joblib/_store_backends.py
Outdated
| 'failed to pickle output. ' | ||
| 'In future versions of joblib ' | ||
| 'this will raise an exception.' | ||
| 'Exception: %s.' % e, |
There was a problem hiding this comment.
We try to avoid the old formating synthax in the new code base, so please use:
| 'Exception: %s.' % e, | |
| f'Exception: {e}.' |
There was a problem hiding this comment.
Also, could you please rewrap the string to better use the available <80 char limit of PEP8?
Some people complain than 80 is not enough, others seem reluctant to use more than 60 apparently ;) 80 is a good middle ground.
joblib/_store_backends.py
Outdated
| except Exception as e: # noqa: E722 | ||
| warnings.warn( | ||
| 'Unable to cache to disk. ' | ||
| 'Possibly a race condition in the creation of the directory. ' | ||
| 'Exception: %s' % e) |
There was a problem hiding this comment.
As we are changing a bit this, I would be in favor of sanitizing this, by putting a try/except for OSError in create_location, and having this raise a deprecation warning saying that any other error will be raised in future version of joblib. WDYT?
Small nitpick on style.
| except Exception as e: # noqa: E722 | |
| warnings.warn( | |
| 'Unable to cache to disk. ' | |
| 'Possibly a race condition in the creation of the directory. ' | |
| 'Exception: %s' % e) | |
| except Exception as e: # noqa: E722 | |
| warnings.warn( | |
| 'Unable to cache to disk. ' | |
| 'Possibly a race condition in the creation of the directory. ' | |
| f'Exception: {e}' | |
| ) |
There was a problem hiding this comment.
So following the conversation with @ogrisel , I would not do a deprecation warning but just a warning for the error.
Maybe create a specfic CacheWarning that will be easy to filter out?
|
We could even have a constructor parameter on the I would be in favor of using |
|
This feels a bit YAGNI to me. There are already many parameters that users don't understand, I would avoid adding some if possible. |
|
Alright then. |
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 #952 and #896 . It does not solve the underlying race condition in #896, but it does implement the suggestion made by @tomMoral in this comment: it just raises an error when the caching fails due to pickling.