FUTURE BUG: reconsider how we deep-copy path objects

@anntzer

We currently use super() in the __deepcopy__ implementation ofPath

Lines 279 to 287 in 183b04f

    
               def __deepcopy__(self, memo=None): 
        
                   """ 
        
                   Return a deepcopy of the `Path`.  The `Path` will not be 
        
                   readonly, even if the source `Path` is. 
        
                   """ 
        
                   # Deepcopying arrays (vertices, codes) strips the writeable=False flag. 
        
                   p = copy.deepcopy(super(), memo) 
        
                   p._readonly = False 
        
                   return p

however, on the main branch of CPython this causes infinite recursion (python/cpython#126817). Although upstream may (or may not) sort out how to fix this, we should see if there is another way we can do what we need to do and avoid any fallout from changes in CPython.

Good first issue because "make sure deepcopy works on Paths" is a narrowly limited task, but hard because it will require understanding why we currently use super() and enough of the deepcopy/pickle protocol details to fix it.

attn @anntzer

Good first issue - notes for new contributors

This issue is suited to new contributors because it does not require understanding of the
Matplotlib internals. To get started, please see our contributing
guide.

We do not assign issues. Check the Development section in the sidebar for linked pull
requests (PRs). If there are none, feel free to start working on it. If there is an open PR, please
collaborate on the work by reviewing it rather than duplicating it in a competing PR.

If something is unclear, please reach out on any of our communication
channels.

Hi! I'm relatively a beginner but I'd love to take this on as a challenge if it's okay 😊

I was wondering, would it be acceptable to replace

p = copy.deepcopy(super(), memo)

with

p = self.__class__.__new__(self.__class__)

and to manually copy the attributes like

p._vertices = copy.deepcopy(self._vertices, memo)
p._codes = copy.deepcopy(self._codes, memo)
p._interpolation_steps = self._interpolation_steps
p._readonly = False

?

The end result would be like

 def __deepcopy__(self, memo=None):
        """
        Return a deepcopy of the `Path`.  The `Path` will not be
        readonly, even if the source `Path` is.
        """

        if id(self) in memo:
            return memo[id(self)]

        p = self.__class__.__new__(self.__class__)
        memo[id(self)] = p

        p._vertices = copy.deepcopy(self._vertices, memo)
        p._codes = copy.deepcopy(self._codes, memo)
        p._interpolation_steps = self._interpolation_steps
        p._readonly = False

        return p

@tacaswell

@tacaswell
I want to ask why we are using super() here if there is no parent class for the path.
This can be another possibility, like code calling the super() again and again, i.e. calling the method that we have in the path.
Instead of using:

 cls = self.__class__
    new_instance = cls.__new__(cls)
    
    # Add the new instance to the memo dictionary to handle circular references
    if memo is None:
        memo = {}
    memo[id(self)] = new_instance

    # Deepcopy the attributes explicitly
    new_instance.vertices = copy.deepcopy(self.vertices, memo)
    new_instance.codes = copy.deepcopy(self.codes, memo)
    new_instance._readonly = False

    return new_instance

This is because in the original code, super() begins searching for the method in the method resolution order (MRO), where the current class itself is considered first since there is no superclass. This results in an infinite recursion since copy.deepcopy finds the __deepcopy__ method in the current class, which then calls super() again, leading to a loop.

By explicitly utilizing the memo dictionary and checking if the current object is already present in it, we ensure that the __deepcopy__ process avoids re-processing the current object. This effectively prevents recursion by leveraging the memoization mechanism, which keeps track of objects that have already been deep-copied.

@saikarna913

@saikarna913 Are you working with @kyracho ? I am a bit confused by you posting an almost identical comment.

Reaching for __new__ is probably not the right answer, can either of you elaborate on why you think this is the right path?

It is probably also worth reading #21280 for the context of why we are using super() (along with the bug it fixed and the alternative PR we declined).

Without looking at the proper fix, it feels like we could/should get rid of the TextPath class and just have a Text(...).to_path() -> Path method?

TextPath seems to have the ability to change the font size (but not the string?) so a one-off converter may not be entirely sufficient.

I apologize for my confusion. I'm trying to clarify something. Since there isn't a deepcopy method in Textpath, it inherits from its parent class(PATH), which calls super() in the deepcopy method. Currently, the method that invoked super() is Textpath, so it will follow the method resolution order (MRO) of Textpath. In this MRO, the parent class is PATH, meaning that super() effectively returns a copy of path to deepcopy. However, this doesn't seem to be creating a true deepcopy of Textpath. Could you help me understand how this code functions? Thank you!

why can't we explicitly define deepcopy for subclasses like Textpath?

TextPath seems to have the ability to change the font size (but not the string?) so a one-off converter may not be entirely sufficient.

It seems simple enough to do textobj.set_size(new_size); path = textobj.to_path(), or, if one is really worried about efficiency, one can also just linearly scale the vertices (and translate them wherever needed) themselves.

However, this doesn't seem to be creating a true deepcopy of Textpath.

What do you mean by this?

why can't we explicitly define deepcopy for subclasses like Textpath?

There is no technical reason we can not, but the behavior we want is "do the normal deepcopy thing, but set writable" which is a behavior inherited from Path so we should not have to copy that logic into every sub-class (of which we apparently only have one).

@tacaswell

@tacaswell
I need small help
i am unable to find the _path which is giving me an import error in the path.py file

and regarding the deepcopy I think this works

in path.py file

def __deepcopy__(self, memo):
    # Create a shallow copy of the object first.
    new_instance = copy.copy(self)
    
    # Deep copy the _vertices array and set the writeable flag based on the readonly flag
    new_instance._vertices = np.copy(self._vertices)
    new_instance._vertices.flags.writeable = not self._readonly
    
    # Deep copy the _codes array if it exists, and handle the writeable flag
    if self._codes is not None:
        new_instance._codes = np.copy(self._codes)
        new_instance._codes.flags.writeable = not self._readonly
    
    # Deep copy any additional attributes that were added in the class
    for key, value in self.__dict__.items():
        # Skip internal attributes (e.g., _vertices, _codes, _readonly)
        if key in ['_vertices', '_codes', '_readonly']:
            continue
        
        # If the attribute is a mutable object, perform deep copy, otherwise just copy it
        if isinstance(value, (np.ndarray, list, dict)):
            new_instance.__dict__[key] = copy.deepcopy(value, memo)
        else:
            new_instance.__dict__[key] = value

    # Retain the readonly flag
    new_instance._readonly = self._readonly
    
    return new_instance

in textpath.py

  def __deepcopy__(self, memo):
        # Create a shallow copy of the TextPath object
        new_instance = super().__deepcopy__(memo)
        
        # Deep copy text-specific attributes
        new_instance._xy = self._xy
        new_instance._size = self._size
        new_instance._should_simplify = self._should_simplify

        # If cached vertices exist, we need to copy them and ensure the writeable flag
        if self._cached_vertices is not None:
            new_instance._cached_vertices = np.copy(self._cached_vertices)
            new_instance._cached_vertices.flags.writeable = not self._readonly
        
        # Mark the object as invalid since it's a deep copy
        new_instance._invalid = True

        return new_instance

_path is a cextension (see src/_path.h and src/_path_wrapper.cpp).

I'm sure with a bit of refinement those methods would work, but I am not sure that mostly re-implementing copy.deepcopy in __deepcopy__ of two of our classes is the correct approach. On one hand it will "work" in that for at least the classes as they are now for the tests we currently have it will pass, but it leaves open a path for getting out-of-sync with upstream in the future and has a good chance of confusing a future maintainer. Before we go that route we should fully rule out using the more "standard" path.

As an implementation detail, can you explain why you went with obj.__dict__[key] = value rather than setattr(obj, key, value)? I am also curious why you copy the cache in both implementations but only bother to invalidate it in textpath?

FIX: account for changes to cpython with copy/deepcopy to super()

Implement Path.__deepcopy__ avoiding infinite recursion

ci: stop testing for 3.14 until 3.10 is fully EOL

Navigation Menu

Repository navigation

FUTURE BUG: reconsider how we deep-copy path objects #29157

Good first issue - notes for new contributors

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	def __deepcopy__(self, memo=None):
	"""
	Return a deepcopy of the `Path`. The `Path` will not be
	readonly, even if the source `Path` is.
	"""
	# Deepcopying arrays (vertices, codes) strips the writeable=False flag.
	p = copy.deepcopy(super(), memo)
	p._readonly = False
	return p

FUTURE BUG: reconsider how we deep-copy path objects #29157

Description

Activity

github-actions commented on Nov 19, 2024

Good first issue - notes for new contributors

kc9323 commented on Nov 20, 2024

saikarna913 commented on Nov 20, 2024

tacaswell commented on Nov 20, 2024

anntzer commented on Nov 20, 2024

tacaswell commented on Nov 20, 2024

saikarna913 commented on Nov 21, 2024

anntzer commented on Nov 21, 2024

tacaswell commented on Nov 21, 2024

saikarna913 commented on Nov 22, 2024

tacaswell commented on Nov 22, 2024

7 remaining items

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions