Skip to content

Prevent GC of animations and widgets still in use. #16221

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 14, 2020

PR Summary

Closes #1656; improves the situation of #3105 (IMO).

It's really a mostly policy choice at that point.

PR Checklist

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

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.

If animation / widgets objects can keep themselves alive and the user loses their references to them, I don't see how they can recover one to stop / disconnect it.

If we are going to go this route of keeping animations alive, I think monkey patching them onto the Figure is a better option than hiding hard references in lambdas in the callbacks.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 15, 2020

If animation / widgets objects can keep themselves alive and the user loses their references to them, I don't see how they can recover one to stop / disconnect it.

If they want to disconnect it they just need to keep a ref to it from the start. I see this as similar to starting a thread in Python without keeping a ref to it:

from threading import Thread; from time import sleep

def work():
    while True:
        sleep(1); print("still here")

Thread(target=work).start()

if you haven't kept a ref on the thread you can't e.g. join() or do anything with it, and it doesn't get garbage collected. (And this is pretty close to the issue at hand, as we're basically arranging for stuff to be called in a separate thread.) So there's definitely prior art for it.

If we are going to go this route of keeping animations alive, I think monkey patching them onto the Figure is a better option than hiding hard references in lambdas in the callbacks.

Well, I really don't like the fact that we use weakrefs anyways. This is also inconsistent with how all GUI toolkits work -- they also keep hardrefs to everything, that's (I think) how the animations still work here (the GUI timers keep hard refs to them).

@timhoffm
Copy link
Member

timhoffm commented Jan 16, 2020

Keeping an internal reference via a lambda is quite arcane. If we really want to use that, it should be documented more extensively.

However, I have the feeling that this is not the right design. In particular, (if I understand this magic correctly) animations would have a zombie life and we'd burden the user to prevent that if they don't want it.

I don't want to re-iterate #1656, but some ideas might clarify the nature of the issue:

Why does the figure not hold a reference to the animation?

In contrast to artists, which are part of the figure, an animation is a kind of FigureController (name made up). It sits outside the figure, manipulates its artists from time to time and calls for redraws. In that sense it's quite similar to GUI toolkits. The controller holds the figure, not the other way round.

In contrast to GUIs, there is nobody naturally holding a reference to the FigureController in case of animations. And then it gets garbage collected.

Why it might nevertheless be a good idea for the figure to hold a reference

The analogy with threads is not too far fetched. However, given that our average user is not necessarily an experienced programmer, I don't expect him to know about references, garbage collection or analogies with threads.

IMHO it would be ok if any FigureController registers itself with the figure it wants to control. That can be as simple as the figure holding a private list of registered controllers. The figure doesn't have to know what the controller is and doesn't do anything with it (could be changed if there was a use later). This is still a rather loose coupling.

From the reference point of view, if nobody outside holds a reference to the figure anymore, figure and animation will get cleaned up together (whereas a zombie animation would keep the figure alive).

Yes, two PRs towards #1656 adding animation references to the figure were rejected, but I still find it the best solution. Maybe the rephrasing as FigureController gives a new logic to motivate that.

The proposed solution proposed here works for both animations and widgets.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 17, 2020

(I allowed myself to reorder your paragraphs to present the reply better.)

Keeping an internal reference via a lambda is quite arcane. If we really want to use that, it should be documented more extensively.

Well, it's needed only because CallbackRegistry has the (questionable, IMO) semantics of only keeping weakrefs to bound methods -- which means that foo.connect("signal", object.method) and foo.connect("signal", lambda *args, **kwargs: object.method(*args, **kwargs)) are subtly different (they should not, but fortunately the second one has the semantics I want).

However, I have the feeling that this is not the right design. In particular, (if I understand this magic correctly) animations would have a zombie life and we'd burden the user to prevent that if they don't want it.

The analogy with threads is not too far fetched. However, given that our average user is not necessarily an experienced programmer, I don't expect him to know about references, garbage collection or analogies with threads.

The inexperienced programmer is actually exactly the one that would benefit the most here. Right now if you look at the animation examples they all finish with the assignment of an unused variable (anim = ...). Fortunately flake8 and friends don't seem to warn about it (even without the excludes file), but how is an inexperienced programmer supposed to understand that assigning the animation object to an unused name is supremely important for the thing to work? It may in fact promote "bad" understandings of how "assignment" works (e.g. you can compare with MATLAB functions, which can check whether their return value is being assigned to something or not (via nargout) and behave differently depending on that.

I don't want to re-iterate #1656, but some ideas might clarify the nature of the issue:

Why does the figure not hold a reference to the animation?

In contrast to artists, which are part of the figure, an animation is a kind of FigureController (name made up). It sits outside the figure, manipulates its artists from time to time and calls for redraws. In that sense it's quite similar to GUI toolkits. The controller holds the figure, not the other way round.

In contrast to GUIs, there is nobody naturally holding a reference to the FigureController in case of animations. And then it gets garbage collected.

Actually we already behave in the opposite way compared to GUI toolkits: in PyQt if you create a QMainWindow() but don't assign it to anything, then it will be gc'd, whereas we maintain an explicit cache of figure()s (aka pyplot). So I don't think the analogy holds.

Why it might nevertheless be a good idea for the figure to hold a reference

(I'm fine with that too. But it's really a lot of machinery for something that should be simple: "if something is supposed to handle callbacks, don't let it be dropped -- or, if you really want it to be weakref'd, set up the weakref yourself at connection time".)

@timhoffm
Copy link
Member

timhoffm commented Feb 16, 2020

[...] are subtly different (they should not, but fortunately the second one has the semantics I want).

We shouldn't build functionality on accidental and undesired subtle code behavior.

Actually we already behave in the opposite way compared to GUI toolkits: [...] So I don't think the analogy holds.

Fair enough. But nevertheless, my point holds that the best approach here would be for the figure to hold a strong reference to the animation.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 16, 2020

We shouldn't build functionality on accidental and undesired subtle code behavior.

My point is that this brittleness/subtleness can, AFAICT, only be removed by making CallbackRegistry hold strong references.

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jul 7, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Jul 7, 2023

Effectively tracked by #16853 and #24373; we can always reopen this if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: inactive Marked by the “Stale” Github Action status: needs comment/discussion needs consensus on next step status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animations & Object Persistence
4 participants