Conversation
Codecov Report
@@ Coverage Diff @@
## master #395 +/- ##
==========================================
- Coverage 91.61% 82.67% -8.95%
==========================================
Files 3 3
Lines 656 658 +2
Branches 135 135
==========================================
- Hits 601 544 -57
- Misses 34 90 +56
- Partials 21 24 +3
Continue to review full report at Codecov.
|
jakirkham
left a comment
jakirkham
left a comment
There was a problem hiding this comment.
Thanks Olivier! 😄 This seems like a nice improvement. Had a small question below
| # and get the C implementation of Pickler take the | ||
| # re-assignment into account. | ||
| self.dispatch_table = ChainMap( | ||
| {}, # to register instance-level custom reducers |
There was a problem hiding this comment.
Should we have an internal variable to point to this?
There was a problem hiding this comment.
That's a good suggestion. I still need to find the time to understand the cause of the PyPy segfault.
|
Actually before considering merging this change we might also want to do some benchmark. Maybe |
|
Note that we found a workaround for the problem we got in the subclass in loky: joblib/loky#272. Still this behavior is really unexpected so instance level isolation and lazy init of the |
|
Yeah this change still seems reasonable to me FWIW 🙂 |
|
Out project has just stumbled on this issue. What is left to do? Do you need any help completing this? |
|
Probably still the PyPy segfaulting issue noted here ( #395 (comment) ) |
|
@pierreglaser this PR requires to be updated (or reworked from scratch) now that we merged #517. I plan to tag 3.0.0 today and publish to pypi.org and conda-forge on Monday, so I don't want to do that work for 3.0.0. But if anybody is interesting in working on this, we could cut a quick 3.1.0 release in the coming weeks. |
3 participants
This change is necessary to make it natural to subclass
CloudPicklerwith a per-instance, custom dispatch table.joblib/loky#260
I also took the opportunity to make it such that
dispatch_tableare isolated by default: meaning that mutated one does not impact other instances.I still need to document this change in a changelog entry.
ping @pierreglaser @tomMoral @jakirkham you might be interested in this.