Fix/deep copy recurse #29393
Draft
Fix/deep copy recurse #29393
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tacaswell
added
the
Release critical
tacaswell
force-pushed
the
fix/deep_copy_recurse
branch
from
464b72e to
ba83fb4
Compare
We provide deepcopy and copy methods as a shortcut for users to avoid having to import the copy module and call the required function. Directly calling `__deepcopy__` side-steps all of the memoization machinery and may lead to incorrect results.
See python/cpython#126817 for upstream discussion. This works around the change by using (private) methods from the copy module to re-implement the path though copy/deepcopy that we would like to use but avoid the special-casing for `super()` objects that is breaking us. We could vendor the current versions of `_keep_alive` (weakref work to manage lifecycles) and `_reconstruct` (where the recursion happens) to superficially avoid using private functions from CPython. However, if these functions do change significantly I worry that our copies would not inter-operate anyway. Closes matplotlib#29157
tacaswell
force-pushed
the
fix/deep_copy_recurse
branch
from
ba83fb4 to
60903f0
Compare
|
Also, a point to stubtest for finding an unintended API change. |
highvelcty
reviewed
Jan 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR summary
See python/cpython#126817 for upstream discussion.
This works around the change by using (private) methods from the copy module to
re-implement the path though copy/deepcopy that we would like to use but avoid
the special-casing for
super()objects that is breaking us.We could vendor the current versions of
_keep_alive(weakref work to managelifecycles) and
_reconstruct(where the recursion happens) to superficiallyavoid using private functions from CPython. However, if these functions do
change significantly I worry that our copies would not inter-operate anyway.
Closes #29157
Without these changes there are two classes of failures
The tests (mostly) pass [^] for me locally with a py314 build with this branch (and you have to be on 3.14 to hit the new code!)
I think we should hold of merging this until closer to py314 being release (so some time over the summer) to see if upstream will change their mind about the change that is affecting us, but if we do need to merge this we should backport it has as we can as this is a show-stopping issue (infinite loop on render).
The new code should work on all our supported versions of Python, but at least for now my view is we should version gate the fishy stuff (using private methods) to only the versions of Python where we have to.
I have no idea if this is going to work as expected on pypy.
PR checklist