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

Clarify how to fully clean widget state when updating versions #1866

Open
ricklupton opened this issue Dec 10, 2017 · 24 comments
Open

Clarify how to fully clean widget state when updating versions #1866

ricklupton opened this issue Dec 10, 2017 · 24 comments
Milestone

Comments

@ricklupton
Copy link

I have some example notebooks in my custom widget repository that I want to update to use the latest version of the JS package whenever I release a new version, but I find it difficult to get it right on first try and often end up with old widget states still in my notebooks.

From experimenting I think the required steps are:

  1. Open notebook
  2. Clear cell output
  3. Clear widget state
  4. Refresh page
  5. Execute cells
  6. Save widget state

(this seems consistent with this comment)

Step 4 seems to be required -- perhaps some widget state persists in the client-side JS even after running "clear widget state". Is this intended? If so, can it be made clearer that this step is necessary, because running "clear widget state" sounds like it should be enough?

I wasn't sure what minimal example would help but I can try to put one together if needed.

@jasongrout
Copy link
Member

Thanks - very good points! The "Clear widget state" menu option is for clearing the stored state in a notebook file, not for clearing the state on the page. You're right that (a) the language is confusing, and (b) it's frustrating that saving the notebook again saves all of the running state, which includes state originally from the document.

I think we should make "clear widget state" clear the runtime state, as well as the document state - this sequence of steps always felt awkward to me.

I usually add a step between 3 and 4, restart kernel, to make sure that I don't pull existing widget state from the kernel over to the notebook frontend.

@jasongrout
Copy link
Member

On the other hand, do you think we should have a menu option to just clear the runtime state? You can do this from python code with:

from ipywidgets import Widget
Widget.close_all()

def close_all(cls):

@jasongrout jasongrout added this to the 7.x milestone Dec 11, 2017
@ricklupton
Copy link
Author

Another option -- which I think it what I expected to happen at first -- would be for the "clear widget state" menu item to just clear the runtime state. "Save widget state" would be the only action that modifies the document. Then the widget-state operations would be:

  1. Clean out widget state from notebook document: clear widget state -> save widget state
  2. Replace existing widget state with new outputs: clear widget state -> run cells -> save widget state
  3. Run and save widgets in a new notebook: run cells -> save widget state

In other words, the current "clear widget state [in document]" operation would be replaced by "clear [runtime] + save [runtime to document]", so there's no loss of functionality.

Is that everything you want to do with widget states? Is restarting the kernel necessary? Is clearing cell outputs before clearing widget state necessary?

@ricklupton
Copy link
Author

I suppose the point about clearing cell outputs before clearing widget states is that it avoids left-over views with no model. But it's already possible to get into that state and it doesn't matter as the views are about to be overwritten in my case.

@jasongrout
Copy link
Member

you should never have views without a model - closing down a model gets rid of the views.

I like your explanation above. Then clear widget state becomes Widget.close_all() from the front end. I think we probably need a different name to convey that we are actually going to close all of the models, so you won't have any models in the kernel either.

Close all widgets? That conveys that it is a runtime thing and mirrors the Widget.close_all().

@ricklupton
Copy link
Author

I think Close all widgets sounds good.

While we're on the topic, I feel the current Save Notebook Widget State would be slightly clearer as Save widget state to notebook if that's not too long. That would then be consistent with Download widget state. I find Embed widgets a bit confusing too as I always think of "embedding" the widget state into the notebook, but that's not what it does -- perhaps Copy widget state to embed (again, if not too long)

  • Close all widgets

  • Save widget state to notebook
  • Download widget state
  • Copy widget state to embed

@jasongrout
Copy link
Member

Is that everything you want to do with widget states? Is restarting the kernel necessary? Is clearing cell outputs before clearing widget state necessary?

I don't think restarting a kernel closes all of the active widgets - I think they stick around in the frontend (sort of orphaned, but still providing models and views to the outputs).

Perhaps any kernel restart should do a 'close all widgets' on the frontend, which will automatically clear them from the outputs.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Jan 20, 2018

Another way to see this is to come back to the idea of a global notebook-level output area.

At the moment, a widget front-end event coming from a model (i.e. a controller widget) cannot result in something being printed in a cell, because it does not come from a specific view and is not associated with any cell in particular.

If there was a notion of non-cell-associated output area (toggable in the UI), we would still be able to get these events for debugging purposes.

Also, this would have the benefit of letting up make the widget manager state part of the output, instead of the notebook-level metadata.

Clear all output would then naturally remove the widget manager state from this notebook-level output.

@SylvainCorlay
Copy link
Member

Another advantage of this, is that the Restart and Restart & Clear Output kernel actions would have a clear meaning for the widgets state, and this would prevent having multiple inconsistent means of changing what is saved, between actions of the Kernel menu and the Widgets menu.

@ricklupton
Copy link
Author

make the widget manager state part of the output, instead of the notebook-level metadata.

Currently it's possible to use widgets without saving the state in the notebook (just by never clicking "save widget state"). If widget state is part of the output, you would save state to notebooks by default. That makes sense to me, but I wonder if there was a reason to not save state by default originally?

@vidartf
Copy link
Member

vidartf commented Feb 12, 2018

Perhaps any kernel restart should do a 'close all widgets' on the frontend, which will automatically clear them from the outputs.

At the very least, if you do a "Restart Kernel and Clear All Outputs" action, I can see no reason why it should not also close all widgets. This would basically give the user an avenue to avoid the browser refresh (step 4. in initial description), at low (no?) risk.

@ricklupton
Copy link
Author

I think saving widget manager state as part of the output (instead of in the notebook metadata) would make widgets behave as requested in #1632 too

@vidartf
Copy link
Member

vidartf commented Feb 14, 2018

@ricklupton Given that the same widget can be used in several outputs, this doesn't seem feasible:

  • The widget state is stored several places (duplication, leading to bloat).
  • There can be conflicting versions of the same widget data stored in different outputs. What should be the state in the kernel on a restore?

@ricklupton
Copy link
Author

@vidartf I was referring to SilvainCorlay's comment about embedding widgets in notebook level (rather than cell-level outputs) -- which I think is currently just a suggestion but wouldn't suffer from those problems as I understand it. But maybe I misunderstood, I was just trying to link the discussion in the two issues!

@jasongrout jasongrout modified the milestones: Minor release, 7.2 Mar 21, 2018
@jasongrout
Copy link
Member

jasongrout commented Mar 21, 2018

Adding to 7.2, with the idea that we can make some progress in clarifying the menu items here. As in the conversation above, I'm thinking the menu will now read (adding a close all item, and tweaking the wording):

  • Close all widgets - closes all widgets currently in the widget manager (which also closes them in the kernel)
  • Save widget state to notebook - Saves the current widget manager state to the notebook, overwriting any existing widget manager state.
  • Clear widget state in notebook - Could be done by close all widgets, then saving widget state, but is more convenient
  • Download widget state - Same as now
  • Copy widget state to html page - same as "Embed" now

@ricklupton
Copy link
Author

Sounds good. Is it obvious that Clear widget state in notebook will also close the visible widgets on the page? Close widgets & clear state in notebook ?

@jasongrout
Copy link
Member

Sounds good. Is it obvious that Clear widget state in notebook will also close the visible widgets on the page?

Ah, I was thinking it wouldn't automatically close all widgets. (Perhaps this is rehashing conversation above...) Should it?

@ricklupton
Copy link
Author

Clear widget state in notebook - Could be done by close all widgets, then saving widget state, but is more convenient

I read this ("could be done by ...") to mean it would close all widgets, is that what you meant?

@jasongrout
Copy link
Member

Right - I guess it isn't exactly equivalent, but perhaps that is too confusing anyway.

@ricklupton
Copy link
Author

In my opinion it would be simplest to understand if there was just one "save" command that modifies the widget state in the file on disk, which always makes the state on disk the same as the state you currently see, as I think I suggested above. So to clear state on disk you would "close all widgets" then "save widget state to notebook". The Clear widget state in notebook (but don't clear the runtime state) option I find unnecessarily confusing (as we've just seen!)

If you're worried about it needing two clicks then maybe follow the example of the Restart kernel & ... options to have a Close all widgets & save widget state to notebook -- less snappy but obvious.

But maybe that's just me. Either way I think the changes you suggested today would be a good improvement.

@jasongrout
Copy link
Member

Okay, you've convinced me. So here's the new proposal:

  • Close all widgets - closes all widgets currently in the widget manager (which also closes them in the kernel if they exist in the kernel).
  • Save widget state to notebook - Saves the current widget manager state to the notebook, overwriting any existing widget manager state.
  • Download widget state - Same as now
  • Copy widget state to html page - same as "Embed" now

@jasongrout
Copy link
Member

jasongrout commented Mar 22, 2018

Tweaked wording:

Close All Widgets
----------------
Save Widgets in Notebook
----------------
Download Widget State
Export Widgets to HTML

Thoughts? Perhaps "Save Widgets to Notebook"?

jasongrout added a commit to jasongrout/ipywidgets that referenced this issue Mar 22, 2018
@ricklupton
Copy link
Author

Sounds good! "Save Widgets to Notebook" is better, as you've done in #2012

@benbovy
Copy link

benbovy commented Dec 14, 2023

I stumbled upon this issue while struggling with widgets that seem to be still alive in the front-end after a kernel restart.

I have some custom "headless" widgets that wrap audio nodes (Web Audio API) or WebMIDI event listeners. When closing those widgets by calling .close() or ipywidget.Widget.close_all(), the models are properly destroyed in the front-end (audio nodes are disposed and MIDI event listeners are removed). However, it is too easy to restart the kernel and leave those "zombie" widget models alive in the front-end, causing significant leaks and mess that only a hard browser page refresh can fix.

At the very least, if you do a "Restart Kernel and Clear All Outputs" action, I can see no reason why it should not also close all widgets.

That would certainly be useful for my use case (actually I thought it was the case but apparently not? I'm using JupyterLab 4).

Having a Close all widgets menu item would greatly help too

EDIT: #1866 (comment) would be nice as well.

Or is there anything else I can do already to prevent such leaks in the front-end?

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

Successfully merging a pull request may close this issue.

5 participants