classification
Title: collections.Counter with added attributes are not deepcopied properly.
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: Olivier.Gagnon, eric.snow, jcea, madison.may, rhettinger, serhiy.storchaka, vajrasky
Priority: low Keywords: patch

Created on 2013-07-03 14:43 by Olivier.Gagnon, last changed 2014-02-17 16:58 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
counter_copy_attrs.patch serhiy.storchaka, 2013-07-03 15:58 review
counter_copy_attrs_2.patch serhiy.storchaka, 2013-07-11 14:30 review
Messages (12)
msg192239 - (view) Author: Olivier Gagnon (Olivier.Gagnon) Date: 2013-07-03 14:43
The following code shows that the Counter is not deepcopied properly. The 
same code with an user defined class or a dict is copied with the "b" attribute.

import collections
import copy

count = collections.Counter()
count.b = 3
print(count.b) # prints 3

count_copy = copy.deepcopy(count)
print(count_copy.b) # raise AttributeError: 'Counter' object has no attribute 'b'
msg192245 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-03 15:58
Here is a preliminary patch. It changes copying and pickling to preserve instance attributes. Actually I'm not sure which attributes should be copied.

I doubt -- should this be considered as a fix or as a new feature?

Perhaps OrderedDict should be changed in same way.
msg192248 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-07-03 17:51
OrderedDict already copies the instance dict in __reduce__().  Are you talking about something more than that?
msg192256 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-03 20:49
I mean OrderedDict.copy().
msg192794 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-10 10:57
The question is whether we should give the freedom to user to add dynamic attribute to Counter object.

Is this freedom has any practicality? Why do we want to add dynamic attributes to Counter object?

Decimal object does not have this freedom.

>>> from decimal import Decimal
>>> z = Decimal('1.0')
>>> z.foo = 'a'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'decimal.Decimal' object has no attribute 'foo'

Actually I am not really sure about this. Maybe someone knows the answer.
msg192813 - (view) Author: Olivier Gagnon (Olivier.Gagnon) Date: 2013-07-10 14:28
The dictionary and the set do not give the freedom to add dynamic attributes to them. I agree that the Counter should have the same behaviour.

However, this will raise the same bug when we inherit from a Counter object.

>>> class mylist(list): pass
... 
>>> l = mylist()
>>> l.foo = "bar"
>>> c = copy.deepcopy(l)
>>> print(c.foo) # prints bar

>>> class myCounter(Counter): pass
... 
>>> original = myCounter()
>>> original.foo = "bar"
>>> c = copy.deepcopy(original)
>>> c.foo
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'myCounter' object has no attribute 'foo'

The reduction function should still copy every dynamic attribute of the object.
msg192872 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-11 14:30
Here is an updated patch. It includes a fix for OrderedDict.copy(). Counter's copy() and __reduce__() are simplified. Added tests for OrderedDict which checks that OrderedDict copying and pickling preserves dynamic attributes.
msg192922 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-07-12 06:23
I don't want the current behavior to change.  The copy() method does not guarantee that it will copy added attributes.
msg192931 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-12 07:54
What about pickling? OrderedDict.__reduce__() saves added attributes, but Counter.__reduce__() does not. One of them should be changed for consistency.
msg192936 - (view) Author: Olivier Gagnon (Olivier.Gagnon) Date: 2013-07-12 12:29
I can understand that the current behaviour can be correct in regard with the added attributes of the object. However, should I open a new issue for the following inheritance behaviour which the reduce function affects also.

class myCounter(Counter):
    def __init__(self, bar, *args):
        self.foo = bar
        super().__init__(*args)

class myDict(dict):
    def __init__(self, bar, *args):
        self.foo = bar
        super().__init__(*args)

c = myCounter("bar")
l = myDict("bar")
print(c.foo) # prints bar
print(l.foo) # prints bar

cc = copy.copy(c)
ll = copy.copy(l)
print(cc.foo) # prints {}
print(ll.foo) # prints bar
msg192996 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-07-13 04:23
Do you guys have any actual motivating use cases or real code that won't work because of the present design?   Consistency arguments are somewhat weak and don't necessarily warrant an API change.   

AFAICT, there is no use for counters going out of their way to save attributes.  Like many classes and types in Python, a subclasser is responsible for adding extra behavior if needed.
msg193010 - (view) Author: Olivier Gagnon (Olivier.Gagnon) Date: 2013-07-13 10:52
Yes I do have code that break because of this behaviour. I'm doing evolutionary algorithm using a framework called DEAP. This framework creates a type called individual at the runtime by subclassing a container and adding it a fitness attribute. Those individual are copied as not to modify every indivual when we work on a single one. AFAIK the only container that can't be used right now is the counter because the fitness is not copied. I'm sure I can come up with a hack to have this behaviour, but it does clash with other standard container type and there is no mention anywhere that the Counter should be different than every other container type in the python standard library.
History
Date User Action Args
2014-02-17 16:58:44jceasetnosy: + jcea
2013-07-13 10:52:37Olivier.Gagnonsetmessages: + msg193010
2013-07-13 04:23:27rhettingersetmessages: + msg192996
2013-07-12 12:29:28Olivier.Gagnonsetmessages: + msg192936
2013-07-12 07:54:25serhiy.storchakasetmessages: + msg192931
2013-07-12 06:23:18rhettingersetstatus: open -> closed
resolution: rejected
messages: + msg192922
2013-07-12 06:13:47rhettingersetpriority: normal -> low
assignee: rhettinger
2013-07-11 14:30:09serhiy.storchakasetfiles: + counter_copy_attrs_2.patch

messages: + msg192872
2013-07-10 14:28:35Olivier.Gagnonsetmessages: + msg192813
2013-07-10 10:57:01vajraskysetnosy: + vajrasky
messages: + msg192794
2013-07-03 20:49:34serhiy.storchakasetmessages: + msg192256
2013-07-03 17:51:48eric.snowsetnosy: + eric.snow
messages: + msg192248
2013-07-03 15:58:51serhiy.storchakasetfiles: + counter_copy_attrs.patch

versions: + Python 3.4, - Python 3.3
keywords: + patch
nosy: + rhettinger, serhiy.storchaka

messages: + msg192245
stage: patch review
2013-07-03 14:49:50madison.maysetnosy: + madison.may
2013-07-03 14:43:25Olivier.Gagnoncreate