Fix pickeling - #717
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
for more information, see https://pre-commit.ci
# Conflicts: # frozenlist/__init__.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #717 +/- ##
===========================================
- Coverage 100.00% 99.74% -0.26%
===========================================
Files 2 2
Lines 370 388 +18
Branches 9 10 +1
===========================================
+ Hits 370 387 +17
- Partials 0 1 +1
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:
|
|
@Vizonex would you bring this back up to date? |
|
@bdraco Sorry for the wait. I can. |
Mukller
left a comment
Mukller
left a comment
There was a problem hiding this comment.
I verified this branch locally against a pickle/deepcopy matrix (plain frozen/unfrozen round-trips, PyFrozenList, copy) — those all pass. However, subclasses silently lose their instance state through both pickling and deepcopy:
from frozenlist import FrozenList
class Sub(FrozenList):
def __init__(self, items=None, tag=None):
super().__init__(items)
self.tag = tag
s = Sub([9], tag="meta"); s.freeze()
r = pickle.loads(pickle.dumps(s))
type(r) is Sub # True - class survives
r.tag # None <- silently reset to the constructor default
copy.deepcopy(s).tag # None <- same hereNo crash — which makes this worse in practice: a subclass carrying metadata comes back hollow. This is exactly the case greptile flagged on #835, where a class-aware reducer fixed it (subclass instance state now survives pickling; there's a regression test for it).
Full disclosure: #835 is my PR (same underlying bug — filed as #834). I'm reviewing competing approaches so maintainers can compare directly; happy either way about which implementation lands, but any merged fix should cover the subclass-state case, since that's the difference between "pickle works" and "pickle corrupts data quietly".
Also FYI on CI: this branch currently shows ~40 failing checks while master is green — may just need a rebase onto current master.
Matrix results on this branch: plain frozen/unfrozen pickle ✓, frozen flag preserved ✓, PyFrozenList ✓, subclass state ✗, deepcopy-of-subclass state ✗.
4 participants
What do these changes do?
I forgot I had made a fork for trying to fix pickling but I seemed to never bother to add it after the issuer told me there was no need but it seems it needs to come back. This PR adds in a new argument to freeze a frozenlist right away which is a tradeoff but it was the only way I could seem to get pickling to work correctly.
Are there changes in behavior for the user?
Related issue number
fixes #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.