
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2007-11-29 00:41 bymlvanbie, last changed2022-04-11 14:56 byadmin. This issue is nowclosed.
| 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 bethrown. This can be fixed by adding one line to copy.py:d[types.MethodType] = _deepcopy_atomicThis will not make duplicate copies of mutable values referenced withinthe instance method (such as its associated instance), but it will bethe same as the handling of other function types (which have the sameproblem). | |||
| msg58839 -(view) | Author: Guido van Rossum (gvanrossum)*![]() | 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 stateand 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 mutateglobal state) -- in all those cases, we're talking about singletonstate, 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 Ibelieve those should be properly copied or refused, rather than creatingaccidental state sharing between the original and its deep copy.I suppose you have a specific use case in mind. Can't you solve that byadding 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 delayedexecutions represented by functions. Copying objects both with andwithout shared state referenced by the functions is important. There isone entry point where I would expect functions to go. On the otherhand, I can't prevent users from taking an instance method and wrappingit 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 aslong as I can document the result. Neither invasive use of __deepcopy__nor throwing an exception is suitable for my use. For reference, theexception isTypeError: instancemethod expected at least 2 arguments, got 0Ideally, deepcopy() and pickle() would copy the internals of allfunction types and I could implement shared mutable state by using__deepcopy__ in a single class. I assume that this is unreasonable toimplement.No matter the implementation, I think that there should be some way ofrunning deepcopy() on all primitive types without throwing exceptions. I can see use cases where throwing exceptions for all types that can'tbe deeply copied would be useful and disabling __deepcopy__ might beimportant. | |||
| msg58854 -(view) | Author: Guido van Rossum (gvanrossum)*![]() | 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 away that would be poorly served by my change. An alternateimplementation 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_methodThe function and class are still shallow-copied but the bound object isdeep-copied. I use type(x)() instead of types.MethodType() becausetypes 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 numericaloptimization framework. Please don't you mind me to increase Severity of the bug to major. Thisbug is driving me mad, as well as some of OO users, and makes OO usagemuch less convenient and obvious.http://openopt.blogspot.com/2008/01/about-prob-structure-redefinition.html | |||
| msg61900 -(view) | Author: Christian Heimes (christian.heimes)*![]() | Date: 2008-01-31 14:18 | |
I've read your blog. You don't have to modify the Python core:import copyimport typesdef _deepcopy_method(x, memo): return type(x)(x.im_func, deepcopy(x.im_self, memo), x.im_class)copy._deepcopy_dispatch[types.MethodType] = _deepcopy_methodI still wonder why you have to use deepcopy in your app. | |||
| msg61901 -(view) | Author: Christian Heimes (christian.heimes)*![]() | Date: 2008-01-31 14:22 | |
I'm bumping up the version number to 2.6. Python 2.5 is in maintenancemode and the behavior of deepcopy for instance methods won't be changed. | |||
| msg61909 -(view) | Author: Guido van Rossum (gvanrossum)*![]() | 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 andIMO usually indicates that there's something wrong with your app. | |||
| msg61921 -(view) | Author: Christian Heimes (christian.heimes)*![]() | Date: 2008-01-31 18:11 | |
Since Guido accepted the proposal can you please provide a patch with atleast 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 copyimport typesdef _deepcopy_method(x, memo): return type(x)(x.im_func, deepcopy(x.im_self, memo), x.im_class)copy._deepcopy_dispatch[types.MethodType] = _deepcopy_methodfrom scikits.openopt import NLPp = 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 usingfor 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 thansomething with type types.MethodType. It is a function in atype-dispatch table, so it will always be called safely. copy._deepcopy_dispatch is that table; if you assign _deepcopy_method tocopy._deepcopy_dispatch[types.MethodType] then copy.deepcopy() willunderstand what to do with method types. See my unit test below for thesemantics.Christian Heimes, this is my unit test:# This test would fail due to an exception during deepcopy in version 2.5.1class C(object): def __init__(self, n): self.n = n def GetN(self): return self.norig_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 = 43if copy_list[1]() != 42: print 'Error: Instance method assoc with orig object'copy_list[0].n = 44if 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 isthat Python is an imperative language that passes and copies references. If a library function doesn't treat its arguments as const, horriblethings could happen. Compounding this, several standard operations(such as sort) operate in place, so if you want to use them then youneed to make shallow copies at the very least. When non-scalar typesnest, the whole structure can be deepcopied or copy can be usedrepeatedly in a selective manner (with a high chance of bugs). Perldeals with this issue by using standard copy semantics (butcall-by-reference) and a copy-on-write implementation. Most operationsoperate out-of-place. Pure languages can use reference-based copysemantics because you can't modify anything, forcing users to create thecopy-on-write implementation when mutating structures. | |||
| msg61941 -(view) | Author: Guido van Rossum (gvanrossum)*![]() | 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 primarilyfrom 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.htmlThe 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 limitationhat can be solved quite simply - is there any harm that will occur if wefix 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)*![]() | Date: 2009-11-27 13:39 | |
Robert's patch looks fine to me. My concern is changing this in a pointrelease (e.g. 2.6.5). I know Guido said he was fine for this going into2.6 but that was in January 08, before 2.6 final was released in October08. At this point, the question is whether this is safe to change in apatch release or whether this needs to go in 2.7.Probably a question for python-dev. | |||
| msg95767 -(view) | Author: Antoine Pitrou (pitrou)*![]() | Date: 2009-11-27 13:47 | |
The test should be a real unittest inLib/test/test_copy, not somethingin the __main__ section ofLib/copy.py. Nobody runs these, as a matterof fact if you runLib/copy.py under the py3k branch it fails.To nitpick a bit, I also think Michael's test above was better, since itwas 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 testfailure on 3 is shallow or deep? | |||
| msg95777 -(view) | Author: Antoine Pitrou (pitrou)*![]() | 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, sinceLib/test/test_copy.py exists and contains tests fordeepcopy; 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 thatLib/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)*![]() | 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)*![]() | Date: 2009-11-28 15:45 | |
Guido - agreed! Versions updated. | |||
| msg95791 -(view) | Author: Antoine Pitrou (pitrou)*![]() | Date: 2009-11-28 15:59 | |
The patch has been committed inr76571 (trunk) andr76572 (py3k). Thank you! | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:56:28 | admin | set | github: 45856 |
| 2009-11-28 15:59:33 | pitrou | set | status: open -> closed resolution: accepted -> fixed messages: +msg95791 stage: resolved |
| 2009-11-28 15:45:30 | barry | set | messages: +msg95790 versions: + Python 3.2, - Python 2.6 |
| 2009-11-28 02:01:57 | gvanrossum | set | messages: +msg95779 |
| 2009-11-27 23:05:08 | rbcollins | set | files: +issue1515.patch messages: +msg95778 |
| 2009-11-27 22:40:36 | pitrou | set | messages: +msg95777 |
| 2009-11-27 22:25:06 | rbcollins | set | messages: +msg95776 |
| 2009-11-27 13:47:31 | pitrou | set | nosy: +pitrou messages: +msg95767 |
| 2009-11-27 13:39:01 | barry | set | nosy: +barry messages: +msg95766 |
| 2009-11-27 09:55:22 | rbcollins | set | messages: +msg95761 versions: + Python 2.7 |
| 2009-11-27 09:54:47 | rbcollins | set | files: +issue1515.patch nosy: +rbcollins messages: +msg95760 keywords: +patch |
| 2009-11-25 12:12:45 | cool-RR | set | nosy: +cool-RR messages: +msg95716 |
| 2008-01-31 23:57:39 | gvanrossum | set | messages: +msg61941 |
| 2008-01-31 22:54:03 | mlvanbie | set | messages: +msg61938 |
| 2008-01-31 18:38:58 | Dmitrey | set | messages: +msg61922 |
| 2008-01-31 18:11:26 | christian.heimes | set | resolution: accepted messages: +msg61921 |
| 2008-01-31 17:15:58 | gvanrossum | set | messages: +msg61909 severity: major -> normal |
| 2008-01-31 14:22:43 | christian.heimes | set | priority: normal messages: +msg61901 components: + Library (Lib) versions: + Python 2.6, - Python 2.5 |
| 2008-01-31 14:18:22 | christian.heimes | set | nosy: +christian.heimes messages: +msg61900 |
| 2008-01-31 10:05:13 | Dmitrey | set | nosy: +Dmitrey messages: +msg61893 severity: normal -> major |
| 2008-01-08 00:11:29 | mlvanbie | set | messages: +msg59508 |
| 2007-12-20 04:28:36 | gvanrossum | set | messages: +msg58854 |
| 2007-12-20 00:47:18 | mlvanbie | set | messages: +msg58851 |
| 2007-12-19 23:03:17 | gvanrossum | set | nosy: +gvanrossum messages: +msg58839 |
| 2007-11-29 00:41:00 | mlvanbie | create | |