Fix numpy.dtype hashing for numpy >= 1.20
#1136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pierreglaser
commented
Dec 4, 2020
Codecov Report
@@ Coverage Diff @@
## master #1136 +/- ##
==========================================
+ Coverage 94.50% 94.52% +0.02%
==========================================
Files 47 47
Lines 6933 6960 +27
==========================================
+ Hits 6552 6579 +27
Misses 381 381
Continue to review full report at Codecov.
|
pierreglaser
commented
Dec 4, 2020
make a deepcopy of each dtype before hashing them in order to prevent spurious pickle memoization that can introduce undesired inconcistency in the hash value (created from a pickle string)
pierreglaser
force-pushed
the
fix-dtype-pickling
branch
from
e0b9cb5
to
4189aba
Compare
|
(rebased) |
ogrisel
reviewed
Dec 4, 2020
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
ogrisel
reviewed
Dec 4, 2020
pierreglaser
commented
Dec 10, 2020
ogrisel
approved these changes
Dec 11, 2020
pierreglaser
commented
Dec 11, 2020
|
Merged! |
|
I did not follow all the discussion but it looked like a complicated one to get right! |
10 tasks
This was referenced Mar 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to fix #1080, where our old way of hashing a
numpy.dtypewas broken by recent changes innumpy.dtypeimplementation, I propose to rely on loss-less, standard pickling to hashdtypes. The risk, (which is the reason, I assume, it was not done this way already) is that thepicklememoization process will interfere will hashing and create spurious changes inpicklestring ofdtypeswith the final consequence of assigning different hash values for seemingly identical objects (see more detail in the comments that I wrote in the code introduced by this PR). To short-circuit memoization, I propose to make a deepcopy of each type that is hashed prior to hashing it.Note: this change in
dtypehashing makes that in the future, when users will upgrade theirjoblibto a release that include this code, all hashes (and thusjoblibcaches) generated using previousversionwill be invalidated. This conflicts with a semi-public contract of joblib (written in the comments of tests, at least) that upgrades of joblib should not interfere with caching.We should circumvent this issue by including this change in
joblib 1.0(note the major version bump), and not before. This should happen soon though, asnumpy 1.20is planned to be released in the near future.In the future,
joblibshould not claim to guarantee hashing consistency across environment changes (be the change related tojoblib, or any other library).TODO:
joblibdoes not guarantee hashing consistency/cache validation as soon as the working environment is altered (update of either joblib, or any other library)CHANGEScc @ogrisel. Still a bit wip, but first batch of comments welcomed.