ipywidgets icon indicating copy to clipboard operation
ipywidgets copied to clipboard

Clarify how to fully clean widget state when updating versions

Open ricklupton opened this issue 7 years ago • 24 comments
trafficstars

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.

ricklupton avatar Dec 10 '17 18:12 ricklupton

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 avatar Dec 11 '17 16:12 jasongrout

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()

https://github.com/jupyter-widgets/ipywidgets/blob/a9c4069570aa92368f590296569e411d9c49e123/ipywidgets/widgets/widget.py#L297

jasongrout avatar Dec 11 '17 16:12 jasongrout

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 avatar Dec 11 '17 17:12 ricklupton

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.

ricklupton avatar Dec 11 '17 17:12 ricklupton

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().

jasongrout avatar Dec 11 '17 18:12 jasongrout

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

ricklupton avatar Dec 19 '17 21:12 ricklupton

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.

jasongrout avatar Jan 18 '18 22:01 jasongrout

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 avatar Jan 20 '18 10:01 SylvainCorlay

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.

SylvainCorlay avatar Jan 20 '18 10:01 SylvainCorlay

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?

ricklupton avatar Jan 24 '18 15:01 ricklupton

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.

vidartf avatar Feb 12 '18 23:02 vidartf

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

ricklupton avatar Feb 13 '18 20:02 ricklupton

@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?

vidartf avatar Feb 14 '18 11:02 vidartf

@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!

ricklupton avatar Feb 14 '18 18:02 ricklupton

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

jasongrout avatar Mar 21 '18 05:03 jasongrout

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 ?

ricklupton avatar Mar 21 '18 22:03 ricklupton

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?

jasongrout avatar Mar 21 '18 22:03 jasongrout

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?

ricklupton avatar Mar 21 '18 22:03 ricklupton

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

jasongrout avatar Mar 21 '18 22:03 jasongrout

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.

ricklupton avatar Mar 21 '18 22:03 ricklupton

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 avatar Mar 22 '18 00:03 jasongrout

Tweaked wording:

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

Thoughts? Perhaps "Save Widgets to Notebook"?

jasongrout avatar Mar 22 '18 01:03 jasongrout

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

ricklupton avatar Mar 22 '18 16:03 ricklupton

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: https://github.com/jupyter-widgets/ipywidgets/issues/1866#issuecomment-359162070 would be nice as well.

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

benbovy avatar Dec 14 '23 11:12 benbovy