Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix/deep copy recurse #29393

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
Loading
from

Conversation

tacaswell
Copy link
Member

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 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 #29157

Without these changes there are two classes of failures

  • infinite recursion with Paths
  • failing to be able to mutate copied Transforms

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

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 31, 2024
@tacaswell tacaswell added this to the v3.11.0 milestone Dec 31, 2024
@tacaswell tacaswell force-pushed the fix/deep_copy_recurse branch 2 times, most recently from ba83fb4 to 60903f0 Compare December 31, 2024 21:11
@tacaswell
Copy link
Member Author

Also, a point to stubtest for finding an unintended API change.

lib/matplotlib/path.py Outdated Show resolved Hide resolved
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 tacaswell force-pushed the fix/deep_copy_recurse branch from 8fedcea to 717ea24 Compare April 28, 2025 01:32
@ksunden
Copy link
Member

ksunden commented May 2, 2025

Given that this is a) in regards to a pre-release version of python and b) awaiting a decision from upstream, I'm not going to hold 3.10.2 up on this.

I consider this in the same bucket as #29816, as potentially deserving of a micro release upon merge, and certainly want it before 3.11.0, but not actually holding up micro releases.

That said, retaining the Release Critical label and will re-evaluate with each release.

@ksunden ksunden modified the milestones: v3.10.2, v3.10.3 May 2, 2025
@tacaswell tacaswell marked this pull request as ready for review May 8, 2025 18:19
@tacaswell tacaswell closed this May 8, 2025
@tacaswell tacaswell reopened this May 8, 2025
@tacaswell
Copy link
Member Author

power-cycled to get a new (synthetic) merge commit and re-start CI.

We should probably review this and get it in sooner rather than later. There has been no motion upstream, but at this point I would rather get a 3.10.3 out with the fix assuming upstream does not change and contemplate reverting if they do rather than do nothing and then have to scramble later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: path handling topic: transforms and scales
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FUTURE BUG: reconsider how we deep-copy path objects
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.