[Bug]: "Unhashable type" when event callback is a method of a dict subclass #26012

Activity

1. ksunden commented on May 30, 2023

   

   ksunden

   on May 30, 2023

   Member

   More actions

   dict objects are not hashable, and the hash we get from weakref.WeakMethod is to the containing class, not the method of the class...

   I think there is a reasonable argument to be had that it should be the hash of the bound method, not the class, but that is upstream CPython behavior, not mpl behavior directly... I do not think we are likely to give up on using WeakMethod for this use-case all together, and don't see a particularly good way to resolve it on our end...

   As a workaround (but acknowledging that it is taking away some of the "dict-like" behavior of your subclass, you can define a __hash__:

   class ThisNowWorksAgain(ThisWorks, dict):
       def __hash__(self):
           return ThisWorks.__hash__(self)

   A couple more thoughts:

   • subclassing from dict can be odd/hard to deal with in general (we do it in a small number of places here, but we are looking to stop doing so...)
   • multiple inheritance without calling super().__init__() is potentially part of the problem (though seemingly not the only problem...)
   • There is something odd about doing the connection with partially initialized objects:
     • The ThisWorks case self.sendEvent is still a function object (rather than a bound method, this causes fallback behavior of keeping a strong reference rather than a weak reference.
       • This feels odd, but is consistent with behavior of some examples, so evidently works.
     • The ThisDoesntWork case self.sendEvent is actually a bound method and thus weakref.WeakMethod will wrap it

   import matplotlib.pyplot as plt
   import weakref
   
   class ThisWorks:
       def __init__(self):
           self.fig, plots = plt.subplots(1)
           # take out the mpl_connect, so I can actually instantiate the subclass
           #self.fig.canvas.mpl_connect('key_press_event', self.sendEvent)
           self.keyListeners = {}
   
       def sendEvent(self, event):
           if event.name=='key_press_event' and event.key in self.keyListeners:
               for f in self.keyListeners[event.key]: f(self.model, event)
   
   class ThisDoesntWork(ThisWorks, dict): pass
   
   y = ThisWorks()
   y_ref = weakref.WeakMethod(y.sendEvent)
   x = ThisDoesntWork()
   x_ref = weakref.WeakMethod(x.sendEvent)
   
   print(hash(y) == hash(y_ref))  # True
   print(hash(y.sendEvent) == hash(y_ref))  # False
   
   hash(x) # raises TypeError
   hash(x_ref) # raises TypeError
   hash(x.sendEvent) # returns an int, no error
2. tacaswell commented on May 30, 2023

   

   tacaswell

   on May 30, 2023

   Member

   More actions

   Another workaround is:

   import matplotlib.pyplot as plt
   
   class ThisWorks:
       def __init__(self):
           self.fig, plots = plt.subplots(1)
           def smuggle(event):
               return self.sendEvent(event)
           self.fig.canvas.mpl_connect('key_press_event', smuggle)
           self._s = smuggle
           self.keyListeners = {}
   
   
       def sendEvent(self, event):
           if event.name=='key_press_event' and event.key in self.keyListeners:
               for f in self.keyListeners[event.key]: f(self.model, event)
   
   class ThisAlsoWorsk(ThisWorks, dict): pass
   
   x = ThisAlsoWorksm()

   I would not over-ride __hash__ on a mutable object unless you are very sure you understand the consequences (see https://docs.python.org/3/reference/datamodel.html#object.__hash__). Falling back to the ThisWorks hash method will fall back to using id(self), however the __eq__ implementation is presumably still inherited from dict and would thus violate:

   The only required property is that objects which compare equal have the same hash value;

   ---

   I think this is expected and correct behavior (leaving aside the running discussion of if we should hold hard or weak references to the callbacks) and think we should close this with no action. If we do take any action, it should be to clarify what the constraints on the callbacks are.

   ---

   I am now curious how __eq__ on bound methods is implemented as

   
   In [7]: ('a',).__len__ == ('a',).__len__
   Out[7]: True
   
   In [8]: {'a': 1}.__len__ == {'a': 1}.__len__
   Out[8]: False
   
   
3. tacaswell commented on May 30, 2023

   

   tacaswell

   on May 30, 2023

   Member

   More actions

   $ ipython
   Python 3.11.3 (main, Apr  5 2023, 15:52:25) [GCC 12.2.1 20230201]
   Type 'copyright', 'credits' or 'license' for more information
   IPython 8.13.2 -- An enhanced Interactive Python. Type '?' for help.
   
   In [1]: a = ('a', )
   
   In [2]: b = ('a',)
   
   In [3]: a is b
   Out[3]: False
   
   In [4]: a.__eq__ == b.__eq__
   Out[4]: False
   
   In [5]: a.__len__ == b.__len__
   Out[5]: False
   
   In [6]: ('a',).__len__ == ('a',).__len__
   Out[6]: True
   
   In [7]: a
   Out[7]: ('a',)
   
   In [8]: b
   Out[8]: ('a',)
   
   In [9]: b.__len__ == a.__len__
   Out[9]: False
   
   In [10]:
   

   I think https://github.com/python/cpython/blob/4571eedca2f70768ddc4a2fd4fba2ae11b4c8037/Objects/descrobject.c#L1322-L1332 is the hash implementation and https://github.com/python/cpython/blob/4571eedca2f70768ddc4a2fd4fba2ae11b4c8037/Objects/descrobject.c#L1296-L1320 is the compare implementation.

   I do not have a good explanation as to why In [6] behaves that way. I had a guess that it was related to PyObjects getting reused (so the first tuple gets gc'd before the second one gets created), but

   In [9]: ('a',) is ('a',)
   <>:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
   <ipython-input-9-4e921d479efb>:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
     ('a',) is ('a',)
   Out[9]: True
   

   so this must be something about the way Python handles literals that come in via the same expression/statement?!
4. anntzer commented on May 30, 2023

   

   anntzer

   on May 30, 2023

   Contributor

   More actions

   @tacaswell I guess the peephole optimizer, or something similar, arranges to store only once the constant in the code object:

   >>> code = compile("('a',) is ('a',)", "<string>", "eval")
   <string>:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
   >>> code.co_consts
   (('a',),)
   >>> dis.dis(code)
     0           0 RESUME                   0
   
     1           2 LOAD_CONST               0 (('a',))
                 4 LOAD_CONST               0 (('a',))
                 6 IS_OP                    0
                 8 RETURN_VALUE
   
5. anntzer commented on May 30, 2023

   

   anntzer

   on May 30, 2023

   Contributor

   More actions

   Actually it looks like the hashability requirement is only used to maintain _func_cid_map, which itself only serves to simplify the callback deduplication code, which I think is a misfeature anyways (#20210) (not the same misfeature as using weakrefs ;-))
   In any case we can easily do without _func_cid_map, at the small cost of making callback registration O(already-present-callbacks) as we instead iterate over all previously present callbacks to check whether the new one is a duplicate or not (if we drop the deduplication (mis)feature, which is something that can be done with a normal deprecation (see discussion at #20210), we go back to O(1) callback registration). The patch should be approximately

   diff --git i/lib/matplotlib/cbook.py w/lib/matplotlib/cbook.py
   index ae78eab831..30b8173b2d 100644
   --- i/lib/matplotlib/cbook.py
   +++ w/lib/matplotlib/cbook.py
   @@ -169,16 +169,14 @@ class CallbackRegistry:
            handled signals.
        """
    
   -    # We maintain two mappings:
   +    # We maintain a mapping:
        #   callbacks: signal -> {cid -> weakref-to-callback}
   -    #   _func_cid_map: signal -> {weakref-to-callback -> cid}
    
        def __init__(self, exception_handler=_exception_printer, *, signals=None):
            self._signals = None if signals is None else list(signals)  # Copy it.
            self.exception_handler = exception_handler
            self.callbacks = {}
            self._cid_gen = itertools.count()
   -        self._func_cid_map = {}
            # A hidden variable that marks cids that need to be pickled.
            self._pickled_cids = set()
    
   @@ -190,30 +188,24 @@ class CallbackRegistry:
                "callbacks": {s: {cid: proxy() for cid, proxy in d.items()
                                  if cid in self._pickled_cids}
                              for s, d in self.callbacks.items()},
   -            # It is simpler to reconstruct this from callbacks in __setstate__.
   -            "_func_cid_map": None,
            }
    
        def __setstate__(self, state):
            vars(self).update(state)
            self.callbacks = {
   -            s: {cid: _weak_or_strong_ref(func, self._remove_proxy)
   +            s: {cid: _weak_or_strong_ref(func, functools.partial(self._remove_proxy, s))
                    for cid, func in d.items()}
                for s, d in self.callbacks.items()}
   -        self._func_cid_map = {
   -            s: {proxy: cid for cid, proxy in d.items()}
   -            for s, d in self.callbacks.items()}
    
        def connect(self, signal, func):
            """Register *func* to be called when signal *signal* is generated."""
            if self._signals is not None:
                _api.check_in_list(self._signals, signal=signal)
   -        self._func_cid_map.setdefault(signal, {})
   -        proxy = _weak_or_strong_ref(func, self._remove_proxy)
   -        if proxy in self._func_cid_map[signal]:
   -            return self._func_cid_map[signal][proxy]
   +        proxy = _weak_or_strong_ref(func, functools.partial(self._remove_proxy, signal))
   +        for cid, p in self.callbacks.setdefault(signal, {}).items():
   +            if p == proxy:
   +                return cid
            cid = next(self._cid_gen)
   -        self._func_cid_map[signal][proxy] = cid
            self.callbacks.setdefault(signal, {})
            self.callbacks[signal][cid] = proxy
            return cid
   @@ -230,23 +222,19 @@ class CallbackRegistry:
    
        # Keep a reference to sys.is_finalizing, as sys may have been cleared out
        # at that point.
   -    def _remove_proxy(self, proxy, *, _is_finalizing=sys.is_finalizing):
   +    def _remove_proxy(self, signal, proxy, *, _is_finalizing=sys.is_finalizing):
            if _is_finalizing():
                # Weakrefs can't be properly torn down at that point anymore.
                return
   -        for signal, proxy_to_cid in list(self._func_cid_map.items()):
   -            cid = proxy_to_cid.pop(proxy, None)
   -            if cid is not None:
   -                del self.callbacks[signal][cid]
   -                self._pickled_cids.discard(cid)
   +        for cid, p in self.callbacks[signal].items():
   +            if p == proxy:
                    break
   -        else:
   -            # Not found
   +        else:  # Not found.
                return
   -        # Clean up empty dicts
   -        if len(self.callbacks[signal]) == 0:
   +        del self.callbacks[signal][cid]
   +        self._pickled_cids.discard(cid)
   +        if len(self.callbacks[signal]) == 0:  # Clean up empty dicts
                del self.callbacks[signal]
   -            del self._func_cid_map[signal]
    
        def disconnect(self, cid):
            """
   @@ -254,25 +242,16 @@ class CallbackRegistry:
    
            No error is raised if such a callback does not exist.
            """
   -        self._pickled_cids.discard(cid)
            # Clean up callbacks
   -        for signal, cid_to_proxy in list(self.callbacks.items()):
   -            proxy = cid_to_proxy.pop(cid, None)
   -            if proxy is not None:
   +        for signal in self.callbacks:
   +            if cid in self.callbacks[signal]:
                    break
   -        else:
   -            # Not found
   +        else:  # Not found
                return
   -
   -        proxy_to_cid = self._func_cid_map[signal]
   -        for current_proxy, current_cid in list(proxy_to_cid.items()):
   -            if current_cid == cid:
   -                assert proxy is current_proxy
   -                del proxy_to_cid[current_proxy]
   -        # Clean up empty dicts
   -        if len(self.callbacks[signal]) == 0:
   +        del self.callbacks[signal][cid]
   +        self._pickled_cids.discard(cid)
   +        if len(self.callbacks[signal]) == 0:  # Clean up empty dicts
                del self.callbacks[signal]
   -            del self._func_cid_map[signal]
    
        def process(self, s, *args, **kwargs):
            """

   (plus adjusting the tests that actually try to touch _func_cid_map).
6. ksunden

   closed this as completedin #26013on Sep 19, 2024
7. QuLogic

   added this to the v3.10.0 milestone on Sep 19, 2024