Make sure that frozenlists are pickle-able #718
Conversation
csm10495
force-pushed
the
master
branch
4 times, most recently
from
6e9dbf6 to
e6946fd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #718 +/- ##
==========================================
+ Coverage 94.90% 95.35% +0.44%
==========================================
Files 10 10
Lines 726 796 +70
Branches 48 60 +12
==========================================
+ Hits 689 759 +70
Misses 13 13
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
I think this needs to introduce snapshot testing — a mechanic allowing us to be sure that pickling/unpickling remains working across different versions of frozenlist. The idea is that they'd be stored in-repo and so pickles created now would keep being checked for years to come. Multidict has something like that but it's too DIY.
Could you try some of the proper plugins?
pytest-harvest: https://smarie.github.io/python-pytest-harvestsyrupy: https://til.simonwillison.net/pytest/syrupy / https://syrupy-project.github.io/syrupypytest-insta: https://github.com/vberlier/pytest-instainline-snapshot: https://15r10nk.github.io/inline-snapshot
syrupy and inline-snapshot look quite active. But since I haven't played with them, I can't tell which one would feel more organic to use. Hence the need for some research.
I'm not fully sure I follow here. Pickling is safe when the serialized data is sent to newer python versions, but not necessarily older ones. I don't think we have a guarantee that we can pickle to binary then unpickle years from now if the structure changes. What types of guarantees are you looking to test? |
Mostly that a pickle created with older frozenlist can be unpickled into the same structure with a newer one. Do you think it's an overkill for now? |
|
Looks like GH autoclosed this because of the keyword 😂. Reopening. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds pickle and deepcopy support to FrozenList, enabling it to be used with multiprocessing. The implementation adds __reduce__ and __deepcopy__ methods to both the pure Python (PyFrozenList) and Cython (CFrozenList) implementations, with proper handling of the frozen state and circular references.
Key changes:
- Implemented
__reduce__and__deepcopy__methods for both PyFrozenList and CFrozenList - Added comprehensive tests for deepcopy with circular references and pickling
- Added contributor and changelog entries
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frozenlist/init.py | Added __deepcopy__ and __reduce__ methods to PyFrozenList, plus helper function _reconstruct_pyfrozenlist for unpickling |
| frozenlist/_frozenlist.pyx | Added __deepcopy__, __reduce__, and __setstate__ methods to the Cython FrozenList implementation |
| tests/test_frozenlist.py | Added tests for deepcopy with circular frozen lists and pickling with both frozen and unfrozen states |
| CONTRIBUTORS.txt | Added Charles Machalow to the contributors list |
| CHANGES/718.bugfix.rst | Added changelog entry documenting the pickle support bugfix |
| docs/spelling_wordlist.txt | Added "unpickled" to the spelling wordlist |
| CHANGES/650.contrib | Reformatted multi-line entry to single line (incidental change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I think its overkill since as long as the physical structure of frozenlist doesn't change in a non compatible way (like internal property names changing) it would be forwards compatible via pickle. In terms of other cases: I don't think libraries typically guarantee pickling cross versions anyways. The big use case is for serialization by multiprocessing. |
Add contributors / changes updates Add missing file
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
|
A forward compatibility test would be nice (though I won't insist). Such a test would look something like this: |
I'm not a huge fan of it but I'll add one. I'm don't think there are guarantees for binary format cross version normally. |
…r memo checking in deepcopy, use better way for self-including check test
Yeah, unfortunately, that test comes from users complaining when we broke forward compatibility. We won't guarantee it, but we can atleast avoid it in patch releases and warn them in the changelog. |
… a bit more natively. Make the testing a bit smarter to auto skip CFrozenList tests if they don't apply.
3 participants
What do these changes do?
Fixes bug where frozenlists can't be pickled.
Are there changes in behavior for the user?
Not directly. Though this fixes a behavior change introduced after 1.6.0
Related issue number
#683
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<category>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.bugfix: Signifying a bug fix..feature: Signifying a new feature..breaking: Signifying a breaking change or removal of something public..doc: Signifying a documentation improvement..packaging: Signifying a packaging or tooling change that may be relevant to downstreams..contrib: Signifying an improvement to the contributor/development experience..misc: Anything that does not fit the above; usually, something not of interest to users.