GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix/deep copy recurse by tacaswell · Pull Request #29393 · matplotlib/matplotlib

Conversation

@tacaswell

@tacaswell tacaswell commented Dec 31, 2024

Copy link
Copy Markdown
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

  • "closes #0000" is in the body of the PR description to link the related issue
  • new and changed code is tested
  • [/] Plotting related features are demonstrated in an example
  • [/] New Features and API Changes are noted with a directive and release note
  • [/] Documentation complies with general and docstring guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@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
@github-actions github-actions Bot added topic: path handling topic: transforms and scales labels Dec 31, 2024
@tacaswell tacaswell mentioned this pull request Dec 31, 2024
maximum recursion depth in deepcopy regression python/cpython#126817
Closed
@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

tacaswell commented Dec 31, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

highvelcty
highvelcty reviewed Jan 7, 2025
View reviewed changes
Comment thread lib/matplotlib/path.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@asmacdo asmacdo mentioned this pull request Jan 14, 2025
Test against pre-release of 3.14 con/duct#230
Merged
@befeleme befeleme mentioned this pull request Apr 22, 2025
[MNT]: Python 3.14.0a7 test failures #29959
Closed
@tacaswell
tacaswell force-pushed the fix/deep_copy_recurse branch from dd33ebc to 8fedcea Compare April 27, 2025 19:58
@tacaswell tacaswell modified the milestones: v3.11.0, v3.10.2 Apr 27, 2025
@tacaswell tacaswell mentioned this pull request Apr 28, 2025
TST: Make refcount tests more resilient to Python changes #29970
Merged
1 of 2 tasks
@tacaswell
tacaswell force-pushed the fix/deep_copy_recurse branch from 8fedcea to 717ea24 Compare April 28, 2025 01:32
@ksunden

ksunden commented May 2, 2025

Copy link
Copy Markdown
Member

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.

tacaswell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

tacaswell commented May 8, 2025

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ksunden
ksunden reviewed Jun 12, 2025
View reviewed changes
Comment thread lib/matplotlib/path.py Outdated
if sys.version_info >= (3, 14):
from copy import _reconstruct, _keep_alive
rv = super().__reduce_ex__(4)
assert memo is not None

@ksunden ksunden Jun 12, 2025

Copy link
Copy Markdown
Member

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.

Assert may not be necessary anymore

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
tacaswell added 2 commits June 12, 2025 16:42
@tacaswell
MNT: use copy.deepcopy to implement deepcopy method
529b3e5
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.
@tacaswell
FIX: account for changes to cpython with copy/deepcopy to super()
e9c66d9
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 717ea24 to e9c66d9 Compare June 12, 2025 20:42
ksunden
ksunden approved these changes Jun 19, 2025
View reviewed changes

@ksunden ksunden left a comment

Copy link
Copy Markdown
Member

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.

Obviously some reservations in regards to using private methods from Python, but appears to be necessary. And would like to move forward with 3.14 support.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tacaswell

tacaswell commented Jun 19, 2025

Copy link
Copy Markdown
Member Author

We know Fedora has rebuilt for 3.14 with this patch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tacaswell tacaswell mentioned this pull request Jun 20, 2025
ci: Enable wheel builds on Python 3.14 #30195
Merged
1 task done
@jkseppan

jkseppan commented Jun 21, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

#30198 is an alternative fix without using private methods.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tacaswell

tacaswell commented Jun 26, 2025

Copy link
Copy Markdown
Member Author

Closed in favor of #30198

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tacaswell tacaswell closed this Jun 26, 2025
@tacaswell
tacaswell deleted the fix/deep_copy_recurse branch June 26, 2025 02:42
@QuLogic QuLogic added the status: superseded label Jun 26, 2025
@QuLogic QuLogic removed this from the v3.10.4 milestone Jun 26, 2025
@tacaswell
tacaswell restored the fix/deep_copy_recurse branch June 26, 2025 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ksunden ksunden ksunden approved these changes
+1 more reviewer
@highvelcty highvelcty highvelcty left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: superseded topic: path handling topic: transforms and scales

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FUTURE BUG: reconsider how we deep-copy path objects

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@tacaswell @ksunden @jkseppan @highvelcty @QuLogic
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.