Allows lab manager to load state from kernel/notebook #2265

vidartf · 2018-11-15T17:37:31Z

When connecting to a new kernel in lab, this code now:

Loads all live widgets from the kernel.
Loads all stored widgets from the notebook (where the comm id does not overlap with a live one).

This attempts to mirror the logic in the widgetsnbextension code, and mostly the code is a hard copy (not DRY as such, and maintainers should note the possible need to keep these in sync in the future).

vidartf · 2018-11-15T17:38:20Z

One issue for a TODO:

When restarting kernel, widgets are not set as disconnected.

vidartf · 2018-11-16T13:28:07Z

Note, this resolves a TODO on get_model that might not be fully backwards compatible.

…gister the jlab widgets.

… reused, we use the same widget manager. This makes widgets work across multiple views of a notebook, since they all share the same widget manager.

…endering widgets until the appropriate widget manager is set. This allows a notebook to open and start ‘rendering’ widgets as soon as it opens, and the real widget rendering will actually happen asynchronously after the widget manager is created.

jasongrout · 2019-03-05T00:58:54Z

Note, this resolves a TODO on get_model that might not be fully backwards compatible.

Unfortunately, this makes it backwards incompatible, which technically means we should bump the base package up a major version number. It feels like we should have a bigger carrot for a major version bump in the base package.

How about we separate these two changes, so we can merge the jlab manager changes in immediately with essentially no change in base, and we take up fixing the get_model function in the next base major release?

(I'm okay with the typings enhancements you did in base, as well as the function you added, so I imagine this would be a minor base version bump, just not a major one yet.)

jasongrout · 2019-03-05T01:33:01Z

Unfortunately, this makes it backwards incompatible, which technically means we should bump the base package up a major version number.

The reason I think we should have a bigger reason to bump a major version is that a major bump of base requires everyone to release their widgets again (presuming they use the standard ^1 semver for base).

Maybe this conflation points us to having the widget manager base as a separate package from the widgets base package in some future version.

jasongrout · 2019-03-05T07:45:29Z

@vidartf - FYI, I'm working on further enhancements to this PR...

vidartf · 2019-03-05T09:51:17Z

@jasongrout OK, give me a ping when you are satisfied, and I'll have a look.

Move to base manager so it can be reused.

Remaining, know issues: - When restarting kernel, widgets are not set as disconnected. - Maybe a problem of deadlock for interdependent widgets? Maybe it was like that from before?

Calling code should handle the case where get_model returns a rejected promise, so subclasses may still choose to return a rejected promise if the model is not found.

* Convert many functions to use async/await * Rework the restoring promise and initial restore logic

…rate functions.

jasongrout · 2019-03-05T19:06:47Z

I rebased on master to pull in changes needed to work with the current jlab prerelease.

jasongrout · 2019-03-06T06:38:46Z

If the widget save setting is 'false', should we clear the existing widget state in the notebook, or just leave it alone? Or perhaps we should clear it from the model after we load it?

Also, does it seem reasonable to only save live widgets? This may be a good first attempt. This may mean that opening a notebook with saved state, running just a few cells, and saving means that other cells with saved widgets then stop working. Perhaps it's better to somehow save live widgets plus any widgets referenced from a view (hard)? Or perhaps any widgets that have been displayed at least once (again, probably hard)? Or perhaps we just have an option to save all widget state, including unseen and unused widgets (easy, but confusing, I think)?

jasongrout · 2019-03-06T06:44:25Z

And another possible TODO: right now, if we can't find a widget model, we just leave a "Loading widget..." message. Perhaps we should start a timeout when we do that, so that if the model doesn't load after a while (5 seconds?), we just explain there is an error, rather than having the user keep waiting for the widget to load.

…d in 5 seconds.

jasongrout · 2019-03-06T20:45:36Z

And another possible TODO: right now, if we can't find a widget model, we just leave a "Loading widget..." message. Perhaps we should start a timeout when we do that, so that if the model doesn't load after a while (5 seconds?), we just explain there is an error, rather than having the user keep waiting for the widget to load.

I did this in the latest commit.

jasongrout · 2019-03-06T20:47:33Z

Wrapping this up until we can discuss with @vidartf (or whoever else wants to review it). This I think requires a minor version bump in base and a major version bump in jlab manager.

vidartf

A few points to discuss.

vidartf · 2019-03-14T19:39:30Z

packages/jupyterlab-manager/src/plugin.ts

+  // For any widgets that have already been rendered with the placeholder, set
+  // the renderer. This iteration structure is closely tied to the structure of
+  // the notebook widget.
+  nb.widgets.forEach(cell => {


This loop is tied to the DOM, and won't pick up on e.g. "New view for output" that can be restored on layout.

I just pushed a commit that (a) refactors the awkward logic into a self-contained iterator, and (b) attempts to also iterate through linked views, using even more awkward logic that relies on specific jlab details. It's hard for me to test, though, since linked output views aren't restored in the latest jlab release.

(note that both of these awkward iterators are just an interim solution until there might be a more supported clean way of iterating through renderers in jlab itself)

packages/jupyterlab-manager/src/manager.ts

vidartf · 2019-03-15T01:19:31Z

packages/jupyterlab-manager/src/manager.ts

+   * Set the dirty state of the notebook model if applicable.
+   *
+   * TODO: should also set dirty when auto-save state is turned on (easy), and when any
+   * model changes any data (likely hard right now).


Note to self: Just before merging, if this TODO note is still here, it should be replaced by an issue.

packages/jupyterlab-manager/src/manager.ts

vidartf · 2019-03-15T01:46:22Z

This may mean that opening a notebook with saved state, running just a few cells, and saving means that other cells with saved widgets then stop working.

Not really, as widgets in the state will be restored (will become live). Your scenario will only happen if you are working super fast (and/or the restore is super slow). Still, it could leave to some weird behavior.

vidartf · 2019-03-15T01:48:03Z

packages/jupyterlab-manager/src/renderer.ts

+      // error message to the user.
+      setTimeout(() => {
+        if (this._rerenderMimeModel) {
+          this.node.textContent = 'Error displaying widget: model not found';


Maybe we should tweak this error message to indicate that, while no actual error has occurred, getting the widget state is taking a lot longer than expected.

I pushed a commit that tries to make the error messages more intelligent, based on whether the manager has restored its status or not.

…r functions. Also, loop through all output views that have been created.

…tored.

jasongrout · 2019-03-21T18:53:24Z

@vidartf - I think I addressed the issues you raised above. Thoughts now?

jasongrout · 2019-03-21T19:23:55Z

This may mean that opening a notebook with saved state, running just a few cells, and saving means that other cells with saved widgets then stop working.

Not really, as widgets in the state will be restored (will become live). Your scenario will only happen if you are working super fast (and/or the restore is super slow). Still, it could leave to some weird behavior.

I was talking about dead widgets that only live in the notebook metadata, not restored widgets from the kernel.

vidartf · 2019-03-28T16:13:39Z

Not really, as widgets in the state will be restored (will become live).

I was mistaken about this, widgets in the notebook without a kernel equivalent, will not become live (they won't get a comm).

vidartf

I'll add some minor typings, and also have a few minor points that should be checked before merging. @jasongrout Would you mind checking those?

packages/base/src/utils.ts

vidartf · 2019-03-28T16:18:29Z

packages/jupyterlab-manager/src/manager.ts

+  _handleKernelStatusChange(args: Kernel.Status) {
+    switch (args) {
+    case 'connected':
+      // TODO: should we clear away any old widgets before restoring?


Did we answer this? If so, we should change this comment to explain what we chose (and why).

Should we also reset this._restoredStatus here?

Is this the only thing left to resolve here?

I think so. I.e. both the TODO, and reviewing if this._restoredStatus should ever be reset (and if so, when and where).

// TODO: should we clear away any old widgets before restoring?

YES

// TODO: should we clear away any old widgets before restoring?

YES

😏 we were discussing this in person with @jasongrout.

I went ahead and reset the restored status in disconnect. I think it makes most sense to reset it there, since a reconnect will then restore widgets again.

Was this actually done? Since you force pushed, the commit log doesn't show any partial diffs for this, so its super hard to track what was changed.

Sorry, and in general this PR was already hard to track being so long and involved. Are you asking if the TODO was fixed, or if the restored status was reset?

packages/jupyterlab-manager/src/manager.ts

packages/jupyterlab-manager/src/plugin.ts

SylvainCorlay · 2019-04-11T23:02:45Z

packages/jupyterlab-manager/src/manager.ts

@@ -78,7 +98,7 @@ class BackboneViewWrapper extends Widget {
 */
 export
 class WidgetManager extends ManagerBase<Widget> implements IDisposable {
-  constructor(context: DocumentRegistry.IContext<DocumentRegistry.IModel>, rendermime: RenderMimeRegistry) {
+  constructor(context: DocumentRegistry.IContext<INotebookModel>, rendermime: RenderMimeRegistry, settings: WidgetManager.Settings) {


Note for self: support the second argument in Voila's widget manager.

SylvainCorlay · 2019-04-11T23:14:07Z

packages/jupyterlab-manager/src/manager.ts

+  _handleKernelStatusChange(args: Kernel.Status) {
+    switch (args) {
+    case 'connected':
+      // TODO: should we clear away any old widgets before restoring?


// TODO: should we clear away any old widgets before restoring?

YES

SylvainCorlay · 2019-04-11T23:14:42Z

packages/jupyterlab-manager/src/manager.ts

+  _handleKernelStatusChange(args: Kernel.Status) {
+    switch (args) {
+    case 'connected':
+      // TODO: should we clear away any old widgets before restoring?


// TODO: should we clear away any old widgets before restoring?

YES

😏 we were discussing this in person with @jasongrout.

SylvainCorlay · 2019-04-11T23:28:34Z

packages/jupyterlab-manager/src/manager.ts

+  async clear_state(): Promise<void> {
+    await super.clear_state();
+    this._modelsSync = new Map();
+    this.setDirty();


note: clear_state and register_model seem to be overriden just for the _modelsSync and setDirty() settings.

SylvainCorlay · 2019-04-11T23:30:17Z

packages/jupyterlab-manager/src/manager.ts

+    if (modelPromise === undefined) {
+      throw new Error('widget model not found');
+    }
+    return modelPromise;


Note for self: seem to be overriden to make the function trully async and not be able to return undefined.

SylvainCorlay · 2019-04-11T23:35:17Z

packages/jupyterlab-manager/src/manager.ts

+   */
+  async restoreWidgets(notebook: INotebookModel): Promise<void> {
+    await this._loadFromKernel();
+    await this._loadFromNotebook(notebook);


note for self: restoreWidgets should be overridden in voila.

SylvainCorlay · 2019-04-11T23:45:43Z

packages/jupyterlab-manager/src/manager.ts

+    await this._loadFromKernel();
+    await this._loadFromNotebook(notebook);
+    this._restoredStatus = true;
+    this._restored.emit();


Should there be two signals for _restoredFromKernel and _restoredFromNotebookStatus. The former would be set to false in disconnect?

This didn’t get pushed to the PR to be included.

jasongrout · 2019-04-13T04:47:12Z

To answer the conversation above: should we clear away old widgets before restoring? Actually, since we're only storing live widget models right, I think it doesn't hurt to leave old widget models in - we won't get file bloating from disconnected widgets.

vidartf mentioned this pull request Nov 16, 2018

Should Session.kernelChanged trigger on kernel restart? jupyterlab/jupyterlab#5632

Open

vidartf requested a review from jasongrout November 16, 2018 13:25

msuperina mentioned this pull request Feb 24, 2019

Question: create widget view without executing cell code #2323

Open

jasongrout added 4 commits February 27, 2019 14:12

Use the notebook tracker instead of the document widget concept to re…

dfdd011

…gister the jlab widgets.

Simplify the code of the _create_comm method in the jlab widget manager.

92fd495

Cache the widget manager in the jlab context, so that if a context is…

fd0ab5b

… reused, we use the same widget manager. This makes widgets work across multiple views of a notebook, since they all share the same widget manager.

vidartf and others added 14 commits March 5, 2019 08:32

Minor typngs improvement

cf45d67

Cleanup

556d635

Avoid use of private variable

7d32942

Refactor widget restore state filter function

2590745

Move to base manager so it can be reused.

WIP

d516695

Switch get_model to reject promise

9f43826

Finish restoreWidget WIP

6078163

Remaining, know issues: - When restarting kernel, widgets are not set as disconnected. - Maybe a problem of deadlock for interdependent widgets? Maybe it was like that from before?

Rename for readability

73f9efa

Fix mis-typing

e211c18

Have tests reflect new get_model behavior

3c59cf8

Use status change for disconnecting/restoring

5a0601a

Revert backwards-incompatible changes to get_model

e94e5fc

Calling code should handle the case where get_model returns a rejected promise, so subclasses may still choose to return a rejected promise if the model is not found.

Revamp widget state restoration for jlab manager

b2bdfda

* Convert many functions to use async/await * Rework the restoring promise and initial restore logic

Refactor jlab loading from kernel and loading from notebook into sepa…

a0e0c8f

…rate functions.

jasongrout force-pushed the lab-state branch from 8a0e68a to a0e0c8f Compare March 5, 2019 19:06

Merge branch 'jlab-tracker' into lab-state

e6b47d8

Display widget loading error in jlab if the renderer hasn’t rerendere…

9942011

…d in 5 seconds.

jasongrout added this to the Minor release milestone Mar 6, 2019

vidartf commented Mar 15, 2019

View reviewed changes

jasongrout added 2 commits March 21, 2019 10:45

Refactor iterating over all previous renderers into separate generato…

57e5974

…r functions. Also, loop through all output views that have been created.

Make error messages more intelligent, based on if the manager has res…

631c4af

…tored.

vidartf commented Mar 28, 2019

View reviewed changes

Add some typings

cf3c8bc

jasongrout force-pushed the lab-state branch from 0fd2203 to cf3c8bc Compare April 11, 2019 23:58

SylvainCorlay reviewed Apr 12, 2019

View reviewed changes

SylvainCorlay merged commit 1df5b73 into jupyter-widgets:master Apr 12, 2019

vidartf deleted the lab-state branch April 13, 2019 00:39

vidartf mentioned this pull request Apr 13, 2019

Chore: Replace TODO comments with issues #2371

Open

jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Apr 13, 2019

Add disconnect handler as mentioned in jupyter-widgets#2265.

8af293c

This didn’t get pushed to the PR to be included.

jasongrout mentioned this pull request Apr 13, 2019

Add jlab manager disconnect function #2372

Merged

This was referenced Apr 13, 2019

Upgrade jlab support #2373

Merged

save widget state in Jupyter Lab jupyterlab/jupyterlab#5235

Closed

jonmmease mentioned this pull request Apr 16, 2019

Using iplot with ipython.display duplicates widgets in JupyterLab plotly/plotly.py#1516

Closed

maartenbreddels mentioned this pull request May 24, 2019

chore(deps,js): move to jupyterlab-manager 0.40 voila-dashboards/voila#153

Closed

lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 21, 2020

lock bot locked as resolved and limited conversation to collaborators May 21, 2020

Allows lab manager to load state from kernel/notebook #2265

Allows lab manager to load state from kernel/notebook #2265

Conversation

vidartf commented Nov 15, 2018

vidartf commented Nov 15, 2018 • edited Loading

vidartf commented Nov 16, 2018

jasongrout commented Mar 5, 2019

jasongrout commented Mar 5, 2019

jasongrout commented Mar 5, 2019

vidartf commented Mar 5, 2019

jasongrout commented Mar 5, 2019

jasongrout commented Mar 6, 2019

jasongrout commented Mar 6, 2019

jasongrout commented Mar 6, 2019

jasongrout commented Mar 6, 2019

vidartf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidartf commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongrout commented Mar 21, 2019

jasongrout commented Mar 21, 2019

vidartf commented Mar 28, 2019

vidartf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SylvainCorlay Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

jasongrout commented Apr 13, 2019

vidartf commented Nov 15, 2018 •

edited

Loading

SylvainCorlay Apr 11, 2019 •

edited

Loading