Skip to content
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

global map of widgets should utilize weak references to avoid memory leaks and excessive reload costs #1345

Open
maxnoy opened this issue May 11, 2017 · 10 comments
Milestone

Comments

@maxnoy
Copy link
Contributor

maxnoy commented May 11, 2017

Currently, all widgets created through the life of the kernel are referenced in a map Widgets.widgets. That map grows if the widgets are not closed down explicitly. It should be some version of WeakDictionary to avoid leaking memory and increasing the time to reinitialize the state of a notebook.

@jasongrout jasongrout added this to the 7.0 milestone May 11, 2017
@maartenbreddels
Copy link
Member

Good idea 👍 always wanted to raise this point, I hope it's this simple though.

@jasongrout
Copy link
Member

This may not help as much as you think. If the comm is active, I think it will have a reference to the widget instance. If you close the widget (which shuts down the comm), it removes itself from the global cache.

@jasongrout
Copy link
Member

I think we should send the entire state of all the widgets at once on page reload, instead of making tons of separate requests.

@maxnoy
Copy link
Contributor Author

maxnoy commented May 12, 2017

Is the memory leak on the kernel side a concern? It doesn't seem like the widgets get closed automatically.

@jasongrout
Copy link
Member

Is the memory leak on the kernel side a concern? It doesn't seem like the widgets get closed automatically.

I think the point is that it isn't a memory leak if it's still communicating with something on the frontend. And the other point is that a weak reference to an active model won't do us any good because the comm still maintains a reference.

Another way to think about the lifecycle problem: the widget needs to be considered unreachable when both the frontend and the backend have no way of using it. That's a tricky problem.

@jasongrout
Copy link
Member

This is sufficiently tricky that I'll push it to the Future milestone and put the discussion label on it.

@jasongrout jasongrout modified the milestones: Future, 7.0 May 17, 2017
@spott
Copy link

spott commented Jan 22, 2019

Is this still an issue? I believe I'm seeing this with another project, and I'm curious how I would go about explicitly closing down the widgets (do I do this from python, or from the javascript side?). Does anyone have a hint on where I should be doing the explicit closing?

@jasongrout
Copy link
Member

We still don't use weak references. If anyone wants to explore it, please go for it.

@ianhi
Copy link
Contributor

ianhi commented Apr 1, 2021

I'm curious how I would go about explicitly closing down the widgets (do I do this from python, or from the javascript side?). Does anyone have a hint on where I should be doing the explicit closing?

For future reference I think this should do it:

from ipywidgets import Widget
Widget.close_all()

@maartenbreddels
Copy link
Member

Note that this problem is solved using https://github.com/widgetti/react-ipywidgets where we manage the widget lifecycle.
e.g.:
image

If we now execute the next two cells:

image

We can see that all widgets that are managed by this react-ipywidgets render context are cleaned up.

Not only are widgets closed/destroyed when the full context is closed, but also when elements/widgets are not used anymore, i.e. during the lifetime of an application (conditional rendering etc)

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

No branches or pull requests

5 participants