classification
Title: deepcopy doesn't copy instance methods
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Dmitrey, barry, christian.heimes, cool-RR, gvanrossum, mlvanbie, pitrou, rbcollins
Priority: normal Keywords: patch

Created on 2007-11-29 00:41 by mlvanbie, last changed 2009-11-28 15:59 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue1515.patch rbcollins, 2009-11-27 09:54 Patch to add InstanceMethod copying.
issue1515.patch rbcollins, 2009-11-27 23:05 Updated patch
Messages (24)
msg57923 - (view) Author: Michael Van Biesbrouck (mlvanbie) Date: 2007-11-29 00:40
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).
msg58839 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-19 23:03
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?
msg58851 - (view) Author: Michael Van Biesbrouck (mlvanbie) Date: 2007-12-20 00:47
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.
msg58854 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-20 04:28
I'll take this off line.
msg59508 - (view) Author: Michael Van Biesbrouck (mlvanbie) Date: 2008-01-08 00:11
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.
msg61893 - (view) Author: Dmitrey (Dmitrey) Date: 2008-01-31 10:05
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
msg61900 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-31 14:18
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.
msg61901 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-31 14:22
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.
msg61909 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-31 17:15
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.
msg61921 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-31 18:11
Since Guido accepted the proposal can you please provide a patch with at
least one unit test?
msg61922 - (view) Author: Dmitrey (Dmitrey) Date: 2008-01-31 18:38
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>
> __________________________________
>
>
>
>
msg61938 - (view) Author: Michael Van Biesbrouck (mlvanbie) Date: 2008-01-31 22:54
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.
msg61941 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-31 23:57
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.
msg95716 - (view) Author: Ram Rachum (cool-RR) * Date: 2009-11-25 12:12
(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.
msg95760 - (view) Author: Robert Collins (rbcollins) * Date: 2009-11-27 09:54
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.
msg95761 - (view) Author: Robert Collins (rbcollins) * Date: 2009-11-27 09:55
This affects 2.7 too.
msg95766 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-11-27 13:39
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.
msg95767 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-27 13:47
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.
msg95776 - (view) Author: Robert Collins (rbcollins) * Date: 2009-11-27 22:25
@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?
msg95777 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-27 22:40
> @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.
msg95778 - (view) Author: Robert Collins (rbcollins) * Date: 2009-11-27 23:05
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.
msg95779 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2009-11-28 02:01
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.
msg95790 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-11-28 15:45
Guido - agreed!  Versions updated.
msg95791 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-28 15:59
The patch has been committed in r76571 (trunk) and r76572 (py3k). Thank you!
History
Date User Action Args
2009-11-28 15:59:33pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg95791

stage: resolved
2009-11-28 15:45:30barrysetmessages: + msg95790
versions: + Python 3.2, - Python 2.6
2009-11-28 02:01:57gvanrossumsetmessages: + msg95779
2009-11-27 23:05:08rbcollinssetfiles: + issue1515.patch

messages: + msg95778
2009-11-27 22:40:36pitrousetmessages: + msg95777
2009-11-27 22:25:06rbcollinssetmessages: + msg95776
2009-11-27 13:47:31pitrousetnosy: + pitrou
messages: + msg95767
2009-11-27 13:39:01barrysetnosy: + barry
messages: + msg95766
2009-11-27 09:55:22rbcollinssetmessages: + msg95761
versions: + Python 2.7
2009-11-27 09:54:47rbcollinssetfiles: + issue1515.patch

nosy: + rbcollins
messages: + msg95760

keywords: + patch
2009-11-25 12:12:45cool-RRsetnosy: + cool-RR
messages: + msg95716
2008-01-31 23:57:39gvanrossumsetmessages: + msg61941
2008-01-31 22:54:03mlvanbiesetmessages: + msg61938
2008-01-31 18:38:58Dmitreysetmessages: + msg61922
2008-01-31 18:11:26christian.heimessetresolution: accepted
messages: + msg61921
2008-01-31 17:15:58gvanrossumsetmessages: + msg61909
severity: major -> normal
2008-01-31 14:22:43christian.heimessetpriority: normal
messages: + msg61901
components: + Library (Lib)
versions: + Python 2.6, - Python 2.5
2008-01-31 14:18:22christian.heimessetnosy: + christian.heimes
messages: + msg61900
2008-01-31 10:05:13Dmitreysetnosy: + Dmitrey
messages: + msg61893
severity: normal -> major
2008-01-08 00:11:29mlvanbiesetmessages: + msg59508
2007-12-20 04:28:36gvanrossumsetmessages: + msg58854
2007-12-20 00:47:18mlvanbiesetmessages: + msg58851
2007-12-19 23:03:17gvanrossumsetnosy: + gvanrossum
messages: + msg58839
2007-11-29 00:41:00mlvanbiecreate