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

collections.OrderedDict collaborative subclassing #68873

Closed
ericfrederich mannequin opened this issue Jul 22, 2015 · 9 comments
Closed

collections.OrderedDict collaborative subclassing #68873

ericfrederich mannequin opened this issue Jul 22, 2015 · 9 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ericfrederich
Copy link
Mannequin

ericfrederich mannequin commented Jul 22, 2015

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
    Copy link
    Mannequin Author

    ericfrederich mannequin commented Jul 22, 2015

    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/

    @ericfrederich ericfrederich mannequin added the stdlib Python modules in the Lib dir label Jul 22, 2015
    @rhettinger rhettinger self-assigned this Jul 23, 2015
    @rhettinger
    Copy link
    Contributor

    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
    Copy link
    Mannequin Author

    ericfrederich mannequin commented Jul 23, 2015

    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
    Copy link
    Mannequin Author

    ericfrederich mannequin commented Jul 23, 2015

    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.

    @ericfrederich ericfrederich mannequin reopened this Jul 23, 2015
    @ericfrederich ericfrederich mannequin removed the invalid label Jul 23, 2015
    @rhettinger
    Copy link
    Contributor

    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.

    @rhettinger rhettinger added the type-feature A feature request or enhancement label Jul 24, 2015
    @ericfrederich
    Copy link
    Mannequin Author

    ericfrederich mannequin commented Jul 24, 2015

    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
    Copy link
    Member

    merwok commented Jul 24, 2015

    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
    Copy link
    Mannequin Author

    ericfrederich mannequin commented Jul 24, 2015

    É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.

    @rhettinger
    Copy link
    Contributor

    Sorry Eric, I'm going to mark this as rejected. IMO this amounts to twisting the implementation into knots without any real payoff. I'm glad you enjoyed my Pycon talk and were inspired by it.

    @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-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants