Experiment: move ipywidgets into notebooks

[rebornix]

@DonJayamanne when we were exploring the separation of NotebookController and KernelExecution, we thought the ipywidgets is also tighted/coupled with NotebookController or Kernel internals so we can't really move it around. I experimented with moving it to notebooks and surprisingly, it is pretty clean:

ipywidgets extension side code is only registered/used by notebookIPyWidgetCoordinator, which is responsible for transferring messages between kernels and vscode.NotebookEditors. IW doesn't depend on this, no other modules need to
be aware of ipywidgets. It looks reasonable to me that we have it in notebooks then both Debugging and IPyWidgets are very similar: handling messages from kernels and convert the messages to data VS Code requires.

Also after moving it out and looking at the code left in Kernels again, I found it closer to what we imagine what the Kernel component offers

• Execution (kernels/execution)
• Launcher (kernels/jupyter, kernels/raw)
• Installer (kernels/installer)

Rest are common/error/telemetry helpers. The only two that are under discussion are

• port. From my understanding, it tells VS Code whether the port opened for jupyter server can be forwarded. It is integration code with VS Code (mainly for remote) so I wouldn't be surprised if it's moved out and into standalone
• variable. This seems necessary as execution seems to depend on it. Totally fine if we have to keep it in kernels.

@DonJayamanne let me know what you think and we can sync next week, cheers!

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
• Title summarizes what is changing.
• Has a news entry file (remember to thank yourself!).
• Appropriate comments and documentation strings in the code.
• Has sufficient logging.
• Has telemetry for feature-requests.
• Unit tests & system/integration tests are added/updated.
• Test plan is updated as appropriate.
• package-lock.json has been regenerated by running npm install (if dependencies have changed).

[codecov-commenter]

Codecov Report

Merging #10903 (90d6008) into main (545c0a7) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##            main   #10903   +/-   ##
======================================
- Coverage     63%      63%   -1%     
======================================
  Files        487      482    -5     
  Lines      33857    33822   -35     
  Branches    5514     5514           
======================================
- Hits       21503    21455   -48     
- Misses     10318    10331   +13     
  Partials    2036     2036           

Impacted Files	Coverage Δ
src/kernels/serviceRegistry.node.ts	94% <ø> (-1%)	⬇️
src/kernels/types.ts	100% <ø> (ø)
src/messageTypes.ts	100% <ø> (ø)
src/extension.node.ts	76% <100%> (ø)
...c/kernels/execution/cellExecutionMessageHandler.ts	77% <100%> (-1%)	⬇️
...ebooks/controllers/notebookIPyWidgetCoordinator.ts	88% <100%> (ø)
src/notebooks/controllers/serviceRegistry.node.ts	100% <100%> (ø)
src/notebooks/serviceRegistry.node.ts	100% <100%> (ø)
src/kernels/common/chainingExecuteRequester.ts	66% <0%> (-17%)	⬇️
src/kernels/execution/cellExecution.ts	72% <0%> (-8%)	⬇️
... and 34 more

[rebornix]

Regarding whether a kernel message handling component should be part of the kernels component, the general rule of thumb is whether this component has side effects. ipywidgets and debugging are only transferring messages but how they handle them will not affect the kernel session, however we have NotebookController deep in the kernels component as the kernel session lifecycle is tight together with a notebook controller (restart, interrupt, etc)

[rchiodo]

be aware of ipywidgets. It looks reasonable to me that we have it in notebooks then both Debugging and IPyWidgets are very similar: handling messages from kernels and convert the messages to data VS Code requires.

I think I disagree with this point. IPyWidgets has nothing to do with VS code notebooks. It should be necessary for all kernels and not just ones talking to VS code.

If there's a class inside the ipywidgets stuff that does know about notebooks, that part should move to the notebooks folder, but otherwise all of these classes are notebook independent.

[rebornix]

I think I disagree with this point. IPyWidgets has nothing to do with VS code notebooks. It should be necessary for all kernels and not just ones talking to VS code.

@rchiodo it does need to know about VS Code Notebooks. The reason we need a IPyWidgets integration/support is we need to offer the bridge between the output renderer side and the underlining Jupyter kernel. The messages are transferred through
NotebookController and we support multiple NotebookEditor sharing the same Jupyter kernel session.

For short, IPyWidgets output rendering has nothing to do with VS code notebooks but supporting it in VS Code does (before we support ipywidget talks to the kernel directly)

[rchiodo]

The messages are transferred through
NotebookController and we support multiple NotebookEditor sharing the same Jupyter kernel session.

I feel like this part could be abstracted. 3rd party kernels might also want to support ipywidgets. They'd have to provide their own message passing but would otherwise load the ipywidget render code into their webview.

[rebornix]

I feel like this part could be abstracted. 3rd party kernels might also want to support ipywidgets

If 3rd party kernels want to support ipywidgets, the integration code we have now can not be reused since it's tied to NotebookDocument (to support multi notebook editors), while 3rd party kernels now have no knowledge of NotebookEditor/Document. At this moment, I don't know how much logic can be reused as our code is handling messaging between kernel and multiple notebook editor, while 3rd party kernel users might be loading ipywidgets in their Web UI (e.g., Data Wrangler), they need to do their own integration. We are not offering:

• 3rd party extension creates their own kernel
• We load their kernel in Notebook Editor and support ipywidgets

We only create kernels for them, and they use the kernel with their components. Same as Debugging, we are not making debugging generic and offering debugging support for 3rd party kernels.

Note that this PR doesn't move the ipywidgets messaging integration into the notebooks component, it already exists https://github.com/microsoft/vscode-jupyter/blob/main/src/notebooks/controllers/notebookIPyWidgetCoordinator.ts , the coordinator does what's described above and here I moved the messaging handling part next to it.

[rebornix]

Had offline discussions with @rchiodo

@rchiodo: I feel like this part could be abstracted

This is referring to ipywidget message handling part, not the ipywidget source loading and notebook editor/controller coordination. 3rd party extensions like Data Wrangler have requests to share this piece of functionality, but to support this, the messaging handling component would need to support 3rd party kernels. Currently the messaging handling only supports first party kernels which are always attached to a NotebookDocument.

As suggested by @rchiodo , we have separated ipywidgets/message and ipywidgets/scriptSourceProvider. The latter is NotebookEditor specific and the former has the potential to be generic and refactored if we see more interest in it.