Notebook API Support #12442

jonah-iden · 2023-04-21T14:27:36Z

What it does

Closes #8395.
Future Improvements: #12863.

This implements the basics of the notebook api for theia. This includes following features:

opening notebooks in a custom editor widget
editing and saving of notebooks through the new widget
executing cells and notebooks and displaying cell outputs through renderers contributed by extensions
general starting and comunicating with notebook extensions like jupyter

Also this PR includes some prepared functionality not in use yet but needed for implementations later on.

To keep the size of the PR on the smaller size (it's already gigantic as it is), some less important notebook features haven't been implemented yet. Don't expect everything to work perfectly as expected
see #12442 (comment) for information on known issues and missing features.

IP Check Ticket

How to test

Basic Notebook support is given through the builtin ipynb extension. So execute yarn download:plugins once in the project root.
For Cell execution the python and juypter extensions are needed.

Make sure that the workspace you're using for testing is trusted or you have disabled the workspace trust feature. Otherwise, some features, such as notebook kernel selection might not work as expected.

Also here is a notebook for testing W39Mon_Lecture8_DemoNotebook.zip

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

vince-fugnitto · 2023-04-25T14:06:35Z

@jonah-iden do you mind resolving the conflicts so we can continue with a review?

tsmaeder · 2023-05-08T08:19:51Z

@jonah-iden since I was on vacation when this was discussed in the dev call: what's the story with this PR? Are you looking to merge this code or just looking for feedback? What's the plan for the missing pieces like renderers?

jonah-iden · 2023-05-08T08:39:58Z

Hi @tsmaeder , to be honest i'm not sure if it was ever discussed how exactly to proceed with this regarding merging. I disabled most of the stuff through a hardcoded feature flag, so i guess merging would definitely be a good possiblity. The Idea was mostly to start working on this feature to raise interest and maybe secure more financing to actually finish it, or get other parties involved who would like to contribute to this feature. I will continue working on it on my open source time, but that probably won't be a lot.

tsmaeder · 2023-05-08T09:43:04Z

@jonah-iden Thanks for the clarification.

tsmaeder

Hi @jonah-iden. It's awesome to see notebooks finally coming to Theia. I did a first pass over the Code and most of the comments I have are about naming and code structure.

packages/core/src/common/types.ts

packages/monaco/src/browser/monaco-editor-provider.ts

tsmaeder · 2023-05-11T14:04:23Z

packages/notebook/src/browser/contributions/notebook-cell-actions-contribution.ts

+export class NotebookCellActionContribution implements MenuContribution, CommandContribution {
+
+    protected runDeleteAction(notebookModel: NotebookModel, cell: NotebookCellModel): void {
+        notebookModel.removeCell(notebookModel.cells.indexOf(cell), 1);


Why not inline this method?

because the DeleteAction will very likely become more complicated later on. Like having to inform the backend/plugin about the delete

tsmaeder · 2023-05-11T14:04:36Z

packages/notebook/src/browser/contributions/notebook-cell-actions-contribution.ts

+    }
+
+    protected requestCellEdit(notebookModel: NotebookModel, cell: NotebookCellModel): void {
+        cell.requestEdit();


Why not inline this method?

same as for the DeleteAction

packages/notebook/src/browser/notebook-cell-resource-resolver.ts

packages/plugin-ext/src/plugin/notebook/notebooks.ts

tsmaeder · 2023-05-15T08:00:39Z

packages/plugin-ext/src/plugin/notebook/notebooks.ts

+
+    // --- serialize/deserialize
+
+    private _handlePool = 0;


This is not a pool: a pool is something you get entities from and return them to after use. Also, why is this a "handle" and not an "id"? My suggestion would be: "nextId".

Also, my preference would be to use distinct id spaces for the different entities: I find it makes it easier to identify stuff when debugging or analyzing logs.

packages/plugin-ext/src/plugin/notebook/notebooks.ts

tsmaeder · 2023-05-15T08:10:20Z

packages/plugin-ext/src/plugin/plugin-manager.ts

@@ -96,7 +96,9 @@ export class PluginManagerExtImpl implements PluginManagerExt, PluginManager {
        'onFileSystem',
        'onCustomEditor',
        'onStartupFinished',
-        'onAuthenticationRequest'
+        'onAuthenticationRequest',
+        'onNotebook',


Don't we need to add something to plugin-activation-events.ts to make this work?

yeah i have to take a look at that. I was kind of confused by the onNotebook and onNotebookSerializer events.

tsmaeder · 2023-05-15T08:17:22Z

@jonah-iden is there a plan of what still needs to be done to consider the feature complete?

jonah-iden · 2023-05-15T09:03:50Z

Hi, thanks a lot for your first pass. There currently is no list of things still needed for feature completion, but i can take some time tomorrow to create one

tsmaeder · 2023-05-15T12:22:06Z

Testing with the ms jupyter extension gave the following exception, which I think is relevant:

2023-05-12T08:54:31.922Z root ERROR Warning: Each child in a list should have a unique "key" prop.

Check the render method of `NotebookCellListView`. See https://reactjs.org/link/warning-keys for more information.
    at NotebookCellListView (http://localhost:3000/packages_notebook_lib_browser_notebook-editor-widget_js-packages_notebook_lib_browser_noteboo-2c2e12.js:792:9)
    at div
2023-05-12T08:54:32.827Z root INFO [hosted-plugin: 101308] PLUGIN_HOST(101308): PluginManagerExtImpl/loadPlugin(/tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js)
2023-05-12T08:54:33.189Z root ERROR [hosted-plugin: 101308] extension activation failed TypeError: this.vscNotebook.onDidChangeNotebookCellExecutionState is not a function
    at new S (/tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:207218)
    at Object.t.resolveInstance (/tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1297910)
    at /tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1299848
    at /tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1298766
    at Array.map (<anonymous>)
    at /tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1298738
    at Array.map (<anonymous>)
    at Object.t.resolveInstance (/tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1297684)
    at /tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1299848
    at Object.t.resolve (/tmp/vscode-unpacked/ms-toolsai.jupyter-2022.10.110.vsix/extension/out/extension.node.js:17:1300111)

jonah-iden · 2023-05-16T07:04:19Z

Regarding the list of things still needed for full feature completion, as it is in vscode, this is what i could think of right now.
I will update the list if i remember more

Code cell execution
Cell editing is missing alot.
a. the context dependent toolbar. (has been started)
b. adding new cells (has been started)
c. drag and drop of cells
General rendering improvements
a. css fixes (there are some smaller issues here and there)
b. use EditorWidget instead of the more heavyweight MonacoEditor created through createInline
c. NotebookCellListView optimization, so only cells in the current viewport are rendered as real cells.
The cell side toolbar
Finish implementing plugin api methods
Custom Notebook Renderes https://code.visualstudio.com/api/extension-guides/notebook#notebook-renderer

tsmaeder · 2023-05-22T08:44:14Z

@jonah-iden what about custom cell renderers?

jonah-iden · 2023-05-22T08:48:27Z

right thats also a feature still missing. Added it to the list

tsmaeder · 2023-05-22T09:27:55Z

@jonah-iden I was playing around with the Python notebooks extensions. For an MVP, that can run python notebooks, I would think we would need cell execution & custom renderers, right? My guesstimate is that this would take around a third of the effort already spent. Would you agree with this? MVP+ would be with decent editing support. Just trying to find out when we could have something we can present to users.

jonah-iden · 2023-05-22T10:21:13Z

Im not totally sure we need the custom renderers for an MVP but yeah code execution and full Editing support would definitley be needed. I think the code execution could be quite complex so i would say that all 3 would easily need 5PD, if not a little bit more. I think mark estimated initially something like 25-30PD for full support of which we put in about 10. Though of course i have by far not nearly as much expreience as all the core theia team yet. So my 10PD is problably far less than someone with more expreience would have been able to do

msujew · 2023-05-22T11:02:52Z

@tsmaeder @jonah-iden Exactly, my initial estimate for the full feature was something along the lines of 25-30 PD, with 10 having been spent on this MVP. I can imagine execution support being implemented (with custom renderers) in roughly 5 days as well.

packages/notebook/src/browser/service/notebook-service.ts

jonah-iden · 2023-07-17T10:21:36Z

@tsmaeder, @JonasHelming
Hi, this last commit finishes the 2nd spike for Notebook api since we used up all the time.
I updated the How to test section since this now all works with jupyter and the builtin vscode.ipynb extension.
Getting cell execution working was alot more complicated than initially estimated so we sadly didn't get to implementing notebook-renderers. But the basic cell execution framework is there and execution of single cells is working.
This probably needs alot of more cleanup before being able to merge and there are still quite a few issues and bugs in it.
But we at Typefox likely allready secured more founding for countinuing development and i will continue working on this in my open source time.

jonah-iden · 2023-07-17T10:29:07Z

Oh and a small general note for testing. There is currently a problem with the python extension. (see #12708 for more details).
So if anything does not work after some time try checking the {userdir}/.theia/plugin-storage/global-state.json ad delete it if it gets to large

tsmaeder · 2023-07-17T11:38:08Z

@jonah-iden awesome, thanks. My hope is to get this merged as soon as possible, but it's a big one, and world+dog seems to be on vacation, so it might take a while.
After this one is merged, I would suggest that we open an epic with the remaining gaps so that willing parties can pick up tasks when they have capacity.

msujew · 2023-08-25T15:00:34Z

Thanks @vince-fugnitto for looking into it!

If we open the notebook, close and re-open we get a memory leak:

Right, there were in fact multiple memory leaks. Those should hopefully all be fixed now. At least I don't get any event listener warnings anymore when opening and closing the widget multiple times.

The latest commit should fix a lot of other smaller issues in general, such as cell document updates in the plugin host and how the cell execution output is handled in the widget (clearOutput requests from plugins weren't respected, leading to duplicated output).

auto-save does not seem to be respected by the notebook

Looking into this, I have a feeling our saving infrastructure is a bit antiquated. Every model is somehow in charge of managing the files.autoSave by itself, with varying results. See also #12844, #11646 and #9230. I would prefer to fix this in a separate PR. I've created #12863 to track this.

vince-fugnitto · 2023-08-28T14:17:07Z

I've noticed a minor issue when testing, there seems to be a bogus toolbar item (000) which does nothing:

msujew · 2023-08-28T14:47:10Z

@vince-fugnitto Thanks for the heads-up, that seems to be a regression from my previous change. I've fixed it 👍

tsmaeder · 2023-08-28T14:49:14Z

I'm currently in the middle of reading through the code.

tsmaeder · 2023-08-24T12:03:08Z

packages/core/src/browser/saveable.ts

+        await this.delegate?.save(options);
+    }
+
+    revert?(options?: Saveable.RevertOptions): Promise<void>;


Does the Saveable mechanism handle dynamically adding the function members? How would actions like "revert" get enabled when the saveable turns out to support them?

packages/core/src/common/array-utils.ts

packages/core/src/browser/saveable.ts

tsmaeder · 2023-08-24T12:28:13Z

packages/core/src/browser/saveable.ts

+            this.dirty = delegate.dirty;
+            this.onDirtyChangedEmitter.fire();
+        });
+        this.autoSave = delegate.autoSave;


How will users pick up that the autosave setting of the delegate has changed?

tsmaeder · 2023-08-30T08:14:30Z

packages/plugin-ext/src/plugin/notebook/notebooks.ts

+
+    registerNotebookCellStatusBarItemProvider(notebookType: string, provider: theia.NotebookCellStatusBarItemProvider): theia.Disposable {
+
+        const handle = this.currentSerializerHandle++;


Either use different handle counters or rename currentSerializerHandle to somethind generic.

tsmaeder · 2023-08-30T08:15:39Z

packages/plugin-ext/src/plugin/notebook/notebooks.ts

+
+export class NotebooksExtImpl implements NotebooksExt {
+
+    private readonly notebookStatusBarItemProviders = new Map<number, theia.NotebookCellStatusBarItemProvider>();


notebookCellStatusBarItemProviders

tsmaeder · 2023-08-30T08:17:39Z

packages/plugin-ext/src/plugin/notebook/notebooks.ts

+                const revivedUri = URI.fromComponents(uri);
+                const document = this.documents.get(revivedUri.toString());
+
+                if (document) {


Maybe warn if we're removing a non-existing document?

tsmaeder · 2023-08-30T08:21:48Z

packages/plugin-ext/src/plugin/notebook/notebooks.ts

+        }
+    }
+
+    getNotebookDocument(uri: TheiaURI, relaxed: true): NotebookDocument | undefined;


I haven't found a call site that uses the "relaxed" parameter. let's remove it.

tsmaeder · 2023-08-30T08:23:02Z

packages/plugin-ext/src/plugin/notebook/notebooks.ts

+        return result;
+    }
+
+    private createExtHostEditor(document: NotebookDocument, editorId: string, data: NotebookEditorAddData): void {


createNotebookEditor.

tsmaeder · 2023-08-30T08:41:18Z

@jonah-iden @msujew I've finished reading through the code. There are a couple of architectural issues, a bunch of functional ones, but many of the comments are just about naming.
Theia is 2m lines of code that a random person who has not yet started university will look at in five years. Using descriptive and consistent naming allows us to read and understand code we're not familiar with quickly and safely. Ideally, we should look at the name of a thing and correctly assume what it is or does without having to look at its implementation or usage. Bad naming in our code base has literally cost me weeks of time (wondering why nothing made sense), that's why I've become a bit of a stickler for naming.
@vince-fugnitto with the other stuff on my plate, I probably won't be able to do any meaningful testing before tomorrow. If you are reasonably convinced this PR will not break the world, can you give it your approval?
As we discussed in yesterday's community meeting, there will be follow-up work on this topic, but we want this in to allow adopters to exercise the feature.

We've decided to go ahead and address comments later

msujew · 2023-08-30T17:14:35Z

@vince-fugnitto Any chance to get an approval today or tomorrow?

tsmaeder · 2023-08-31T07:29:21Z

When I install the python & jupyter extensions, the python extension seems to be activated even though I don't work with python files at all: in fact, I'm working on the Theia project itself.

2023-08-31T07:20:43.721Z root ERROR [hosted-plugin: 3092] Promise rejection not handled in one second: Error: Client is not running and can't be stopped. It's current state is: startFailed , reason: Error: Client is not running and can't be stopped. It's current state is: startFailed
2023-08-31T07:20:43.721Z root ERROR [hosted-plugin: 3092] With stack trace: Error: Client is not running and can't be stopped. It's current state is: startFailed
    at E.shutdown (c:\Users\thomas\AppData\Local\Temp\vscode-unpacked\ms-python.python-2023.14.0.vsix\extension\out\client\extension.js:2:2063715)
    at E.stop (c:\Users\thomas\AppData\Local\Temp\vscode-unpacked\ms-python.python-2023.14.0.vsix\extension\out\client\extension.js:2:2063296)
    at E.stop (c:\Users\thomas\AppData\Local\Temp\vscode-unpacked\ms-python.python-2023.14.0.vsix\extension\out\client\extension.js:2:2225576)
    at E.doInitialize (c:\Users\thomas\AppData\Local\Temp\vscode-unpacked\ms-python.python-2023.14.0.vsix\extension\out\client\extension.js:2:2063119)
    at async E.start (c:\Users\thomas\AppData\Local\Temp\vscode-unpacked\ms-python.python-2023.14.0.vsix\extension\out\client\extension.js:2:2059685)

tsmaeder · 2023-08-31T07:31:15Z

I also get "python" and "python language server" channels in the "Output" widget. And error messages when starting up about not being able to start up the language server, etc.
@jonah-iden could you have a quick look at this?

jonah-iden · 2023-08-31T07:51:39Z

sure i can take a look later today

jonah-iden · 2023-08-31T09:26:30Z

@tsmaeder
The python extension being activated seems to be a general thing since it has an onDebugDynamicConfigurations activation event which in theia is currently allways being fired on startup. It seems to be fired from the DebugConfigurationSelect component when its mounted. If thats a problem we should probably create a sperate issue for that.
Regarding your error messages for the language server. I'm sadly not able to reproduce them. For me the python extension is working fine and i don't see any errors in the output widget

vince-fugnitto

I'm fine with merging the pull-request even with improvements in #12863 and feedback from Thomas not being implemented for the release since it offers functionality that was previously missing in the framework, and is requested by interested parties 👍

I did not notice any regressions with content outside of notebook editors, and CI successfully passes.

vince-fugnitto · 2023-08-31T12:02:16Z

@msujew @jonah-iden do you want to organize and preserve some commits, or do you want to squash into a single one?

jonah-iden · 2023-08-31T12:13:24Z

I think squashing is fine. There should not be any important information in the single commits

tsmaeder · 2023-08-31T17:03:37Z

🥳 🎉

msujew added notebook issues related to notebooks vscode issues related to VSCode compatibility labels Apr 21, 2023

vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023

jonah-iden force-pushed the msujew/notebooks branch 2 times, most recently from ece1f44 to b0089c9 Compare May 2, 2023 07:00

msujew mentioned this pull request May 9, 2023

[vscode] native notebook support #8395

Closed

tsmaeder previously requested changes May 15, 2023

View reviewed changes

rschnekenbu mentioned this pull request May 15, 2023

[vscode] Support NotebookDocumentWillSaveEvent #12529

Closed

tsmaeder mentioned this pull request May 31, 2023

support vscode-jupyter extension #9867

Closed

tsmaeder reviewed Jun 7, 2023

View reviewed changes

packages/notebook/src/browser/service/notebook-service.ts Outdated Show resolved Hide resolved

jonah-iden force-pushed the msujew/notebooks branch from 9bea3ab to ef22665 Compare June 16, 2023 06:39

jonah-iden force-pushed the msujew/notebooks branch from 4de79bf to a79e28a Compare June 29, 2023 11:23

jonah-iden force-pushed the msujew/notebooks branch from 82bfd0a to 4c5025b Compare July 13, 2023 08:15

Fix notebook header rendering

9add50f

tsmaeder reviewed Aug 30, 2023

View reviewed changes

tsmaeder self-requested a review August 30, 2023 08:42

vince-fugnitto approved these changes Aug 31, 2023

View reviewed changes

msujew merged commit 97fe17e into eclipse-theia:master Aug 31, 2023

vince-fugnitto added this to the 1.41.0 milestone Aug 31, 2023

jonah-iden mentioned this pull request Sep 15, 2023

Notebook API post review improvements #12910

Merged

1 task

jonah-iden mentioned this pull request Oct 20, 2023

notebook api review comments batch 2 #13012

Merged

1 task

This was referenced Jan 31, 2024

Improves style and titles of toolbar items in notebooks. #13334

Merged

Improved notebook scrolling behaviour #13338

Merged

jbicker mentioned this pull request Feb 28, 2024

Fixed scroll behaviour of Notebooks #13430

Merged

1 task


		registerNotebookCellStatusBarItemProvider(notebookType: string, provider: theia.NotebookCellStatusBarItemProvider): theia.Disposable {

		const handle = this.currentSerializerHandle++;


		export class NotebooksExtImpl implements NotebooksExt {

		private readonly notebookStatusBarItemProviders = new Map<number, theia.NotebookCellStatusBarItemProvider>();

Notebook API Support #12442

Notebook API Support #12442

Conversation

jonah-iden commented Apr 21, 2023 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto commented Apr 25, 2023

tsmaeder commented May 8, 2023

jonah-iden commented May 8, 2023 • edited Loading

tsmaeder commented May 8, 2023

tsmaeder 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

tsmaeder commented May 15, 2023

jonah-iden commented May 15, 2023 • edited Loading

tsmaeder commented May 15, 2023

jonah-iden commented May 16, 2023 • edited Loading

tsmaeder commented May 22, 2023

jonah-iden commented May 22, 2023

tsmaeder commented May 22, 2023

jonah-iden commented May 22, 2023

msujew commented May 22, 2023

jonah-iden commented Jul 17, 2023 • edited Loading

jonah-iden commented Jul 17, 2023

tsmaeder commented Jul 17, 2023

msujew commented Aug 25, 2023 • edited Loading

vince-fugnitto commented Aug 28, 2023

msujew commented Aug 28, 2023

tsmaeder commented Aug 28, 2023

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

tsmaeder commented Aug 30, 2023

msujew commented Aug 30, 2023 • edited Loading

tsmaeder commented Aug 31, 2023

tsmaeder commented Aug 31, 2023 • edited Loading

jonah-iden commented Aug 31, 2023

jonah-iden commented Aug 31, 2023

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Aug 31, 2023 • edited Loading

jonah-iden commented Aug 31, 2023

tsmaeder commented Aug 31, 2023

jonah-iden commented Apr 21, 2023 •

edited by vince-fugnitto

Loading

jonah-iden commented May 8, 2023 •

edited

Loading

jonah-iden commented May 15, 2023 •

edited

Loading

jonah-iden commented May 16, 2023 •

edited

Loading

jonah-iden commented Jul 17, 2023 •

edited

Loading

msujew commented Aug 25, 2023 •

edited

Loading

msujew commented Aug 30, 2023 •

edited

Loading

tsmaeder commented Aug 31, 2023 •

edited

Loading

vince-fugnitto commented Aug 31, 2023 •

edited

Loading