`ruff server`: Support Jupyter Notebook (`*.ipynb`) files by snowsignal · Pull Request #11206 · astral-sh/ruff

snowsignal · 2024-04-29T17:49:46Z

Summary

ruff server now supports *.ipynb (aka Jupyter Notebook) files. Extensive internal changes have been made to facilitate this, which I've done some work to contextualize with documentation and an pre-review that highlights notable sections of the code.

*.ipynb cells should behave similarly to *.py documents, with one major exception. The format command ruff.applyFormat will only apply to the currently selected notebook cell - if you want to format an entire notebook document, use Format Notebook from the VS Code context menu.

Test Plan

The VS Code extension does not yet have Jupyter Notebook support enabled, so you'll first need to enable it manually. To do this, checkout the pre-release branch and modify src/common/server.ts as follows:

Before:

After:

I recommend testing this PR with large, complicated notebook files. I used notebook files from this popular repository in my preliminary testing.

The main thing to test is ensuring that notebook cells behave the same as Python documents, besides the aforementioned issue with ruff.applyFormat. You should also test adding and deleting cells (in particular, deleting all the code cells and ensure that doesn't break anything), changing the kind of a cell (i.e. from markup -> code or vice versa), and creating a new notebook file from scratch. Finally, you should also test that source actions work as expected (and across the entire notebook).

Note: ruff.applyAutofix and ruff.applyOrganizeImports are currently broken for notebook files, and I suspect it has something to do with #11248. Once this is fixed, I will update the test plan accordingly.

github-actions · 2024-04-29T18:07:29Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

snowsignal

I've left some comments to add context for a few parts of the code and also to remind myself to refactor a few things before we merge 🙂

dhruvmanila

I've provided some initial comments and I plan to look at other changes soon. It's quite a big PR :)

It would be helpful if you could briefly enumerate all the changes made in the PR description. I'd especially find the list of refactoring useful. It would be also useful for posterity to provide any context on the reasoning behind the implementation and if there were any other solutions that you thought of but didn't implement it.

Can you also update the test plan with all the cases that you've tested this against? Ideally it could contain the VS Code settings used and the action performed (command, code action, etc.). There are so many VS Code settings, commands and code actions for Notebook 😅

dhruvmanila · 2024-05-15T13:44:09Z

+                        types::NotebookDocumentFilter::ByType {
+                            notebook_type: "jupyter-notebook".to_string(),
+                            scheme: Some("file".to_string()),
+                            pattern: None,
+                        },


Is this specific to VS Code? I'm referring to the "jupyter-notebook" type. For reference, we don't use any notebook document filter in ruff-lsp:

https://github.com/astral-sh/ruff-lsp/blob/b8a48009f35756dc51268beb6bbc39c21a1e3a46/ruff_lsp/server.py#L129-L140

This asks to only send the request to sync the Python code cells as we aren't interested in any kind of cell.

I got this from the LSP specification:

I assume this would be the same across editors.

I see. I'm not sure where does the jupyter-notebook type comes from.

I assume this would be the same across editors.

I think it depends - If it is set by VS Code, then I don't think it would be same.

I don't think we need to worry about this now but, just to be on the safer side, I'd probably keep this same as ruff-lsp.

Does this work on unsaved Notebook files? Like, a new notebook file is opened in VS Code but it doesn't exists on disk yet.

Does this work on unsaved Notebook files? Like, a new notebook file is opened in VS Code but it doesn't exists on disk yet.

This does fail - I think it might be because an unsaved URL isn't a valid file path.

Hmmm.... it seems like, in general, that the server doesn't support files created from Ctrl+N in VS Code if they aren't saved to the file system. I'll create a follow-up issue for this.

This does fail, but only because new notebook cell url gets passed into didOpenTextDocument, which isn't expected. Do we need to support 'unattached' notebook cells?

I just checked, we do support it. It's probably because we never touch the filesystem in ruff-lsp and always work with stdin with ruff command.

…ses cell boundaries

…#11492) ## Summary Recent changes made in the [Jupyter Notebook feature PR](#11206) caused automatic configuration reloading to stop working. This was because we would check for paths to reload using the changed path, when we should have been using the parent path of the changed path (to get the directory it was changed in). Additionally, this PR fixes an issue where `ruff.toml` and `.ruff.toml` files were not being automatically reloaded. Finally, this PR improves configuration reloading by actively publishing diagnostics for notebook documents (which won't be affected by the workspace refresh since they don't use pull diagnostics). It will also publish diagnostics for text documents if pull diagnostics aren't supported. ## Test Plan To test this, open an existing configuration file in a codebase, and make modifications that will affect one or more open Python / Jupyter Notebook files. You should observe that the diagnostics for both kinds of files update automatically when the file changes are saved. Here's a test video showing what a successful test should look like: https://github.com/astral-sh/ruff/assets/19577865/7172b598-d6de-4965-b33c-6cb8b911ef6c

## Summary This PR fixes a bug where the server wouldn't retain the cell content in case of a reorder change request. As mentioned in #12573 (comment), this change request is modeled as (a) remove these cell URIs and (b) add these cell URIs. The cell content isn't provided. But, the way we've modeled the `NotebookCell` (it contains the underlying `TextDocument`), we need to keep track of the deleted cells to get the content. This is not an ideal solution and a better long term solution would be to model it as per the spec but that is a big structural change and will affect multiple parts of the server. Modeling as per the spec would also avoid bugs like #11864. For context, that model would add complexity per #11206 (comment). fixes: #12573 ## Test Plan This video shows the before and after the bug is fixed: https://github.com/user-attachments/assets/2fcad4b5-f9af-4776-8640-4cd1fa16e325

snowsignal added the server Related to the LSP server label Apr 29, 2024

snowsignal added this to the Ruff Server: Beta milestone Apr 29, 2024

snowsignal force-pushed the jupyter-notebook branch 5 times, most recently from 3a721e2 to ef8727f Compare May 9, 2024 09:02

snowsignal force-pushed the jupyter-notebook branch 13 times, most recently from 4fb4f0f to a8e00a6 Compare May 14, 2024 05:40

snowsignal commented May 14, 2024

View reviewed changes

snowsignal force-pushed the jupyter-notebook branch from a8e00a6 to 4ddd930 Compare May 14, 2024 06:38

snowsignal marked this pull request as ready for review May 14, 2024 06:55

snowsignal requested a review from dhruvmanila as a code owner May 14, 2024 06:55

dhruvmanila reviewed May 14, 2024

View reviewed changes

dhruvmanila reviewed May 15, 2024

View reviewed changes

Comment thread crates/ruff_server/src/session/index.rs

snowsignal requested a review from dhruvmanila May 16, 2024 04:39

snowsignal force-pushed the jupyter-notebook branch from 38f718d to 8cb7d55 Compare May 16, 2024 04:45

snowsignal added 11 commits May 21, 2024 08:51

Implement support for Jupyter Notebooks in ruff server

365cf9c

Address some suggestions

fe648b5

Address additional suggestions

8a7ae3c

Pass in correct source type to formatter

5d81700

Remove notebook type check

a572acf

Update notebook selector to match ruff-lsp

7d1b08a

Use match instead of if/else for range edge case

7aaad37

Disable document versioning to fix notebook workspace edits

13ebc81

Avoid adding a line ending to a notebook cell when formatting

b425f3b

Add TODO

03837ee

Introduce notebook source actions

eae6368

snowsignal force-pushed the jupyter-notebook branch from 92c764a to eae6368 Compare May 21, 2024 15:51

snowsignal added 6 commits May 21, 2024 13:57

Fix VS Code initialization options test on Windows

67f5a4e

Add special case handling for the \r\n line ending

9972173

Clarify notebook range edge case and panic when a notebook range cros…

327c32f

…ses cell boundaries

Disable VS Code initialization option tests for Windows

4ab4eb7

Remove panic from to_notebook_range

392d333

Revise notebook range code

272f2fa

snowsignal force-pushed the jupyter-notebook branch from 04b7e15 to 272f2fa Compare May 21, 2024 22:25

snowsignal enabled auto-merge (squash) May 21, 2024 22:26

snowsignal merged commit b0731ef into main May 21, 2024

snowsignal deleted the jupyter-notebook branch May 21, 2024 22:29

snowsignal mentioned this pull request May 22, 2024

Fix automatic configuration reloading for text and notebook documents #11492

Merged

BrewTestBot mentioned this pull request May 23, 2024

ruff 0.4.5 Homebrew/homebrew-core#172580

Merged

AlexWaygood mentioned this pull request May 27, 2024

Rename document module to text_document #11571

Merged

snowsignal mentioned this pull request Jun 3, 2024

Workspace edits from ruff server commands occasionally fail in VS Code when operating on an yet-to-be-created file #11248

Closed

dhruvmanila mentioned this pull request Jun 4, 2024

Add document version to workspace edits #11730

Open

dhruvmanila mentioned this pull request Jul 30, 2024

Keep track of deleted cell for reorder change request #12575

Merged

Conversation

snowsignal commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions Bot commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

snowsignal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhruvmanila May 15, 2024

Choose a reason for hiding this comment

Uh oh!

snowsignal May 16, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila May 16, 2024

Choose a reason for hiding this comment

Uh oh!

snowsignal May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowsignal May 16, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snowsignal commented Apr 29, 2024 •

edited

Loading

github-actions Bot commented Apr 29, 2024 •

edited

Loading

`ruff-ecosystem` results

snowsignal May 16, 2024 •

edited

Loading

dhruvmanila May 16, 2024 •

edited

Loading