-
-
Notifications
You must be signed in to change notification settings - Fork 814
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[tool]: update VarAccess pickle implementation #4270
Conversation
can we add an explainer comment on why's this necessary? |
will get to it |
# see https://github.com/python/cpython/issues/124937#issuecomment-2392227290 | ||
def __reduce__(self): | ||
dict_obj = {f.name: getattr(self, f.name) for f in fields(self)} | ||
return self.__class__._produce, (dict_obj,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use the following trick if all fields can be passed as keyword arguments to constructor in subclasses:
return self.__class__._produce, (dict_obj,) | |
return partial(self.__class__, **dict_obj), () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, is there a performance difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be slower. The advantage is that you do not need to add private method _produce()
.
copying in some offline discussion with @cyberthirst about why the hash is safe even though the data structure is cyclic:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4270 +/- ##
==========================================
- Coverage 91.30% 90.07% -1.23%
==========================================
Files 110 110
Lines 15764 15770 +6
Branches 3461 3463 +2
==========================================
- Hits 14393 14205 -188
- Misses 935 1095 +160
- Partials 436 470 +34 ☔ View full report in Codecov by Sentry. |
What I did
fix a bug where unpickling
annotated_vyper_module
would lead to a crash:a repro can be found at charles-cooper@2a5facf.
this is a blocker for tooling, for instance titanoboa relies on pickle/unpickle to cache CompilerData objects:
https://github.com/vyperlang/titanoboa/blob/86df8936654db20686410488738d7abaf165a4c9/boa/util/disk_cache.py#L65-L66
the underlying issue is that
pickle.loads()
callsobj.__hash__()
for objects that are keys in a hashed data structure - namely, dicts, sets and frozensets. this causes a crash when there is a cycle in the object graph, because the object is not fully instantiated at the time that__hash__()
is called. this is a cpython bug, reported at python/cpython#124937.@serhiy-storchaka suggested the approach taken in this PR, which breaks the loop before pickling: python/cpython#124937 (comment).
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture