Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Support unhashable callbacks in CallbackRegistry#26013
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not convinced this is a good idea, but looks technically correct.
…ion.... to remove the need to loop over all signals in _remove_proxy.
It is easier to manipulate a flat (signal, proxy) -> cid map rather thana nested signal -> (proxy -> cid) map.
... by replacing _func_cid_map by a dict-like structure (_UnhashDict)that also supports unhashable entries.Note that _func_cid_map (and thus _UnhashDict) can be dropped if we getrid of proxy deduplication in CallbackRegistry.
After discussion with@ksunden , we think it is simpler to fully remove |
Did you consider#20210 as well? |
We would still need the linear search in the |
Not if you also record the proxy's cid in the weakref callback, similarly to how I record the signal name atb629ff8#diff-84a45e2dda93ad4c1d950da5c16846965aa8d96aebfe8d00acaeb83a18c1b2f0R215? (Fortunately, per the weakref docs: "If the referents are still alive, two references have the same equality relationship as their referents (regardless of the callback".) |
you are correct. I went through a couple of versions of injecting the cid that did not work but did not make it to |
The patch is basically diff --git i/lib/matplotlib/cbook.py w/lib/matplotlib/cbook.pyindex 4d6dc04a9b..7c1b0ef246 100644--- i/lib/matplotlib/cbook.py+++ w/lib/matplotlib/cbook.py@@ -107,7 +107,7 @@ class _StrongRef: return hash(self._obj)-def _weak_or_strong_ref(func, callback):+def _weak_or_strong_ref(func, callback=None): """ Return a `WeakMethod` wrapping *func* if possible, else a `_StrongRef`. """@@ -204,7 +204,8 @@ class CallbackRegistry: cid_count = state.pop('_cid_gen') vars(self).update(state) self.callbacks = {- s: {cid: _weak_or_strong_ref(func, functools.partial(self._remove_proxy, s))+ s: {cid: _weak_or_strong_ref(func,+ functools.partial(self._remove_proxy, s, cid)) for cid, func in d.items()} for s, d in self.callbacks.items()} self._func_cid_map = {@@ -217,10 +218,15 @@ class CallbackRegistry: 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, functools.partial(self._remove_proxy, signal))+ # First check whether we're actually going to add the callback. This+ # relies on weakref comparison not taking the weakref destruction+ # callback (which includes the cid here) into account.+ proxy = _weak_or_strong_ref(func) if proxy in self._func_cid_map[signal]: return self._func_cid_map[signal][proxy] cid = next(self._cid_gen)+ proxy = _weak_or_strong_ref(func,+ functools.partial(self._remove_proxy, signal, cid)) self._func_cid_map[signal][proxy] = cid self.callbacks.setdefault(signal, {}) self.callbacks[signal][cid] = proxy@@ -238,16 +244,14 @@ class CallbackRegistry: # Keep a reference to sys.is_finalizing, as sys may have been cleared out # at that point.- def _remove_proxy(self, signal, proxy, *, _is_finalizing=sys.is_finalizing):+ def _remove_proxy(self, signal, cid, proxy, *, _is_finalizing=sys.is_finalizing): if _is_finalizing(): # Weakrefs can't be properly torn down at that point anymore. return- cid = self._func_cid_map[signal].pop(proxy, None)- if cid is not None:- del self.callbacks[signal][cid]- self._pickled_cids.discard(cid)- else: # Not found- return+ if self._func_cid_map[signal].pop(proxy, None) is None:+ return # Not found.+ del self.callbacks[signal][cid]+ self._pickled_cids.discard(cid) # Clean up empty dicts if len(self.callbacks[signal]) == 0: del self.callbacks[signal] where the need for pickling is really restricted only to _func_cid_map (i.e. deduplication) now. Even if we really want to support an option for deduplication (which I would say should not be the default) per#20210 (comment), we could just fall back to linear search for that case only. |
Coming back to this I think we do want to add an option to deduplicate or not defaulting to not (and a version where if not explicitly told we do deduplicate but warn we won't in the future). As part of that we should move to paying for a linear search and not pick up a dictionary cousin. |
@tacaswell perhaps I'll close this and let you take it over? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Following up, no further work has materialized, this adds some complexity, but does not break any existing behavior and fixes a thing that would be reasonable that it worked.
We can leave the de-duplication work for later (and punt simplifying it to then).
power-cycled to re-run the tests. |
9675122 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
* Record the connected signal in CallbackRegistry weakref cleanup function.... to remove the need to loop over all signals in _remove_proxy.* Flatten CallbackRegistry._func_cid_map.It is easier to manipulate a flat (signal, proxy) -> cid map rather thana nested signal -> (proxy -> cid) map.* Support unhashable callbacks in CallbackRegistry.... by replacing _func_cid_map by a dict-like structure (_UnhashDict)that also supports unhashable entries.Note that _func_cid_map (and thus _UnhashDict) can be dropped if we getrid of proxy deduplication in CallbackRegistry.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Closes#26012.
The first two commits are cleanup-ish:
a nested signal -> (proxy -> cid) map (especially for the third commit).
The last commit implements support for unhashable callbacks, by replacing _func_cid_map by a dict-like structure (_UnhashDict) that also supports unhashable entries.
Note that _func_cid_map (and thus _UnhashDict) can be dropped if we get rid of proxy deduplication in CallbackRegistry (#20210).
edit: ah, the joys of type-checking 😑 (python/mypy#4266)
PR checklist