Skip to content

Instance-level dispatch table#395

Open
ogrisel wants to merge 1 commit into
cloudpipe:masterfrom
ogrisel:lazy-init-dispatch-table
Open

Instance-level dispatch table#395
ogrisel wants to merge 1 commit into
cloudpipe:masterfrom
ogrisel:lazy-init-dispatch-table

Conversation

@ogrisel
Copy link
Copy Markdown
Contributor

@ogrisel ogrisel commented Sep 22, 2020

This change is necessary to make it natural to subclass CloudPickler with a per-instance, custom dispatch table.

joblib/loky#260

I also took the opportunity to make it such that dispatch_table are 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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 22, 2020

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.67%. Comparing base (d3279a0) to head (d6a0263).
⚠️ Report is 90 commits behind head on master.

Files with missing lines Patch % Lines
cloudpickle/cloudpickle_fast.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have an internal variable to point to this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. I still need to find the time to understand the cause of the PyPy segfault.

@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Sep 23, 2020

Actually before considering merging this change we might also want to do some benchmark. Maybe ChainMap is not that fast when doing many consecutive getitems with maybe misses on the first map which will be empty most of the time.

@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Sep 24, 2020

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 dispatch_table might be more intuitive for other subclassing this Pickler.

@jakirkham
Copy link
Copy Markdown
Member

Yeah this change still seems reasonable to me FWIW 🙂

faucct pushed a commit to mrMakaronka/pyenv that referenced this pull request Apr 13, 2021
@faucct
Copy link
Copy Markdown

faucct commented Apr 13, 2021

Out project has just stumbled on this issue. What is left to do? Do you need any help completing this?

@jakirkham
Copy link
Copy Markdown
Member

Probably still the PyPy segfaulting issue noted here ( #395 (comment) )

@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Oct 13, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants