collections.OrderedDict collaborative subclassing

[ericfrederich]

BPO	24685
Nosy	@rhettinger, @merwok, @ericsnowcurrently, @ericfrederich
Files	inj.py: Test collection.OrderedDict for cooperative dependency injection inj2.py inj3.py

^{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 = 'https://github.com/rhettinger'
closed_at = <Date 2016-04-26.06:47:00.828>
created_at = <Date 2015-07-22.17:04:00.967>
labels = ['type-feature', 'library']
title = 'collections.OrderedDict collaborative subclassing'
updated_at = <Date 2016-04-26.06:47:00.827>
user = 'https://github.com/ericfrederich'

bugs.python.org fields:

activity = <Date 2016-04-26.06:47:00.827>
actor = 'rhettinger'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2016-04-26.06:47:00.828>
closer = 'rhettinger'
components = ['Library (Lib)']
creation = <Date 2015-07-22.17:04:00.967>
creator = 'eric.frederich'
dependencies = []
files = ['39982', '39999', '40000']
hgrepos = []
issue_num = 24685
keywords = []
message_count = 9.0
messages = ['247136', '247160', '247224', '247228', '247237', '247276', '247293', '247295', '264220']
nosy_count = 4.0
nosy_names = ['rhettinger', 'eric.araujo', 'eric.snow', 'eric.frederich']
pr_nums = []
priority = 'low'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue24685'
versions = ['Python 3.6']

[ericfrederich]

After watching the PyCon talk Super considered super[1] and reading the corresponding blog post[2] I tried playing with dependency injection.

I was surprised to notice that the example he gave did not work if I swap the order of the classes around. I think it should have. See attached file.

I think this is a bug in collections.OrderedDict
OrderedDict is not well-behaved as far as cooperative subclassing is concerned.

The source code is hard wired with a keyword argument dict_setitem=dict.__setitem__ which it then calls at the end with dict_setitem(self, key, value)

A quick search of github for dict_setitem shows that this
bad practice seems be making its way into other projects

If dict_setitem keyword arg is really necessary to have, then maybe:

(a) have it default to None
(b) at the end of __setitem__ do something like:

        if dict_setitem is not None:
            return dict_setitem(self, key, value)
        
        super(OrderedDict, self).__setitem__(key, value)

After a discussion on #py-dev this seemed like a reasonable request (not necessarily the implementation, but the idea that OrderedDict should cooperate).
I tested this against the C implementation of OrderedDict in Python 3.5 and noticed that it doesn't cooperate either.

[1] https://www.youtube.com/watch?v=EiOglTERPEo
[2] https://rhettinger.wordpress.com/2011/05/26/super-considered-super/

[rhettinger]

This is an intentional design choice. One reason for tightly coupling OrderedDict to dict was to preserve freedom for a C-implementation. Another reason was for performance. IIRC, using super() in __setitem__ slowed the OD from 10x slower than dicts to 20x.

Non-cooperative classes (of which Python has many) can be wrapped to make the classes cooperative. The technique is discussed in the blog post https://rhettinger.wordpress.com/2011/05/26/super-considered-super/ .

[ericfrederich]

Raymond,

Thanks for the explanation of your reasoning.
Could you please provide an example of how to create a cooperative subclass of OrderedDict?

I have attempted to make one.
I succeeded to make it work where the previous example failed but in doing made the example that used to work now fail.
I have attached my attempt at making a cooperative OrderedDict in inj2.py

class coop_OrderedDict(OrderedDict):
    """
    A cooperative version of OrderedDict
    """
    def __setitem__(self, k, v, **kwargs):
        # OrderedDict calls dict.__setitem__ directly skipping over LoggingDict
        # fortunately we can control this with dict_setitem keyword argument

        # calculate OrderedDict's real parent instead of skipping right to dict.__setitem__
        # though depending on the hierarchy it may actually be dict.__setitem__
        m = super(OrderedDict, self).__setitem__
        # dict_setitem wants an unbound method
        unbound_m = m.im_func
        return super(coop_OrderedDict, self).__setitem__(k, v, dict_setitem=unbound_m)

In Python2 it fails with:

Traceback (most recent call last):
  File "/tmp/inj2.py", line 51, in <module>
    old1['hooray'] = 'it worked'
  File "/tmp/inj2.py", line 30, in __setitem__
    return super(LoggingDict, self).__setitem__(k, v)
  File "/tmp/inj2.py", line 18, in __setitem__
    unbound_m = m.im_func
AttributeError: 'method-wrapper' object has no attribute 'im_func'

In Python3 both cases fail.

[ericfrederich]

Attached, as inj3.py, is a version I made which seems to work with Python2 but not with Python3's C implementation of OrderedDict.

I had to walk the MRO myself to get the unbound method to pass along as dict_setitem.

With Python3 it doesn't look like doing this was left configurable.
It crashes complaining "TypeError: wrapper __setitem__ doesn't take keyword arguments"

Re-opening this bug since it seems impossible to make OrderedDict cooperative in Python3 even with a wrapper.

Perhaps Python3's OrderedDict should either
(a) be cooperative at the C level
(b) support dict_setitem keyword argument to maintain compatibility with Python2.

[rhettinger]

Eric, I believe you're profoundly misunderstanding my Pycon talk. The posted code is not expected "to work if I swap the order of the classes around" -- MRO's are controlled by the child classes (subject to a number of constraints) and the order classes are listed matters greatly. Also, cooperative classes have to be designed cooperatively. The standard library OD class was intentionally tightly bound to its parent class to provide opportunities for a C implementation, to provide acceptable performance, and to make it easier for Jython and PyPy to implement the OD in ways favorable to their implementations. Likewise it was intentional to hide access to the underlying dict using name mangling for the same reasons (that one was Guido's suggestion). Also note, Guido's defaultdict also won't let you step in-between the class and its parent.

Also, your posted mal-functioning wrapper is mis-implemented. Please see the "How to Incorporate a Non-cooperative Class" section of the referenced blog post. The dependency injection technique is something that be used when testing your own subclasses which were built a loose coupling to a "default supplier" and where the calls between the consumer and supplier are a well defined part of the public API. Only a handful of standard library tools were meant to be used this way. (That is why I the non-cooperative class integration part of the blog post was necessary).

I've reclassified the tracker item as a feature request for 3.6. Without compelling use cases, it will likely be re-rejected shortly. That said, I'm concerned that you've got "a burr under your saddle" and won't take no for an answer, so I'll leave this open for a while to allow you a chance to think about it and re-close it yourself. I hope your will understand that we have valid reasons for not over-specifying the API and have a reluctance to tie our hands to the implementation details as the OD gets transitioned to a C implementation suitable internal use within the core. FWIW, the OD code is pure python and there's nothing stopping you from using your own variants.

[ericfrederich]

I understand that in the general case you cannot just swap the order around and get the same behaviour.

This LoggingDict just prints some stuff to the screen and delegates to super and so I believe it should work wherever it is placed in a cooperative hierarchy. Do you agree?

Now, I understand that OrderedDict is not cooperative. You stated that this is a design decision and I respect that choice, but you also stated that classes can be made to be cooperative by creating a wrapper.

The reason I re-opened this bug is because I fail to see a way in which to create such a wrapper for Python3. Do you believe that it should be possible to create a cooperative wrapper?

If it is possible (and its just my inability to create one) then I have no issue and the bug should be closed.
If it is not possible, then perhaps it could be noted somewhere that its not cooperative and impossible to make it cooperative and it should be listed last when using multiple inheritance.

[merwok]

FWIW I once helped a friend combine OrderedDict and defaultdict:
https://gist.github.com/merwok/11268759

The behavior can be surprising if we don’t know what Raymond said about design choices in OrderedDict, but it was not hard (in the default+ordered case) to work around.

[ericfrederich]

Éric (Araujo),

Combinding defaultdict and OrderedDict is a little easier since one of them (defaultdict) has special behavior on getitem while the other (OrderedDict) has special behavior on setitem.

I played with mixing those two myself and saw some issues and found that I had to explicitly call __init__ on both base classes to get them primed properly.