Skip to content

Use CallbackRegistry in Widgets and some related cleanup #18226

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

Merged
merged 4 commits into from
Aug 18, 2020

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Aug 12, 2020

PR Summary

This came about from this Discourse post: https://discourse.matplotlib.org/t/have-a-button-disconnect-its-own-callback/21465
Using a CallbackRegistry simplifies processing, fixes memory leaks with weak references, and fixes bugs like being unable to disconnect a callback in its own code, as in the above post.

I also cleaned up some conditions and deprecated the old callback-related attributes.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [n/a] New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • [n/a] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

@QuLogic QuLogic added this to the v3.4.0 milestone Aug 12, 2020
@tacaswell tacaswell requested a review from timhoffm August 12, 2020 02:20
@tacaswell
Copy link
Member

It also needs to be mentioned in the API change note as if you register a method into it currently we will keep a hard-ref that keeps that object alive, the CallbackRegistry will only keep a weakref and if you otherwise let the object get gc'd your callback will go away. I don't think we need a deprecation path on that.

On the other hand, to channel @anntzer he will say we should always keep a hard-ref and keep objects alive (I am skeptical of this).

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note subtle behavior in keeping the objects of registered methods alive.

Anyone can dismiss this review and if there is one other 👍 @QuLogic can self-merge.

@QuLogic
Copy link
Member Author

QuLogic commented Aug 12, 2020

I added an API change note.

@anntzer
Copy link
Contributor

anntzer commented Aug 12, 2020

(I'm not going to fight over this, at least being consistent within the library is good too.)

The `.widgets.Widget` classes that allow event observers now use a
`.CallbackRegistry` to save callbacks. This provides consistency with canvas
event callbacks, and fixes some bugs that are already handled there. It also
means that callbacks are now stored as weak references, which means you must
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only applies to methods, we keep hard-refs to everything else.

Widget class internals
~~~~~~~~~~~~~~~~~~~~~~

Several `.widgets.Widget` class internals have been privatized and deprecated:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we think that there was anyone directly adding things to the observers dictionary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then they should have updated cnt so that followup additions would have the right number. I would expect them to get an error from that, and hopefully figure it out.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am 👍 on this, would like @timhoffm to review this before merged.

The wording on the deprecation can still be clarified a bit.

@cbook.deprecated("3.4")
@property
def cnt(self):
# Not real, but close enough.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widget.cnt is roughly the next cid to be returned, but this is just the count of the callbacks added. I can't get the next cid without reaching into CallbackRegistry internals (and maybe not even then since it uses itertools.count().)

This simplifies processing, fixes memory leaks with weak references, and
fixes bugs like being unable to disconnect a callback in its own code.
@timhoffm timhoffm merged commit a1159a4 into matplotlib:master Aug 18, 2020
@QuLogic QuLogic deleted the widget-cbs branch August 18, 2020 20:17
@anntzer
Copy link
Contributor

anntzer commented Nov 7, 2020

Actually I missed two points here:

  • We could have kept the old strong refs behavior by using the wrap-in-lambda trick (Prevent GC of animations and widgets still in use. #16221/Use CallbackRegistry for {Artist,Collection}.add_callback. #18912).
  • If you connect the same function twice to a CallbackRegistry, the second (and later) connections are silently dropped per
    if proxy in self._func_cid_map[s]:
    return self._func_cid_map[s][proxy]
    which is perhaps even more likely to be a source of API breakage than the strong/weakref difference. Again wrapping in lambdas solves the issue (because each lambda is effectively unique, and equality of lambdas doesn't look into their implementation), but at least this should be noted in the API changes note. (This can easily be checked by e.g. comparing for _ in range(2): gcf().canvas.mpl_connect("key_press_event", print) and for _ in range(2): gcf().canvas.mpl_connect("key_press_event", lambda e: print(e)).)

May be nice to decide on this before 3.4 gets released @QuLogic @tacaswell @timhoffm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants