Skip to content

maximum recursion depth in deepcopy regression #126817

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.

Sign up for GitHub

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

Jump to bottom
Open
tacaswell opened this issue Nov 14, 2024 · 7 comments
Open

maximum recursion depth in deepcopy regression #126817

tacaswell opened this issue Nov 14, 2024 · 7 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@tacaswell
Copy link
Contributor

tacaswell commented Nov 14, 2024 •
edited
Loading

Bug report

Bug description:

The following code works on py313 and below (this is a very cut-down version of some code in Matplotilb (matplotlib.path.Path)) but hit the maximum recursion depth on main.

# Add a code block here, if required
import copy

class Test:
    def __init__(self, a, b):
        self.a = a
        self.b = a
        self._state = True

    def __deepcopy__(self, memo=None):
        p = copy.deepcopy(super(), memo)
        p._state = False
        return p

t = Test(1, 2)
t2 = copy.deepcopy(t)

I bisected this to #125781 and suspect the problem is that because it is returning a new object it is escaping the memoization and looping.

I'm happy to be told we should not be doing this fix it on the Matplotlib side, but I would like to be sure that this was an expected consequence of the change.

attn @serhiy-storchaka

CPython versions tested on:

3.14, CPython main branch

Operating systems tested on:

Linux
@tacaswell tacaswell added the type-bug An unexpected behavior, bug, or error label Nov 14, 2024
@picnixz picnixz added the stdlib Python modules in the Lib dir label Nov 14, 2024
@serhiy-storchaka
Copy link
Member

Interesting. I did not foresee such use case.

In general, such code worked by accident, because the code in the copy module is more permissive than in the pickle module. Although it does not work if the reducing function returns a string (enums or other singletons). It does not work if copying in the parent class was implemented by the __copy__ and __deepcopy__ methods.

The question is how to support that use case -- copying an object ignoring the special methods in the current class. Should we use super() for this? How to resolve conflict with different semantic for pickling? 🤔

@tacaswell tacaswell mentioned this issue Nov 19, 2024
Open
@tacaswell
Copy link
Contributor Author

sounds like we should find another way downstream no matter which way CPython goes on this question?

@tacaswell tacaswell mentioned this issue Nov 25, 2024
Closed
@github-actions github-actions bot mentioned this issue Dec 1, 2024
Closed
@tacaswell
Copy link
Contributor Author

Another example from Matplotlib that I suspect is coming from the same cause (but have not actually bisected) is

https://github.com/matplotlib/matplotlib/blob/f2717a5486f782baca9fedd5fdb28f73e6c366d0/lib/matplotlib/transforms.py#L143-L153

    def __copy__(self):
        other = copy.copy(super())
        # If `c = a + b; a1 = copy(a)`, then modifications to `a1` do not
        # propagate back to `c`, i.e. we need to clear the parents of `a1`.
        other._parents = {}
        # If `c = a + b; c1 = copy(c)`, then modifications to `a` also need to
        # be propagated to `c1`.
        for key, val in vars(self).items():
            if isinstance(val, TransformNode) and id(self) in val._parents:
                other.set_children(val)  # val == getattr(other, key)
        return other

That is now failing with

E       AttributeError: 'super' object has no attribute '_parents' and no __dict__ for setting new attributes

It seems right to be to be able to access "copy/deepcopy this as if the object were its parent class" via copy.deepcopy(super()) because for all other methods that is what super().foo() does. A lazy (but apparently wrong) way to think about copy/deepcopy is "please invoke __copy__ / __deepcopy__ for me".

I'm indifferent on if pickling super objects should work or not (I think a compelling case could be made for pickle simply refusing to tray), but feel pretty strongly that copy.deepcopy(super()) when used in __deepcopy__ should return a copy of the calling object as if it were the next class in the mro. Otherwise it is not clear how we should be extending a base classes copying implementation.

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Dec 31, 2024
@tacaswell
FIX: account for changes to cpython with copy/deepcopy to super()
464b72e 
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 mentioned this issue Dec 31, 2024
Open
2 tasks
@tacaswell
Copy link
Contributor Author

matplotlib/matplotlib#29393 is a hacky work-around to this change.

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Dec 31, 2024
@tacaswell
FIX: account for changes to cpython with copy/deepcopy to super()
ba83fb4 
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 added a commit to tacaswell/matplotlib that referenced this issue Dec 31, 2024
@tacaswell
FIX: account for changes to cpython with copy/deepcopy to super()
60903f0 
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
@asmacdo asmacdo mentioned this issue Jan 14, 2025
Draft
@tacaswell
Copy link
Contributor Author

@serhiy-storchaka Can this be labeled so it is resolved one way or the other by 3.14.0b1?

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Apr 27, 2025
@tacaswell
FIX: account for changes to cpython with copy/deepcopy to super()
a9f8dae 
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
Copy link
Contributor Author

@serhiy-storchaka At this point should I assume that this will be the state in 3.14 and start to address the change downstream?

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Apr 28, 2025
@tacaswell
FIX: account for changes to cpython with copy/deepcopy to super()
42874f9 
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
@mdboom
Copy link
Contributor

mdboom commented Apr 28, 2025

We should probably determine whether we can/should fix or add a note to WHATSNEW.

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Jun 12, 2025
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Pickle and copy issues 🥒
Status: No status
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mdboom @tacaswell @serhiy-storchaka @picnixz