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 pickle protocol #143

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Fix pickle protocol #143

merged 2 commits into from
Apr 19, 2022

Conversation

olebole
Copy link
Member

@olebole olebole commented Apr 12, 2022

Protocol version 4 is supported since Python 3.4 and the default starting with Python 3.8.

Fixing the protocol version allows to downgrade the Python version. To make it more robust, also trigger a re-build of the cache entry if the cache was written by an unsupported protocol version.

Fixes #142.

Protocol version 4 is supported since Python 3.4 and the default
starting with Python 3.8.
@jehturner
Copy link
Contributor

Thanks, Ole!

This fixes my problem with starting PyRAF under 3.7 after initially creating the cache under 3.9 (both with this branch). However, if I start PyRAF under 3.7 with an existing cache entry for "gemini" that was created using 2.2.1 under Python 3.8, I get this when trying to load the package:

PyRAF 2.2.1.dev53+g50abdfe
Python/CL command line wrapper
  .help describes executive commands
--> gemini
Traceback (innermost last):
  File "<CL script CL1>", line 1, in <module>
KeyError: 'fd14f25516d1b332bba797340d93d9494c'

Not sure how much we care about that (I have a meeting now, so don't have much time to think about it) but it doesn't seem as intended?

@jehturner
Copy link
Contributor

The above error (from the new try ... except, at L113) only occurs when using PyRAF 2.2.1 on Python 3.8/3.9, followed by this branch on 3.7. The comment suggests that the KeyError will cause the cache entry to be rebuilt, but apparently that is not happening?

This way we make sure that we unpickle only objects with a supported
pickle format version.
@olebole
Copy link
Member Author

olebole commented Apr 13, 2022

The idea was that when the id is not there, the old version is overwritten by one with a new pickle version. This however appears not to work.
Then, probably the best solution is to add the pickle protocol to the version string. Can you try this out again?

@jehturner
Copy link
Contributor

That version seems fine, thanks (on Linux). I suppose it will mean more old duplicate entries sitting around, but I never really see more than a few MB and people can delete them if they want to.

@olebole
Copy link
Member Author

olebole commented Apr 14, 2022

That is true; however this will happen also when we introduce another incompatible change (which changes the version of the cache). And I think that we will very rarely change the pickle protocol in future.

@olebole olebole merged commit 7a55ea1 into main Apr 19, 2022
@temuller
Copy link

Hi! I don't think this has been added to the latest version, right? pip installs pyraf v2.2.1, but this fix is not included.

@olebole olebole deleted the fix-pickle-protocol branch April 29, 2022 15:06
@olebole
Copy link
Member Author

olebole commented Apr 29, 2022

No, it is not included in 2.2.1. I didn't make a new release because downgrading Python seemed a rare case, and one can work around the problem by removing the cache after downgrading.

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

Successfully merging this pull request may close these issues.

Unsupported pickle protocol for clcache
3 participants