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

Feature: always save state of widgets, and only those required #3114

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maartenbreddels
Copy link
Member

The default is now changed to save widgets state in the notebook, but to avoid
it becoming too large, we only save those that are needed to display
the visible ones (and thus find all widgets that are connected).

Closed #3108

I've left open the option for jupyter lab to change the default and not store the state. And left in the API a way to store the while widget state (that could be triggered by a custom command).

The default is to save widgets state in the notebook, but to avoid
it being too large, we only save those that are needed to display
the visible ones (and thus find all widgets that are connected).
@maartenbreddels maartenbreddels force-pushed the feat_store_only_visible_widgets branch from 811d2d4 to 9a71668 Compare February 11, 2021 11:43
@maartenbreddels
Copy link
Member Author

Example:

import ipywidgets as widgets
no_connection = widgets.HTML(value="Hi")
ft = widgets.FloatText(value=2.3)
fs = widgets.FloatSlider(value=42)
widgets.jslink((fs, 'value'), (ft, 'value'))
fs

After running this cell, the save code will find that the fs widget needs to be exported, and it will find its children (Layout and Style). It will also find that a Link widget refers to it, which also requires the ft widget.

The no_connection widget is an island, so it's not included.

Also, running a cell with this code 100 times, will only save the state in the notebook for the 7 widgets required to render the single output (not 700 widgets, as before).

Copy link
Contributor

@eteq eteq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a big fan of this change! (@maartenbreddels knows this since I suggested something in this direction elsewhere). I do, though, think it might be good to maintain a way to save without widget state (i.e. the "old way"). I don't think it should be default, but it should at least be possible. See my inline comment for a concrete approach to that.

'save-clear-widgets',
'widgets'
);
this.notebook.events.on('before_save.Notebook', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how much cleaner this is... but I wonder if this suggests the current save 'save-with-widgets' should be replaced with a 'save-without-widgets' option? Or alternatively, just maintain 'save-clear-widgets' and bind that to the old menu item (line 288 before this PR).

That way the default would be to save (which I think is more intuitive), but there's still a way to save without the widget state for the cases where the widget really needs to be saved that way, and an advanced user can rebind keys to do that etc.

@vidartf
Copy link
Member

vidartf commented Feb 19, 2021

Saving only the required state is easy to say yes to compared to changing the default of whether to save the state or not. If we turn it on by default, then maybe nbstripout/jupytext etc will need to include a filter for removing widget state if they do not already have it? Given the size that widget state can grow to (both in terms of bytes and % of document size when viewing the raw JSON), it really feels like it ought to be a opt-in.

@maartenbreddels
Copy link
Member Author

Maybe this is a good solution

  • By default we store the widget metadata as suggested in this PR. (setting "store": "visible")
  • We have an option to fully opt-out, which gets stored in the notebook metadata. I think this makes sense because it's particular notebooks that generate a lot of widget data, so the setting belongs to the notebook itself.(setting "store": "none")
  • We have an option to save all widgets, to get back the old behaviour, this also gets stored in the notebook metadata. (setting "store": "all")

The setting for the last 2 items can be stored under metadata/widgets/store as follows:

"metadata": {
  "kernelspec": {
   .....
  },
  "language_info": {

  "widgets": {
   "store": "visible/none/all",
   "application/vnd.jupyter.widget-state+json": {
    "state": {
     ....
   }
}

@willingc
Copy link
Contributor

willingc commented Mar 4, 2021

FYI @MSeal

@maartenbreddels
Copy link
Member Author

Addressing the comments by @eteq and the concerns for changing the default, I changed the menu to have radio buttons, so a user can choose:
image

Which by default will be to not store the widgets (although I think we should change the default to 'displayed').

Seeing what this looks like in a screencapture, showing the notebook JSON content:
widget-state-3-options

@maartenbreddels
Copy link
Member Author

Also lab support is, instead of having a global setting, it is a per notebook option.
image

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

Successfully merging this pull request may close these issues.

We should save the (minimal) widget state in the notebook file by default
4 participants