Skip to content
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

deepcopy doesn't copy instance methods #45856

Closed
mlvanbie mannequin opened this issue Nov 29, 2007 · 24 comments
Closed

deepcopy doesn't copy instance methods #45856

mlvanbie mannequin opened this issue Nov 29, 2007 · 24 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@mlvanbie
Copy link
Mannequin

mlvanbie mannequin commented Nov 29, 2007

BPO 1515
Nosy @gvanrossum, @warsaw, @pitrou, @tiran, @rbtcollins, @cool-RR
Files
  • issue1515.patch: Patch to add InstanceMethod copying.
  • issue1515.patch: Updated patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2009-11-28.15:59:33.453>
    created_at = <Date 2007-11-29.00:41:00.076>
    labels = ['type-bug', 'library']
    title = "deepcopy doesn't copy instance methods"
    updated_at = <Date 2009-11-28.15:59:33.451>
    user = 'https://bugs.python.org/mlvanbie'

    bugs.python.org fields:

    activity = <Date 2009-11-28.15:59:33.451>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2009-11-28.15:59:33.453>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2007-11-29.00:41:00.076>
    creator = 'mlvanbie'
    dependencies = []
    files = ['15405', '15406']
    hgrepos = []
    issue_num = 1515
    keywords = ['patch']
    message_count = 24.0
    messages = ['57923', '58839', '58851', '58854', '59508', '61893', '61900', '61901', '61909', '61921', '61922', '61938', '61941', '95716', '95760', '95761', '95766', '95767', '95776', '95777', '95778', '95779', '95790', '95791']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'barry', 'pitrou', 'christian.heimes', 'rbcollins', 'mlvanbie', 'Dmitrey', 'cool-RR']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue1515'
    versions = ['Python 2.7', 'Python 3.2']

    @mlvanbie
    Copy link
    Mannequin Author

    mlvanbie mannequin commented Nov 29, 2007

    Currently, using deepcopy on instance methods causes an exception to be
    thrown. This can be fixed by adding one line to copy.py:

    d[types.MethodType] = _deepcopy_atomic

    This will not make duplicate copies of mutable values referenced within
    the instance method (such as its associated instance), but it will be
    the same as the handling of other function types (which have the same
    problem).

    @mlvanbie mlvanbie mannequin added the type-bug An unexpected behavior, bug, or error label Nov 29, 2007
    @gvanrossum
    Copy link
    Member

    I disagree that this would be harmless; this was excluded intentionally.
    A (bound) method contains an instance which definitely represents state
    and calling the method can easily mutate that state.

    This is different from classes (which could contain state) or modules
    (which almost certainly contain state) or functions (which could mutate
    global state) -- in all those cases, we're talking about singleton
    state, for which it makes sense not to create a clone. But for methods,
    we're talking about instances, of which there could be many, and I
    believe those should be properly copied or refused, rather than creating
    accidental state sharing between the original and its deep copy.

    I suppose you have a specific use case in mind. Can't you solve that by
    adding a __deepcopy__ method to some object?

    @mlvanbie
    Copy link
    Mannequin Author

    mlvanbie mannequin commented Dec 20, 2007

    I am implementing a library that makes extensive use of delayed
    executions represented by functions. Copying objects both with and
    without shared state referenced by the functions is important. There is
    one entry point where I would expect functions to go. On the other
    hand, I can't prevent users from taking an instance method and wrapping
    it inside of a callable object that lacks an appropriate __deepcopy__
    method (this would be a sensible thing to do, in fact).

    I don't mind whether instance methods have shallow or deep copies as
    long as I can document the result. Neither invasive use of __deepcopy__
    nor throwing an exception is suitable for my use. For reference, the
    exception is

    TypeError: instancemethod expected at least 2 arguments, got 0

    Ideally, deepcopy() and pickle() would copy the internals of all
    function types and I could implement shared mutable state by using
    __deepcopy__ in a single class. I assume that this is unreasonable to
    implement.

    No matter the implementation, I think that there should be some way of
    running deepcopy() on all primitive types without throwing exceptions.
    I can see use cases where throwing exceptions for all types that can't
    be deeply copied would be useful and disabling __deepcopy__ might be
    important.

    @gvanrossum
    Copy link
    Member

    I'll take this off line.

    @mlvanbie
    Copy link
    Mannequin Author

    mlvanbie mannequin commented Jan 8, 2008

    Guido pointed out a common use case where people use bound methods in a
    way that would be poorly served by my change. An alternate
    implementation with a deeper copy will cause less surprise:

    def _deepcopy_method(x, memo):
        return type(x)(x.im_func, deepcopy(x.im_self, memo), x.im_class)
    d[types.MethodType] = _deepcopy_method

    The function and class are still shallow-copied but the bound object is
    deep-copied. I use type(x)() instead of types.MethodType() because
    types will be unbound when the function runs.

    @Dmitrey
    Copy link
    Mannequin

    Dmitrey mannequin commented Jan 31, 2008

    Hallo, I' developer of the OpenOpt, free Python-based numerical
    optimization framework.

    Please don't you mind me to increase Severity of the bug to major. This
    bug is driving me mad, as well as some of OO users, and makes OO usage
    much less convenient and obvious.

    http://openopt.blogspot.com/2008/01/about-prob-structure-redefinition.html

    @tiran
    Copy link
    Member

    tiran commented Jan 31, 2008

    I've read your blog. You don't have to modify the Python core:

    import copy
    import types
    
    def _deepcopy_method(x, memo):
        return type(x)(x.im_func, deepcopy(x.im_self, memo), x.im_class)
    copy._deepcopy_dispatch[types.MethodType] = _deepcopy_method

    I still wonder why you have to use deepcopy in your app.

    @tiran
    Copy link
    Member

    tiran commented Jan 31, 2008

    I'm bumping up the version number to 2.6. Python 2.5 is in maintenance
    mode and the behavior of deepcopy for instance methods won't be changed.

    @tiran tiran added the stdlib Python modules in the Lib dir label Jan 31, 2008
    @gvanrossum
    Copy link
    Member

    I'm fine with applying Michael's suggestion to 2.6.

    I're reverting the priority change though; using deepcopy is not OO and
    IMO usually indicates that there's something wrong with your app.

    @tiran
    Copy link
    Member

    tiran commented Jan 31, 2008

    Since Guido accepted the proposal can you please provide a patch with at
    least one unit test?

    @Dmitrey
    Copy link
    Mannequin

    Dmitrey mannequin commented Jan 31, 2008

    I don't know did you mean it to me (as I've noticed from address) or no.
    I can hardly provide any help for fixing the bug, I'm not skilled enough
    in Python core files. Here's results of the proposition mentioned:

    import copy
    import types
    
    def _deepcopy_method(x, memo):
        return type(x)(x.im_func, deepcopy(x.im_self, memo), x.im_class)
    copy._deepcopy_dispatch[types.MethodType] = _deepcopy_method
    
    from scikits.openopt import NLP
    p = NLP(lambda x: x**2, 1)
    p3 = _deepcopy_method(p, None)
    Traceback (most recent call last):
      File "test_deepcopy.py", line 10, in <module>
        p3 = _deepcopy_method(p, None)
      File "test_deepcopy.py", line 5, in _deepcopy_method
        return type(x)(x.im_func, deepcopy(x.im_self, memo), x.im_class)
    AttributeError: NLP instance has no attribute 'im_func'

    as for copy.py from Python core, it seems like the file has no
    "_deepcopy_method" function.

    Users want to run r = p.solve(solverName) for several different solvers and same p instance. So it needs each time using
    for sn in [solverName1, solverName2, ..., solverNameN]:
    p = NLP(dozens of params)
    p.somefiled.paramN+1 =valN+1
    p.somefiled.paramN+2 =valN+2
    ...
    r{i} = p.solve(sn)
    while in my previous MATLAB version of the toolbox it looks just like:

    (assign prob only once)
    p = ooAssign(params)
    for sn in [solverName1, solverName2, ..., solverNameN]
    r{i} = p.solve(sn)
    end

    Christian Heimes wrote:

    Christian Heimes added the comment:

    Since Guido accepted the proposal can you please provide a patch with at
    least one unit test?

    ----------
    resolution: -> accepted


    Tracker <report@bugs.python.org>
    <http://bugs.python.org/issue1515\>


    @mlvanbie
    Copy link
    Mannequin Author

    mlvanbie mannequin commented Jan 31, 2008

    Dmitrey: You can't call _deepcopy_method() on anything other than
    something with type types.MethodType. It is a function in a
    type-dispatch table, so it will always be called safely.
    copy._deepcopy_dispatch is that table; if you assign _deepcopy_method to
    copy._deepcopy_dispatch[types.MethodType] then copy.deepcopy() will
    understand what to do with method types. See my unit test below for the
    semantics.

    Christian Heimes, this is my unit test:

    # This test would fail due to an exception during deepcopy in version 2.5.1

    class C(object):
      def __init__(self, n):
        self.n = n
    
      def GetN(self):
        return self.n
    
    orig_obj = C(42)
    copy_list = copy.deepcopy([orig_obj, orig_obj.GetN])
    if copy_list[1]() != 42:
      print 'Error: Instance method lost object?'
    orig_obj.n = 43
    if copy_list[1]() != 42:
      print 'Error: Instance method assoc with orig object'
    copy_list[0].n = 44
    if copy_list[1]() != 44:
      print 'Error: Instance method should assoc with new object copy'
    if not type(copy_list[1]) is type(orig_obj.GetN):
      print 'Error: Deepcopy changed type of instance method'

    Why do people want to use copy and deepcopy? I think that the issue is
    that Python is an imperative language that passes and copies references.
    If a library function doesn't treat its arguments as const, horrible
    things could happen. Compounding this, several standard operations
    (such as sort) operate in place, so if you want to use them then you
    need to make shallow copies at the very least. When non-scalar types
    nest, the whole structure can be deepcopied or copy can be used
    repeatedly in a selective manner (with a high chance of bugs). Perl
    deals with this issue by using standard copy semantics (but
    call-by-reference) and a copy-on-write implementation. Most operations
    operate out-of-place. Pure languages can use reference-based copy
    semantics because you can't modify anything, forcing users to create the
    copy-on-write implementation when mutating structures.

    @gvanrossum
    Copy link
    Member

    On Jan 31, 2008 2:54 PM, Michael Van Biesbrouck wrote:

    Why do people want to use copy and deepcopy? I think that the issue is
    that Python is an imperative language that passes and copies references.
    If a library function doesn't treat its arguments as const, horrible
    things could happen. Compounding this, several standard operations
    (such as sort) operate in place, so if you want to use them then you
    need to make shallow copies at the very least.

    I see the point for shallow copies. I just don't see the point for deep copies.

    When non-scalar types
    nest, the whole structure can be deepcopied or copy can be used
    repeatedly in a selective manner (with a high chance of bugs).

    I don't understand the use case. I still think this comes primarily
    from a lack of familiarity with how Python is typically used.

    @cool-RR
    Copy link
    Mannequin

    cool-RR mannequin commented Nov 25, 2009

    (I see some time has passed since the last message. I'm assuming the
    issue wasn't fixed, correct me if I'm wrong.)

    Here's a use case for using deepcopy: I'm developing a simulations
    framework called GarlicSim, all Python. You can see a short video here:
    http://garlicsim.org/brief_introduction.html
    The program handles world states in simulated worlds. Some simpacks
    deepcopy the existing world state to generate the next world state.

    This bug is currently preventing me from writing simpacks whose world
    states reference instance methods, which is a severe limitation for me.

    I think this issue should be bumped in priority. Also, in the mean time,
    a more informative error message should be given (Like "deepcopy can't
    handle instance methods."), instead of the current cryptic one.

    @rbtcollins
    Copy link
    Member

    Ran into this trying to do some test isolation stuff.

    Notwithstanding the questions about 'why', this is a clear limitation
    hat can be solved quite simply - is there any harm that will occur if we
    fix it?

    I've attached a patch, with a test (in the style of the current tests)
    which shows the copy works correctly.

    @rbtcollins
    Copy link
    Member

    This affects 2.7 too.

    @warsaw
    Copy link
    Member

    warsaw commented Nov 27, 2009

    Robert's patch looks fine to me. My concern is changing this in a point
    release (e.g. 2.6.5). I know Guido said he was fine for this going into
    2.6 but that was in January 08, before 2.6 final was released in October
    08. At this point, the question is whether this is safe to change in a
    patch release or whether this needs to go in 2.7.

    Probably a question for python-dev.

    @pitrou
    Copy link
    Member

    pitrou commented Nov 27, 2009

    The test should be a real unittest in Lib/test/test_copy, not something
    in the __main__ section of Lib/copy.py. Nobody runs these, as a matter
    of fact if you run Lib/copy.py under the py3k branch it fails.

    To nitpick a bit, I also think Michael's test above was better, since it
    was checking whether the copied method actually worked.

    @rbtcollins
    Copy link
    Member

    @antoine, I agree that the tests for copy should be a proper unit test;
    that seems orthogonal to this patch though :)

    I don't have a checkout of 3 at the moment, but do you think the test
    failure on 3 is shallow or deep?

    @pitrou
    Copy link
    Member

    pitrou commented Nov 27, 2009

    @antoine, I agree that the tests for copy should be a proper unit test;
    that seems orthogonal to this patch though :)

    Not really, since Lib/test/test_copy.py exists and contains tests for
    deepcopy; you should add the new test there.

    I don't have a checkout of 3 at the moment, but do you think the test
    failure on 3 is shallow or deep?

    What I mean is that Lib/copy.py fails without your changes under py3k.

    @rbtcollins
    Copy link
    Member

    Oh man, I looked for a regular unit test - sorry that I missed it. Bah.

    I've added a call to the method and moved it into test_copy.

    @gvanrossum
    Copy link
    Member

    Smells like a feature to me.

    "Bug" == "coding error"

    "Feature" == "change in (documented or intended) behavior"

    I'm fine with this going into 2.7 / 3.2.

    @warsaw
    Copy link
    Member

    warsaw commented Nov 28, 2009

    Guido - agreed! Versions updated.

    @pitrou
    Copy link
    Member

    pitrou commented Nov 28, 2009

    The patch has been committed in r76571 (trunk) and r76572 (py3k). Thank you!

    @pitrou pitrou closed this as completed Nov 28, 2009
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants