Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix python implementation pickling #91

Merged
merged 7 commits into from
Nov 28, 2023

Conversation

apmorton
Copy link
Contributor

@apmorton apmorton commented Nov 24, 2023

This PR is split into two commits for easier review since git doesn't correctly show the rename operation on core.py 😞 .

The first moved the pure python implementation to _frozendictpy so that core.py can become a compatibility wrapper, but makes no other changes. This allows existing pickles that mention frozendict.core.frozendict to correctly depickle as the native implementation in the future on python 3.11.

The second implements the fix for #87.

@Marco-Sulla
Copy link
Owner

Excuse me, but why you have renamed also core.py? Fixing #87 was not sufficient?

PS: I added a build and test for the pure py implementation in the pipeline, so I'm going to remove the pure py tests from the C extension tests.

Wait for it.

@apmorton
Copy link
Contributor Author

Excuse me, but why you have renamed also core.py?

I explained this in the PR description.

Existing files that have been pickled on python 3.11 mention frozendict.core.frozendict.
This means that if/when 3.11 gets a native extension these files will still depickle to the pure python implementation and you can end up with mixed native/python instances in a single process.

Moving the python implementation and setting up frozendict.core.frozendict to be an alias for frozendict.frozendict resolves this issue.

@Marco-Sulla
Copy link
Owner

This means that if/when 3.11 gets a native extension these files will still depickle to the pure python implementation and you can end up with mixed native/python instances in a single process.

Ok, but what if I pickle frozendict._frozendictpy.frozendict and then I switch to the C extension?

At this point, is it not much simpler to remove core.py?

@apmorton
Copy link
Contributor Author

Ok, but what if I pickle frozendict._frozendictpy.frozendict and then I switch to the C extension?

If you pickle frozendict._frozendictpy.frozendict it will appear in the pickle file as frozendict.frozendict because it has __module__ set to frozendict and depickle correctly in the future using the C extension.

src/frozendict/__init__.py Outdated Show resolved Hide resolved
src/frozendict/core.py Show resolved Hide resolved
test/common.py Outdated Show resolved Hide resolved
test/frozendict_only.py Outdated Show resolved Hide resolved
@Marco-Sulla Marco-Sulla merged commit 60211ac into Marco-Sulla:master Nov 28, 2023
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.

2 participants