-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
use __reduce__ for pickling instead of __setstate__ / __getstate__ #1089
base: main
Are you sure you want to change the base?
Changes from all commits
dc7af2c
c1b278e
2c6c779
364d793
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2353,13 +2353,18 @@ def hexdigest(self, hashname="sha1"): | |
|
||
return checksum | ||
|
||
def __getstate__(self): | ||
return (self._store, self._path, self._read_only, self._chunk_store, | ||
self._synchronizer, self._cache_metadata, self._attrs.cache, | ||
self._partial_decompress, self._write_empty_chunks, self._version) | ||
|
||
def __setstate__(self, state): | ||
self.__init__(*state) | ||
def __reduce__(self): | ||
args = (self.store, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure which to use, but the public properties seemed more intuitive to me. Happy to change to using the private properties if there's a material difference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd stick with what is already here |
||
self.path, | ||
self.read_only, | ||
self.chunk_store, | ||
self.synchronizer, | ||
self._cache_metadata, | ||
self.attrs.cache, | ||
self._partial_decompress, | ||
self.write_empty_chunks, | ||
self._version) | ||
return (self.__class__, args) | ||
|
||
def _synchronized_op(self, f, *args, **kwargs): | ||
|
||
|
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.
Thoughts on implementing
__getnewargs_ex__
instead?In particular this Python doc text seems relevant:
Same question with other changes below
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.
how does implementing
__getnewargs_ex__
avoid the need to call__init__
directly in__setstate__
?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.
Because this only returns the arguments that are passed to
__new__
and not a function to call (nor does it call one) itself. Also because__setstate__
would be dropped then.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.
Note: We may be able to use
__getnewargs__
instead of__getnewargs_ex__
if we only use positional arguments, which is preferredFrom
__getnewargs_ex__
docs: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 how is passing arguments to
__new__
helpful here? We need to run routines contained in__init__
to properly set up the class instanceThere 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.
if you could point me to an example of this pattern it would be helpful i think
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.
__new__
is run before__init__
. The arguments would go to__init__
as well. We just wouldn't have to do that ourselves. Details in these docs.Sure this would work
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.
OK, that's very helpful! I'll look into using this instead of
__reduce__