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

Formatting imports in notebook in VSCode injects random code into cell #680

Open
JohnAtl opened this issue Feb 5, 2025 · 11 comments
Open
Labels
code-actions Related to code actions notebook Related to (Jupyter) notebooks

Comments

@JohnAtl
Copy link

JohnAtl commented Feb 5, 2025

When I save the notebook after making changes to my imports, random bits of code from the file are interspersed and repeated in the cell containing the imports.

Version
2025.4.0
Last Updated
2025-01-24, 08:43:26

In this video, I click File|Save, and the reformatting happens. I closed and reopened the file, and it still happened. I closed and reopened VSCode, and it still happened, though the amount of code injected was less.

I haven't seen this occur in .py files, only .ipynb files.

@dhruvmanila
Copy link
Member

Hi, thanks for the report.

Can you provide more details regarding the issue? I'm specifically looking for:

  • VS Code settings
  • Do you have any other extensions installed that supports formatting Python source code? Like the isort extension?
  • I'm assuming that you have either editor.formatOnSave or editor.codeActionsOnSave turned on but I'm not sure which one
  • Which Ruff version are you using?

There's the troubleshooting guide which can be used to get the logs which will be very useful to debug the issue.

@dhruvmanila dhruvmanila added the needs-info More information is needed from the issue author label Feb 6, 2025
@hamdanal
Copy link

hamdanal commented Feb 8, 2025

Cross posting my reproducer from astral-sh/ruff#15991 (reply in thread)

I've got a reproducer, here are the steps:

  1. Create a project and open it in vscode
    mkdir ruff-server-bug
    cd ruff-server-bug
    uv venv --python 3.12
    uv pip install ipykernel seaborn
    touch main.ipynb
    code .
  2. Install The ruff and Jupyter extension from Microsoft (I also have the Python and Pylance extensions from Microsoft)
  3. Create the following .vscode/settings.json file:
    {
        "notebook.formatOnSave.enabled": true,
        "notebook.codeActionsOnSave": {
            "source.organizeImports.ruff": "explicit",
        },
        "[python]": {
            "editor.defaultFormatter": "charliermarsh.ruff",
            "editor.codeActionsOnSave": {
                "source.organizeImports.ruff": "explicit",
            },
        },
        "ruff.trace.server": "verbose",
        // "ruff.nativeServer": "off",
    }
  4. Open the main.ipynb file
  5. Create a cell with import numpy as np, seaborn as sns
  6. Create an empty cell below it
  7. Hit Ctrl+S to save the notebook <--------------- Magic happens in the first cell (either import seaborn as sns gets duplicated or it gets deleted)
  8. Hit more Ctrl+S to see more magic

The problematic behavior goes away if you either:

  1. turn off the native ruff server (uncomment last line in the settings above)
  2. replace the line "source.organizeImports.ruff": "explicit", in the settings by the line "notebook.source.organizeImports.ruff": "explicit",
  3. delete the final empty cell

Final note: I saw this behavior both on Linux Debian 12 and on Windows 11 with the latest (and with earlier) versions of VS Code and the ruff extension

@dhruvmanila
Copy link
Member

2. replace the line "source.organizeImports.ruff": "explicit", in the settings by the line "notebook.source.organizeImports.ruff": "explicit",

We don't recommend using source.organizeImports.ruff for notebooks, instead notebook.* prefixed code actions should be used. They both behave very differently and is the reason that the VS Code team introduced notebook prefixed source code actions.

Maybe we can improve the UX around code actions between notebook and Python files to disallow this kind of problems.

@hamdanal
Copy link

hamdanal commented Feb 9, 2025

We don't recommend using source.organizeImports.ruff for notebooks, instead notebook.* prefixed code actions should be used. They both behave very differently and is the reason that the VS Code team introduced notebook prefixed source code actions.

Yes but now you run into #593

Maybe we can improve the UX around code actions between notebook and Python files to disallow this kind of problems.

I think that if the new ruff server doesn’t support the source.organizeImports action it should do nothing instead of replacing code with gibberish. I lost about two hours worth of work once because of it, that’s why I still use the old lsp server.

Since the only combination of server/settings that always work is the source.organizeImports action with the old lsp server, I recommend not removing the old server until these issues are resolved.

@AlexWaygood AlexWaygood removed the needs-info More information is needed from the issue author label Feb 9, 2025
@dhruvmanila
Copy link
Member

Ok, I think I've narrowed this down to why it's happening in the native server and not ruff-lsp for source.organizeImports.ruff:

  • ruff-lsp receives the request for each individual cells and sends the response for that specific cell only
  • Native server receives the request for each individual cells and sends the response for the entire notebook which means it sends two edit request to the client which results in gibberish text

I think the reason is that Ruff requires context of the entire notebook but that's only relevant for fixAll and not organizeImports (import organization doesn't require context from other cells). So, the solution would be to:

  1. Remove support for non-notebook prefixed code actions from native server
  2. Fix this issue such that applying code actions on individual cells work

I'm leaning towards (1) because (2) will still be problematic for the "fixAll" code action and will only work for "organizeImports" code action.


(I initially posted this in #593 but it's specific to this issue)

@MichaReiser
Copy link
Member

Would it help if we included the document version into the edit responses? I think that came up somewhere before

@dhruvmanila
Copy link
Member

Would it help if we included the document version into the edit responses? I think that came up somewhere before

I'll have to check, not sure

@dhruvmanila
Copy link
Member

Looked into versioned edits a bit, I think it might require some more time to invest into how that is being considered but I tried the following patch and that didn't seem to work:

diff --git a/crates/ruff_server/src/edit.rs b/crates/ruff_server/src/edit.rs
index 3a7ffb4e3..4d5546461 100644
--- a/crates/ruff_server/src/edit.rs
+++ b/crates/ruff_server/src/edit.rs
@@ -120,7 +120,7 @@ impl WorkspaceEditTracker {
     pub(crate) fn set_edits_for_document(
         &mut self,
         uri: Url,
-        _version: DocumentVersion,
+        version: DocumentVersion,
         edits: Vec<lsp_types::TextEdit>,
     ) -> crate::Result<()> {
         match self {
@@ -136,8 +136,7 @@ impl WorkspaceEditTracker {
                 document_edits.push(lsp_types::TextDocumentEdit {
                     text_document: lsp_types::OptionalVersionedTextDocumentIdentifier {
                         uri,
-                        // TODO(jane): Re-enable versioned edits after investigating whether it could work with notebook cells
-                        version: None,
+                        version: Some(version),
                     },
                     edits: edits.into_iter().map(lsp_types::OneOf::Left).collect(),
                 });
diff --git a/crates/ruff_server/src/session/index.rs b/crates/ruff_server/src/session/index.rs
index 4067d6be1..c0d40f8bf 100644
--- a/crates/ruff_server/src/session/index.rs
+++ b/crates/ruff_server/src/session/index.rs
@@ -583,7 +583,16 @@ impl DocumentQuery {
     pub(crate) fn version(&self) -> DocumentVersion {
         match self {
             Self::Text { document, .. } => document.version(),
-            Self::Notebook { notebook, .. } => notebook.version(),
+            Self::Notebook {
+                notebook,
+                cell_url: None,
+                ..
+            } => notebook.version(),
+            Self::Notebook {
+                notebook,
+                cell_url: Some(cell_url),
+                ..
+            } => notebook.cell_document_by_uri(cell_url).unwrap().version(),
         }
     }

I see many different version numbers in the trace logs (3x, 7x, etc.) for notebooks and cells so I would need to understand the lifecycle of it and how it's being considered.

@MichaReiser
Copy link
Member

Different version numbers sort of make sense, no? I'd expect that the notebook and every cell are tracked individually (or, at least, VS Code has the freedom to do so)

@dhruvmanila
Copy link
Member

I looked into this a bit more and I think there might be a problem here.

Considering my patch above, the version that is being passed here:

https://github.com/astral-sh/ruff/blob/6e34f74c164a646f07f3fe9bed9492c3557a95d7/crates/ruff_server/src/server/api/requests/code_action_resolve.rs#L88-L88

would be the version of either a text document (Python file), notebook document or a single notebook cell.

So, for instance, when VS Code is requesting to resolve a code action for a specific notebook cell (2 separate cells in the following example):

[Trace - 3:55:59 PM] Sending request 'codeAction/resolve - (31)'.
Params: {
    "title": "Ruff: Organize imports",
    "data": "vscode-notebook-cell:/private/tmp/ruff-server-bug/main.ipynb#W1sZmlsZQ%3D%3D",
    "kind": "source.organizeImports.ruff"
}


[Trace - 3:55:59 PM] Sending request 'codeAction/resolve - (32)'.
Params: {
    "title": "Ruff: Organize imports",
    "data": "vscode-notebook-cell:/private/tmp/ruff-server-bug/main.ipynb#W2sZmlsZQ%3D%3D",
    "kind": "source.organizeImports.ruff"
}

The server will use the version of that specific cell for all the cells that makes up the workspace edit.

https://github.com/astral-sh/ruff/blob/6e34f74c164a646f07f3fe9bed9492c3557a95d7/crates/ruff_server/src/edit.rs#L105-L115

Here, the version is for a specific notebook cell while fixes contains edits for all cells in that notebook as it's a source level code action.

I could see if fixing this would resolve the issue.

@dhruvmanila
Copy link
Member

I could see if fixing this would resolve the issue.

I don't think it's possible as I just realized that these requests only have access to a single document and thus can't query the version for other notebook cells.

@dhruvmanila dhruvmanila added code-actions Related to code actions notebook Related to (Jupyter) notebooks labels Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-actions Related to code actions notebook Related to (Jupyter) notebooks
Projects
None yet
Development

No branches or pull requests

5 participants