Add support for widgets in JupyterLab code consoles

[jtpio]

trafficstars

Fixes #2370

This mimics the same approach as for the notebooks for now, but it might be possible to have both the consoles and notebooks share more logic.

[jasongrout]

Awesome!

Just curious, if a notebook and console share a kernel, do the widgets stay in sync?

[jtpio]

Just curious, if a notebook and console share a kernel, do the widgets stay in sync?

Not for now:

[jtpio]

It should be possible to make it work by changing the way widget managers are stored here:

https://github.com/jupyter-widgets/ipywidgets/blob/51322341d4f6d88449f9dbf6d538c60de6d23543/packages/jupyterlab-manager/src/plugin.ts#L112

https://github.com/jupyter-widgets/ipywidgets/blob/51322341d4f6d88449f9dbf6d538c60de6d23543/packages/jupyterlab-manager/src/plugin.ts#L323-L330

So that instead of a DocumentRegistry.Context it would be something specific to a kernel (or something common to both a notebook and a code console).

[jtpio]

It should be possible to make it work by changing the way widget managers are stored here:

Trying that out locally. for example by using the session context path (shared between the notebook and the console):

[jasongrout]

Nice! And it still works between two views of a notebook, or two views of an output?

[jtpio]

In that case yes, as all the views share the same WidgetManager instance:

for example by using the session context path

But using the path is a bit too brittle. Maybe there should be a SessionContextManager or similar that could be leveraged for both the consoles and notebooks main area widgets. Or checking whether a manager already exists for a notebook when creating a new console.

[jasongrout]

But using the path is a bit too brittle

Agreed. I haven't checked, but is there any object that the two KernelConnections share? Some reference to the underlying kernel representation, perhaps?

[jasongrout]

Or checking whether a manager already exists for a notebook when creating a new console.

I'd rather avoid having the console know too much about the notebook, and vice versa, as hard-coding these checks doesn't scale to other extensions as well.

[tacaswell]

:peanuts: gallery here: I am very excited about the PR and wanted exactly this with ipympl figures today.

[caenrigen]

Great PR, looking forward to this feature as well!! Kind regards

[jtpio]

Rebased so this branch can be tested on Binder:

[jtpio]

Just rebased one onto the latest main.

Trying that out locally. for example by using the session context path (shared between the notebook and the console):

Here is a diff to use the session context path:

diff --git a/jupyterlab_widgets/package.json b/jupyterlab_widgets/package.json
index 9f22f8cb..e4095418 100644
--- a/jupyterlab_widgets/package.json
+++ b/jupyterlab_widgets/package.json
@@ -68,7 +68,6 @@
     "@lumino/coreutils": "^1.3.0",
     "@lumino/disposable": "^1.1.1",
     "@lumino/messaging": "^1.2.1",
-    "@lumino/properties": "^1.1.0",
     "@lumino/signaling": "^1.2.0",
     "@lumino/widgets": "^1.3.0",
     "@types/backbone": "1.4.1",
diff --git a/jupyterlab_widgets/src/plugin.ts b/jupyterlab_widgets/src/plugin.ts
index 10fafc8d..fe5bb93b 100644
--- a/jupyterlab_widgets/src/plugin.ts
+++ b/jupyterlab_widgets/src/plugin.ts
@@ -33,8 +33,6 @@ import { toArray, filter } from '@lumino/algorithm';

 import { DisposableDelegate } from '@lumino/disposable';

-import { AttachedProperty } from '@lumino/properties';
-
 import { WidgetRenderer } from './renderer';

 import {
@@ -89,8 +87,7 @@ function* consoleWidgetRenderers(
 ): Generator<WidgetRenderer, void, unknown> {
   for (const cell of toArray(console.cells)) {
     if (cell.model.type === 'code') {
-      for (const codecell of ((cell as unknown) as CodeCell).outputArea
-        .widgets) {
+      for (const codecell of (cell as unknown as CodeCell).outputArea.widgets) {
         for (const output of toArray(codecell.children())) {
           if (output instanceof WidgetRenderer) {
             yield output;
@@ -136,11 +133,11 @@ export function registerWidgetManager(
   rendermime: IRenderMimeRegistry,
   renderers: IterableIterator<WidgetRenderer>
 ): DisposableDelegate {
-  let wManager = Private.widgetManagerProperty.get(context);
+  let wManager = Private.widgetManagerProperty.get(context.sessionContext.path);
   if (!wManager) {
     wManager = new WidgetManager(context, rendermime, SETTINGS);
     WIDGET_REGISTRY.forEach((data) => wManager!.register(data));
-    Private.widgetManagerProperty.set(context, wManager);
+    Private.widgetManagerProperty.set(context.sessionContext.path, wManager);
   }

   for (const r of renderers) {
@@ -172,14 +169,14 @@ export function registerConsoleWidgetManager(
   rendermime: IRenderMimeRegistry,
   renderers: IterableIterator<WidgetRenderer>
 ): DisposableDelegate {
-  let wManager = Private.widgetManagerProperty.get(console);
+  let wManager = Private.widgetManagerProperty.get(console.sessionContext.path);
   if (!wManager) {
     wManager = new KernelWidgetManager(
       console.sessionContext.session?.kernel!,
       rendermime
     );
     WIDGET_REGISTRY.forEach((data) => wManager!.register(data));
-    Private.widgetManagerProperty.set(console, wManager);
+    Private.widgetManagerProperty.set(console.sessionContext.path, wManager);
   }

   for (const r of renderers) {
@@ -249,7 +246,9 @@ function activateWidgetExtension(
       return;
     }

-    const wManager = Private.widgetManagerProperty.get(nb.context);
+    const wManager = Private.widgetManagerProperty.get(
+      nb.context.sessionContext.path
+    );
     if (wManager) {
       wManager.onUnhandledIOPubMessage.connect(
         (
@@ -414,12 +413,8 @@ namespace Private {
   /**
    * A private attached property for a widget manager.
    */
-  export const widgetManagerProperty = new AttachedProperty<
-    DocumentRegistry.Context | CodeConsole,
+  export const widgetManagerProperty = new Map<
+    string,
     WidgetManager | KernelWidgetManager | undefined
-  >({
-    name: 'widgetManager',
-    create: (owner: DocumentRegistry.Context | CodeConsole): undefined =>
-      undefined,
-  });
+  >();
 }
diff --git a/yarn.lock b/yarn.lock
index bae0d729..49a69346 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1551,7 +1551,7 @@
     "@lumino/disposable" "^1.7.0"
     "@lumino/signaling" "^1.7.0"

-"@lumino/properties@^1.1.0", "@lumino/properties@^1.2.3", "@lumino/properties@^1.5.0":
+"@lumino/properties@^1.2.3", "@lumino/properties@^1.5.0":
   version "1.5.0"
   resolved "https://registry.npmjs.org/@lumino/properties/-/properties-1.5.0.tgz#7e8638e84c51bb110c5a69f91ca8b0e40b2c3fca"
   integrity sha512-YqpJE6/1Wkjrie0E+ypu+yzd55B5RlvKYMnQs3Ox+SrJsnNBhA6Oj44EhVf8SUTuHgn1t/mm+LvbswKN5RM4+g==

cc @trungleduc who was interested in this

[trungleduc]

Thank @jtpio, i'm trying to use kernel id from session context as map keys with handler for kernelChanged event. It works but is it a good approach?

[jtpio]

Thanks @trungleduc for looking into it.

kernel id from session context

Sounds like this could work? How does it behave with kernel restarts?

[trungleduc]

It works pretty well with kernel restart and kernel change events. I stored current id of kernel so inside the kernelChanged event handler i can update the map with new kernel id.

    sessionContext.kernelChanged.connect((_, args) => {
      const { newValue } = args;
      if (newValue) {
        const newKernelId = newValue.id;
        const oldwManager = Private.widgetManagerProperty.get(currentOwner);

        if (oldwManager) {
          Private.widgetManagerProperty.delete(currentOwner);
          Private.widgetManagerProperty.set(newKernelId, oldwManager);
        }
        currentOwner = newKernelId;
      }
    });

[jtpio]

NIce, thanks @trungleduc for sharing.

Maybe you would like to open a PR with your additional commits, so it's easier to check the diff and try it on Binder?

[trungleduc]

PR opened on jtpio/ipywidgets#2

[jtpio]

Thanks @trungleduc!

I just pushed a change to fix the integrity check.

[jtpio]

For those who would like to test this PR on Binder:

https://mybinder.org/v2/gh/jtpio/ipywidgets/code-consoles-widgets?urlpath=lab

https://user-images.githubusercontent.com/591645/126165203-2af518dd-f296-4016-a458-657b5bc45683.mp4

Marking as ready for review, so other folks can start testing the feature and reviewing the code. Thanks!

[jtpio]

Adding to the 8.0 milestone for consideration, since this is changing a couple of interfaces.

[SylvainCorlay]

This is great! Many thanks to @jtpio and @trungleduc!

I can't wait to see this land in the next 8.0 pre-release.

[jasongrout]

I've been going over this code and thinking about this, and there's something I think this effort is exposing about limitations in how the current lab widget manager is written. This approach (properly, I think) tries to push the notion of a widget manager down to the kernel level (i.e., the widget manager is "owned" by the kernel id, for example). However, the notebook widget manager really lives above that, at the session context level (e.g., a notebook widget manager can be "owned" by several different kernel ids over its lifetime, and right now there really isn't a provision for a notebook widget manager that doesn't happen to be associated with a kernel, as Jeremy points out above when talking about the assertion operator.

This makes me think that perhaps we should restructure the code so that we have one concept of a widget manager that is tied to a kernel, and use composition to interface with the notebook rather than inheritance. In other words, we have a layer on top of the kernel widget manager that manages notebook state and interfaces with the (kernel) widget manager to deal with saving and restoring state from a notebook. In other words, the object at the notebook level HAS a kernel widget manager (not IS a widget manager), and the kernel widget manager can be swapped out or created as needed when the kernel changes. This way consoles and notebooks can cache and share kernel widget managers freely on equal footing. Still thinking through it, but this seems to address this weird disconnect we have between notebook widget managers and kernel widget managers.

[jasongrout]

Here are some more thoughts about how we can structure the widget managers, taking into account the extra state that the notebook introduces. Up to this point, we've conflated the widget state in a notebook that was tied to the kernel with the widget state that was saved in the notebook to activate and display the disconnected widgets. This approach tries to separate those two kinds of widget state so that the active widget state can be shared with other components.

• We have a kernel widget manager that maintains state for widgets currently connected to a live kernel. The lifecycle of this widget manager is tied to the kernel lifecycle. These widget managers are identified by the kernel id, maintain a kernel connection (not sure if they should have their own kernel connection, or if they can share someone else's kernel connection), and can be shared with anything that is connected to that kernel. We keep a map of these, like this PR introduced, keyed on kernel id.
• We have a notebook "widget manager" (not sure if we should call it that, given the confusion it might introduce about lifecycles, etc.), with lifecycle tied to the notebook widget. This maintains a reference to the current kernel's kernel widget manager, but additionally has a cache of disconnected widgets. The primary purpose for having an object tied to the notebook lifecycle is to maintain state for widgets that are no longer associated with the active kernel (either to store their state in the notebook, or retrieve their state from the notebook, so that disconnected widgets in the notebook can be rendered). So the notebook metadata can populate this disconnected widget cache, and we can also save this disconnected widget state back to the notebook. When an active kernel widget manager for a notebook is no longer relevant (e.g., if the notebook switches kernels so it is no longer connected to that kernel), we copy the state from the active kernel widget manager to this disconnected widget cache. The total widget state available to the notebook is the union of these two states (in other words, when trying to render or save a widget, we first try to get the state from the currently active state, and then try the disconnected state).

Thoughts?

[martinRenou]

Looks good! This is really exciting, especially in combination with your recent work on replite

@jasongrout this PR is already a good improvement, maybe we could iterate later (in separate PRs) on making this more generic?

I don't want to be asking too much, but it would be amazing if this could be included in 8.0.

[martinRenou]

This seems to need a rebase though

[jtpio]

Thanks @martinRenou.

Starting the rebase now.

[jtpio]

Arf looks like the rebase and auto yarn resolve brings some new dependencies and typing conflicts:

Will get back to it later unless someone wants to give it a try before.

[jtpio]

For reference this means widgets also show up in the RetroLab (future Notebook v7) code consoles:

[AckslD]

I wanted to give this PR a try to see if I can help with testing, rebasing or anything. However when I do

pip install jupyterlab
./dev-install.sh

in a fresh virtualenv I still get the "Loading widget..." when creating eg an IntSlider. Are there any additional steps to make this work or has there been changes in eg jupyterlab what requires updates here?

[davidbrochart]

This PR is probably a big improvement for lots of reasons, but I have the feeling that consoles should not be a different beast than notebooks anyway. They should be a "restricted notebook", where one can only append new cells and not be able to change previous cells. If we based consoles on notebooks, then they would support widgets out of the box, right?