Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor builtin method pickling #262
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
Refactor builtin method pickling #262
Changes from all commits
6d9e7e7
d741d1e
5a0081d
5a9c93c
9d9c25a
e578356
c5457b0
9d5f0f4
ac5ae5d
ce5bf75
d212c10
d4447d4
e1b59c8
8365991
0a36b98
504d0c9
16e79b5
8b2f390
dc79489
106700a
6c60eaf
43e358b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This change solves a related bug, but it deserves a different PR.
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.
Can you be more explicit? Maybe you could include a non-regression test directly in this PR and document it the changelog?
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.
precisely, python3 does not make a difference between unbound methods and functions.
Consider
pickle._Pickler.dump
: this is a non-builtin unbound method.The bug is was mentioning is that
cloudpickle
master currently fails at detecting this as global, and instead pickles this dynamically:gives:
The reason being that
pickle._Pickler.dump.__name__
is simplydump
, whereas to retrieve this function, we need to get the qualified name of this function, namely_Pickler.dump
If we use
__qualname__
instead of__name__
, we cannot usegetattr
though, becausegetattr
does not accept dotted path. Instead, we need to usepickle._getattribute
.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.
But this (usinggetattr
) is what you are doing here, no?Sorry, I misread what you said. That sounds good.
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.
So to summarize, this can only be (easily) fixed in Python 3?
I think we still need a specific test for this fix.
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.
in
python2
, unbound methods are methods, and get serialized usingsave_instancemethod
: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.
ditto