-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Conversation
1c6f6c9
to
f82998b
Compare
f82998b
to
58b1000
Compare
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 👎 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.
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:
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).
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.
(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".)
[...] 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.
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.
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.
PR Summary
Closes #1656; improves the situation of #3105 (IMO).
It's really a mostly policy choice at that point.
PR Checklist