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

Notebook exploration #86632

Merged
merged 319 commits into from
Mar 18, 2020
Merged

Notebook exploration #86632

merged 319 commits into from
Mar 18, 2020

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Dec 10, 2019

This PR covers the work behind #84293

Annotaions

Mark Description
[ ] work not started
✔️ work completed
🏃 on-going work

Currently we track all features for a decent generic notebook client implementation in https://github.com/rebornix/notebook-test/blob/master/roadmap.ipynb and this PR tracks the work we have done to prove the ideas. While we continue to checking off the feature list, we also need to move away from workarounds in the code base to ensure that the code is mergeable.

  • List View
    • Optional List Item Height style
    • Willscroll event
    • Dynamic height update
    • isRendering state
    • Backing layer
  • Webview
    • body height/width 100%
    • wheel event handler
    • Event for initial layout (for dimensions info)
  • CodeEditorWidget
    • Remove DOM safely

The ultimate goal is we send PRs to feature owners and get them merged before this PR>

Legacy todo list and notes

Old todo list

The feature list below is now tracked in https://github.com/rebornix/notebook-test/blob/master/roadmap.ipynb .

TOC

  • Loading/Rendering
  • Editing
  • Execution
  • Language Feature
  • Debugging/Search/etc
  • Notebook loading
    • 💡Load from file on disk. Currently we load .ipynb file directly from disk and parse the JSON content. The content might be contributed through API.
    • 💡Virtualization by leveraging List View (We can probably use Tree View to support dynamic loaded outputs)
    • Markdown cell. Currently rendered by internal Markdown engine
    • Code cell. Rendered in full sizeMonaco Editor. Currently we use full size Monaco Editor.
    • Output
      • stdout/stderr. Colorized with ansi colors.
      • image/png
      • svg
      • stream
      • ...
    • 🏃Dynamic height change. Currently we observe size change on output elements and update the list item height cache directly to trigger minimal relayout.
    • isolation for insecure outputs
      • the output from code execution can be anything, depending on how much a frontend can implement. Rendering text/images are okay most of the time but rendering raw html with scripts can pollute the workbench immediately without isolation.
  • Notebook editing
    • Create new code cell
    • 🏃Create new markdown cell
      • Insert editor with markdown language
      • Render preview and adjust list item height on model change
    • Delete cell
    • Save. Currently we can serialize the model behind the notebook and save it to disk directly.
  • Language features
    • ⛑ Syntax highlighting. Hardcoded with python, it should be contributed by notebook/cell metadata.
    • Suggestion
    • ParameterHint/Hover. Currently they are not positioned correctly due to layering.
    • Richer cross cell language support
  • Notebook execution. This requires new API with Notebook/Cell knowledge.
  • Others
    • Debugging
    • Keybindings support in Notebook editor
    • Find/Replace
    • Diff

Notes

Virtualization

To ensure that notebooks' file loading and scrolling is performant (see benchmarks and analysis in rebornix/notebook-test#1), we adopted List View to provide virtualization to cells in a notebook. Every cell in a notebook is rendered as a list item which has dynamic height.

To render a list view with dynamic height items, the List View will do following steps

  • Pre-calculate the size for every item
    • For fixed height list item, get it from the renderer directly
    • For dynamic height item, add it to the container, have it rendered and get its dimension.
    • Then remove the item from the container
  • Rerender
    • Render every list item visible in the viewport with the dimension information calculated in the above step.

The pre-calculate step is costly, especially when the DOM structure is complex, as it adds the item to the DOM tree once and then removes it. For code cells which are rendered as Monaco Editor, the pre-calculate step might take hundreds of milliseconds, which is noticeable when scrolling.

💪 We may want to have a smarter dynamic height calculation in List View.

Output rendering

Outputs for code cell execution come in various formats, including text, errors, png images, svg or even html. Text and PNGs can be rendered in a control way but svg/html contents are more abitrary so we might need to render in sandboxed iframes or webview to avoid them misbehaving.

Code cell language/debugging support

For every visible code cell, we create a text model with preferred language (python at this moment) and attach it to a monaco editor. Extensions will see it as a regular vscode.TextDocument and stored in memory. Extensions which rely on file system might break in this case.

The other catch is monaco editors in the same notebook share the same tab and editor group. How would document based operations work? What should happen when users run Go To Definition?

The same problem applies to Debugging too as currently debuggers work on files/documents, but not fragments.

@davidanthoff
Copy link
Contributor

Am I interpreting this correctly, that you are planning to add support for Jupyter notebooks into the base product? That would be awesome! I'm one of the two folks maintaining the Julia extension for VS Code, and there would be a huge demand for an ability to use Jupyter notebooks that have Julia code inside them.

I'd be happy to be the point person from the Julia side to make sure this works with Julia. I just tried to compile this branch, but it errored. Maybe you could ping me when this branch is in a state where it might make sense to start testing it with Julia? Also feel free to reach out if you have any other questions re Julia and how we could make sure it works well with this work here.

@rebornix
Copy link
Member Author

@davidanthoff sorry that I missed your comment as GitHub somehow hides the comment automatically for me ;(

We are not trying to embed Jupyter notebook into the core. Instead the exploration we are doing here is to see if we can add a general performant and secure notebook rendering in the core and then extensions can contribute their Notebook implementation, thus one extension can implement Jupyter notebook support on top of the work we did here.

there would be a huge demand for an ability to use Jupyter notebooks that have Julia code inside them

AFAIK, to make this happen, you need to implement a Julia kernel for Jupyter notebook first (it seems there is already one listed in https://github.com/jupyter/jupyter/wiki/Jupyter-kernels). Once that happens, you can use any Jupyter frontend to talk to the Julia kernel.

@davidanthoff
Copy link
Contributor

Ah, that also sounds great :) So essentially someone (you guys?) should revive https://marketplace.visualstudio.com/items?itemName=donjayamanne.jupyter once the support for notebook rendering is in the core. Maybe more of a side point, but I think having Jupyter notebook support in the Python extension is really not in the spirit of the Jupyter project and very much not how all the other Jupyter Notebook clients are working right now (JupyterLab, nteract etc.). So I really hope the Jupyter proper extension will come back into existence, so that this feature doesn't live in a language specific extension :)

Yes, Julia has had a Jupyter kernel for ages (in fact the "Ju" part in "Jupyter" stands for Julia). I was really just referring to having Jupyter notebook support that works with the existing Julia kernel inside VS Code, that would be super popular.

'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'hr', 'i', 'img', 'ins', 'kbd', 'li', 'main', 'mark',
'ol', 'p', 'pre', 'section', 'span', 'strike', 'strong', 'sub', 'summary', 'sup', 'table',
'tbody', 'td', 'th', 'thead', 'tr', 'u', 'ul', 'input'
],
Copy link
Member

Choose a reason for hiding this comment

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

This is risky, right? Don't we use this function to also render extension contributed markdown and thereby also allowing them to render html?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the remind. This one is about allowing the core markdown renderer to display checkbox which is sanitized by insane. I'll revert the change for now and see ppl's feedback.

@rebornix rebornix marked this pull request as ready for review March 18, 2020 17:03
@rebornix rebornix merged commit 1c2c8ba into master Mar 18, 2020
@rebornix rebornix deleted the rebornix/notebook branch March 18, 2020 19:06
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants