GeezerVison is showing github.com — Sixth element of tuple from __reduce__(), inconsistency between pickle and copy · Issue #90494 · python/cpython
Sixth element of tuple from __reduce__(), inconsistency between pickle and copy #90494Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2022-01-10.21:41:04.524>
labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
title = 'Sixth element of tuple from __reduce__(), inconsistency between pickle and copy'
updated_at = <Date 2022-01-10.22:33:13.389>
user = 'https://bugs.python.org/levbishop' bugs.python.org fields:
activity = <Date 2022-01-10.22:33:13.389>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2022-01-10.21:41:04.524>
creator = 'lev.bishop'
dependencies = []
files = ['50554']
hgrepos = []
issue_num = 46336
keywords = []
message_count = 2.0
messages = ['410256', '410262']
nosy_count = 5.0
nosy_names = ['rhettinger', 'pitrou', 'serhiy.storchaka', 'iritkatriel', 'lev.bishop']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue46336'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11'] Activitylevbishop commented on Jan 10, 2022 MannequinAuthor More actions As discussed in discord thread https://discuss.python.org/t/sixth-element-of-tuple-from-reduce-inconsistency-between-pickle-and-copy/12902 where guido suggested to open this issue.
Both the pickle and copy modules of the standard library make use of a class’s __reduce__() method for customizing their pickle/copy process. They seem to have a consistent view of the first 5 elements of the returned tuple:
(func, args, state, listiter, dictiter) but the 6th element seems different. For pickle it’s state_setter , a callable with signature state_setter(obj, state)->None , but for copy it’s deepcopy with signature deepcopy(arg: T, memo) -> T .
This seems to be unintentional, since the pickle documentation states:
As we shall see, pickle does not use directly the methods described
above. In fact, these methods are part of the copy protocol which
implements the __reduce__() special method. The copy protocol provides
a unified interface for retrieving the data necessary for pickling
and copying objects
It seems like in order to make a class definition for __reduce__() returning all 6 elements, then the __reduce__() would have to do something very awkward like examining its call stack in order to determine if it is being called in pickle or copy context in order to return an appropriate callable? (Naively providing the same callable in both contexts would cause errors for one or the other).
I attach a test file which defines two classes making use of a __reduce__() returning a 6 element tuple. One class Pickleable can be duplicated via pickling, but not deepcopied. The converse is true for the Copyable class.
Other than the 6th element of the tuple returned from __reduce__() the classes are identical.
Guido dug into the history and found that:
it looks like these are independent developments:
the 6th arg for deepcopy was added 6 years ago via bpo-26167: Improve copy.copy speed for built-in types (list/set/dict) - Python tracker
the 6th arg for pickle was adde 3 years ago via bpo-35900: Add pickler hook for the user to customize the serialization of user defined functions and types. - Python tracker
I’m guessing the folks doing the latter weren’t aware that deepcopy already uses the 6th arg. Sorting this out will be painful.
iritkatriel commented on Jan 10, 2022 I added Serhiy as the author of the deepcopy optimization. Although it was the first to use the 6th item, it is not documented so I wonder if it's the easier of the two to change. serhiy-storchaka commented on Jun 8, 2022 The code was written with expectation that __reduce__() returns 2 to 5 items. The deepcopy parameter was not designed to be overridden, it was a pure microoptimization (access to a global). The simplest correct solution is either to make it keyword-only, or remove it at all.
There is only one vague test for the 6th item of __reduce__(), so I hesitate to implement its support for copy. nneonneo commented on Sep 27, 2022 I just ran into this bug. Why can't we just add support for the sixth setstate arg by testing for it before if hasattr(y, '__setstate__'):? Something like this:
--- copy.py.old 2022-09-27 12:30:15.000000000 -0700
+++ copy.py.new 2022-09-27 12:30:44.000000000 -0700
@@ -258,7 +258,7 @@
def _reconstruct(x, memo, func, args,
state=None, listiter=None, dictiter=None,
- *, deepcopy=deepcopy):
+ setstate=None, *, deepcopy=deepcopy):
deep = memo is not None
if deep and args:
args = (deepcopy(arg, memo) for arg in args)
@@ -269,7 +269,9 @@
if state is not None:
if deep:
state = deepcopy(state, memo)
- if hasattr(y, '__setstate__'):
+ if setstate is not None:
+ setstate(y, state)
+ elif hasattr(y, '__setstate__'):
y.__setstate__(state)
else:
if isinstance(state, tuple) and len(state) == 2:I think this would be a cleaner fix; I doubt anyone is depending on passing a different deepcopy as the sixth argument because it would directly break pickling.
The test case I'm using:
import pickle
import copy
class X:
def setstate(self, state):
# This is passed {"x": 42} as expected with pickle,
# but is incorrectly passed the memo object with copy
print(self, state)
self.__dict__.update(state)
def __reduce__(self):
return (X, (), dict(x=42), iter([]), iter([]), X.setstate)
print("Pickle:")
print(pickle.loads(pickle.dumps(X())))
print("Deepcopy:")
print(copy.deepcopy(X()))serhiy-storchaka commented on Sep 28, 2022 It would be not safer to add the setstate in a bugfix release, because the code written with intention of using this feature would could work incorrectly (producing a wrong result rather than failing) in older releases. This should be considered as a new feature and only added in a new feature release (3.12). Please open a new issue for this.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|