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

Improve notebook cell model lifecycle #13675

Merged
merged 3 commits into from
May 6, 2024
Merged

Conversation

msujew
Copy link
Member

@msujew msujew commented May 2, 2024

What it does

After #13488, I've noticed a few issues in incorrect caching of notebook cells. Mostly related to them staying in storage for too long, thereby overriding new cells. This is mostly related to:

  • Missing calls to $acceptEditorsAndDocumentsDelta when creating new notebook cell editors
  • Dangling monaco-model references after calling createTextModelsForNotebook
  • Dangling monaco-model references after disposing a cell model

How to test

I've been pretty successful in reproducing the behavior on master by:

  1. Opening an existing notebook file
  2. Adding a few new code cells to the start of the file
  3. Closing the notebook file again
  4. Opening it again

On master the new code cells contain content of other, existing code cells (this seems to happen almost at random, though it is consistent). This change should produce empty cells after opening the notebook again.

Furthermore, reviewers should test that cell execution on newly created cells works as expected (to test the newly added call to $acceptEditorsAndDocumentsDelta).

Review checklist

Reminder for reviewers

@msujew msujew added the notebook issues related to notebooks label May 2, 2024
@msujew msujew requested a review from jonah-iden May 2, 2024 15:03
Copy link
Contributor

@jonah-iden jonah-iden left a comment

Choose a reason for hiding this comment

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

Seems to work nicely and looks like the performance improvement is still working.
Not sure if that was introduced through this PR but im always getting 2 of the following Errors when opening a notebook. Regardless of the number of cells:

2024-05-03T07:01:19.789Z root ERROR [hosted-plugin: 33280] Error: MISSING extHostDocument for notebook cell: vscode-notebook-cell:/c%3A/Typefox/Open_Source/theia_test_workspace/notebooks/W39WED_Leon.ipynb#W0sZmlsZQ%3D%3D
    at get apiCell [as apiCell] (C:\Typefox\Open_Source\theia\examples\electron\lib\backend\node_modules_theia_monaco-editor-core_esm_vs_base_common_sync_recursive-packages_core_shared_-54ebfc.js:11427:23)
    at C:\Typefox\Open_Source\theia\examples\electron\lib\backend\node_modules_theia_monaco-editor-core_esm_vs_base_common_sync_recursive-packages_core_shared_-54ebfc.js:11522:51
    at Array.map (<anonymous>)
    at Object.getCells (C:\Typefox\Open_Source\theia\examples\electron\lib\backend\node_modules_theia_monaco-editor-core_esm_vs_base_common_sync_recursive-packages_core_shared_-54ebfc.js:11522:34)
    at sE.analyzeNotebook (C:\Users\jonah\.theia\deployedPlugins\ms-toolsai.jupyter-2023.9.100\extension\out\extension.node.js:24:526631)
    at sE.<anonymous> (C:\Users\jonah\.theia\deployedPlugins\ms-toolsai.jupyter-2023.9.100\extension\out\extension.node.js:24:524879)
    at C:\Typefox\Open_Source\theia\examples\electron\lib\backend\packages_core_lib_common_index_js-node_modules_vscode-languageserver-types_lib_umd_sync_recursive.js:1410:69
    at CallbackList.invoke (C:\Typefox\Open_Source\theia\examples\electron\lib\backend\packages_core_lib_common_index_js-node_modules_vscode-languageserver-types_lib_umd_sync_recursive.js:1416:26)
    at Emitter.fire (C:\Typefox\Open_Source\theia\examples\electron\lib\backend\packages_core_lib_common_index_js-node_modules_vscode-languageserver-types_lib_umd_sync_recursive.js:1531:36)
    at NotebooksExtImpl.$acceptDocumentsAndEditorsDelta (C:\Typefox\Open_Source\theia\examples\electron\lib\backend\node_modules_theia_monaco-editor-core_esm_vs_base_common_sync_recursive-packages_core_shared_-54ebfc.js:12793:55)

@msujew
Copy link
Member Author

msujew commented May 6, 2024

@jonah-iden good catch! I actually already broke this in #13606, but it should be fixed now.

Copy link
Contributor

@jonah-iden jonah-iden left a comment

Choose a reason for hiding this comment

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

Now that the error is fixed looks good to me

@msujew msujew merged commit ef04dc7 into master May 6, 2024
11 of 14 checks passed
@msujew msujew deleted the msujew/fix-notebook-cell-model branch May 6, 2024 11:05
@github-actions github-actions bot added this to the 1.50.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants