Add support for auto-fix in Jupyter notebooks by dhruvmanila · Pull Request #4665 · astral-sh/ruff

dhruvmanila · 2023-05-26T08:33:00Z

Summary

Add initial support for applying auto-fixes in Jupyter Notebook.

Solution

Cell offsets are the boundaries for each cell in the concatenated source code. They are represented using TextSize. It includes the start and end offset as well, thus creating a range for each cell. These offsets are updated using the SourceMap markers.

SourceMap

SourceMap contains markers constructed from each edits which tracks the original source code position to the transformed positions. The following drawing might make it clear:

The center column where the dotted lines are present are the markers included in the SourceMap. The Notebook looks at these markers and updates the cell offsets after each linter loop. If you notice closely, the destination takes into account all of the markers before it.

The index is constructed only when required as it's only used to render the diagnostics. So, a OnceCell is used for this purpose. The cell offsets, cell content and the index will be updated after each iteration of linting in the mentioned order. The order is important here as the content is updated as per the new offsets and index is updated as per the new content.

Limitations

1 (resolved `d721923`)

Auto-fixes which spans across multiple cell will panic. This is because it's difficult to determine which part of the edit content belongs to which cell. This mainly includes the import sorter in the following scenario where there are 2 continuous cells with import statements:

import random
import os
# ---
import math

(The commented line break is just to visualize cell separation and is not part of the code.)

Here, the concatenated content would be:

import random
import os
import math

And the formatted output would be:

import math
import random
import os

Now, it's not possible to determine which part of the content belongs to which cell and even if we try to update it as per the line count the context will change as an import might move out from the actual cell.

Possible solutions:

We could ignore the Edit which spans across multiple cells but this creates a problem in the lint loop. Take the above code as an example where there are 2 cells containing import statements which needs to be sorted. Now, this edit will span both cells, we'll ignore this edit, in the next iteration the same edit will be created and ignored and this will go on till we reach MAX_ITERATION.
Scope rules to be either global or local to the cell -- should we apply this rule to the concatenated source code or individually to each cell?
As this problem mainly occurs in the import sorter due to the fact that the Edit is scoped to an entire import block, we could refactor the sorter to create individual edits such as move this import statement from line 4 to line 2, remove this import statement, remove this symbol from this import statement, add a symbol to this import statement, etc. We would have to think about how to represent an edit like separating each symbol on it's own line (black style).

2

Styling rules such as the ones in pycodestyle will not be applicable everywhere in Jupyter notebook, especially at the cell boundaries. Let's take an example where a rule suggests to have 2 blank lines before a function and the cells contains the following code:

import something
# ---
def first():
	pass

def second():
	pass

(Again, the comment is only to visualize cell boundaries.)

In the concatenated source code, the 2 blank lines will be added but it shouldn't actually be added when we look in terms of Jupyter notebook. It's as if the function first is at the start of a file.

nbqa solves this by recording newlines before and after running autopep8, then running the tool and restoring the newlines at the end (refer nbQA-dev/nbQA#807).

Test Plan

Three commands were run in order with common flags (--select=ALL --no-cache --isolated) to isolate which stage the problem is occurring:

Only diagnostics
Fix with diff (--fix --diff)
Fix (--fix)

https://github.com/facebookresearch/segment-anything

-------------------------------------------------------------------------------
 Jupyter Notebooks       3            0            0            0            0
 |- Markdown             3           98            0           94            4
 |- Python               3          513          468            4           41
 (Total)                            611          468           98           45
-------------------------------------------------------------------------------

$ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/segment-anything/**/*.ipynb --fix
...
Found 180 errors (89 fixed, 91 remaining).

https://github.com/openai/openai-cookbook

-------------------------------------------------------------------------------
 Jupyter Notebooks      65            0            0            0            0
 |- Markdown            64         3475           12         2507          956
 |- Python              65         9700         7362         1101         1237
 (Total)                          13175         7374         3608         2193
===============================================================================

$ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/openai-cookbook/**/*.ipynb --fix
error: Failed to parse /path/to/openai-cookbook/examples/vector_databases/Using_vector_databases_for_embeddings_search.ipynb:cell 4:29:18: unexpected token '-'
...
Found 4227 errors (2165 fixed, 2062 remaining).

https://github.com/tensorflow/docs

-------------------------------------------------------------------------------
 Jupyter Notebooks     150            0            0            0            0
 |- Markdown             1           55            0           46            9
 |- Python               1          402          289           60           53
 (Total)                            457          289          106           62
-------------------------------------------------------------------------------

~~These repository includes some notebooks where it panics because an edit spans across multiple cells. This is due to the import statements are in 2 or more continuous cells.~~ (resolved d721923)

$ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/tensorflow-docs/**/*.ipynb --fix
error: Failed to parse /path/to/tensorflow-docs/site/en/guide/extension_type.ipynb:cell 80:1:1: unexpected token Indent
error: Failed to parse /path/to/tensorflow-docs/site/en/r1/tutorials/eager/custom_layers.ipynb:cell 20:1:1: unexpected token Indent
error: Failed to parse /path/to/tensorflow-docs/site/en/guide/data.ipynb:cell 175:5:14: unindent does not match any outer indentation level
error: Failed to parse /path/to/tensorflow-docs/site/en/r1/tutorials/representation/unicode.ipynb:cell 30:1:1: unexpected token Indent
...
Found 12726 errors (5140 fixed, 7586 remaining).

https://github.com/tensorflow/models

-------------------------------------------------------------------------------
 Jupyter Notebooks      46            0            0            0            0
 |- Markdown             1           11            0            6            5
 |- Python               1          328          249           19           60
 (Total)                            339          249           25           65
-------------------------------------------------------------------------------

$ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/tensorflow-models/**/*.ipynb --fix
...
Found 4856 errors (2690 fixed, 2166 remaining).

To-Do:

~~Revert back the auto-fixes if the linter loop panics (as is done for Python files)~~
Verify that the updated notebook is parseable by nbformat (only for testing purposes)
~~Add roundtrip support (this will most likely be done with the second todo)~~ (different PR)
Ignore cells containing magic commands (%, !, ?) on any cells (this will ignore cells which contains code as well as magic commands). ~~Just for backup, ignore any cells containing syntax error~~
~~Add --diff preview for Jupyter notebook~~
Newline at the end of each cell is removed but then Ruff will complain about missing newline at the end of file. The --fix flag will try to add the newline, but we'll remove it before updating the cell and this loop will go on.

resolves: #1218
fixes: #4556

github-actions · 2023-05-26T09:02:07Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.04      8.5±0.18ms     4.8 MB/sec    1.00      8.2±0.16ms     5.0 MB/sec
formatter/numpy/ctypeslib.py               1.02  1758.8±53.46µs     9.5 MB/sec    1.00  1722.1±44.25µs     9.7 MB/sec
formatter/numpy/globals.py                 1.02    169.5±5.41µs    17.4 MB/sec    1.00    166.4±9.00µs    17.7 MB/sec
formatter/pydantic/types.py                1.02      3.5±0.08ms     7.3 MB/sec    1.00      3.4±0.13ms     7.5 MB/sec
linter/all-rules/large/dataset.py          1.00     18.7±0.30ms     2.2 MB/sec    1.01     18.9±0.28ms     2.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.4±0.10ms     3.8 MB/sec    1.01      4.4±0.12ms     3.8 MB/sec
linter/all-rules/numpy/globals.py          1.00   550.5±10.93µs     5.4 MB/sec    1.02   563.6±20.20µs     5.2 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.8±0.18ms     3.3 MB/sec    1.02      7.9±0.23ms     3.2 MB/sec
linter/default-rules/large/dataset.py      1.00      8.5±0.12ms     4.8 MB/sec    1.04      8.9±0.14ms     4.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1867.9±53.82µs     8.9 MB/sec    1.02  1908.7±37.67µs     8.7 MB/sec
linter/default-rules/numpy/globals.py      1.00   218.4±12.47µs    13.5 MB/sec    1.01    219.8±6.26µs    13.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.0±0.09ms     6.4 MB/sec    1.02      4.1±0.14ms     6.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      8.0±0.05ms     5.1 MB/sec    1.00      7.9±0.06ms     5.1 MB/sec
formatter/numpy/ctypeslib.py               1.02  1627.4±14.17µs    10.2 MB/sec    1.00  1593.1±13.14µs    10.5 MB/sec
formatter/numpy/globals.py                 1.00    157.0±1.79µs    18.8 MB/sec    1.00    156.7±3.35µs    18.8 MB/sec
formatter/pydantic/types.py                1.00      3.3±0.03ms     7.8 MB/sec    1.00      3.3±0.04ms     7.8 MB/sec
linter/all-rules/large/dataset.py          1.00     16.9±0.15ms     2.4 MB/sec    1.00     16.9±0.13ms     2.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.4±0.04ms     3.8 MB/sec    1.00      4.3±0.08ms     3.8 MB/sec
linter/all-rules/numpy/globals.py          1.01    440.7±7.74µs     6.7 MB/sec    1.00    436.0±6.83µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.3±0.06ms     3.5 MB/sec    1.00      7.4±0.09ms     3.5 MB/sec
linter/default-rules/large/dataset.py      1.01      8.5±0.10ms     4.8 MB/sec    1.00      8.4±0.07ms     4.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1768.0±13.79µs     9.4 MB/sec    1.00  1747.5±12.47µs     9.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    189.9±2.89µs    15.5 MB/sec    1.00    189.7±5.45µs    15.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.8±0.02ms     6.7 MB/sec    1.00      3.8±0.02ms     6.7 MB/sec

konstin

looks good so far, i'll give it a more nit-picky review once it's not a draft anymore

konstin · 2023-05-26T15:31:16Z

crates/ruff/src/jupyter/index.rs

+    }
+}
+
+impl JupyterIndexBuilder {


Could we do without the separate builder here?

The reason to have a builder is to have both the index and cell offsets built in a single loop with each individual cell.

Cell offsets are built from the source code only for the first time. They cannot be refreshed using the transformed source code but require to process each individual edit to update the offsets.

When a single auto-fix iteration ends, the index is refreshed from the transformed source code, source code is updated in each cell and cell offsets are updated from the edits (order is important).

crates/ruff/src/jupyter/notebook.rs

dhruvmanila · 2023-06-01T06:44:03Z

crates/ruff/src/jupyter/notebook.rs

+                .unwrap_or_else(|| panic!("cell content out of bounds: {:?}", &self.raw.cells[pos]))
+                .trim_end_matches(|c| c == '\r' || c == '\n')
+                .to_string();
+            self.raw.cells[pos].source = SourceValue::String(cell_content);


The reason to choose String over StringArray is that the latter should contain the newline as part of each element which means that str.lines would not work and we would either need a custom iterator with strings including the newline character or add newlines to each element individually.

crates/ruff/src/jupyter/index.rs

MichaReiser · 2023-06-02T06:46:40Z

crates/ruff/src/jupyter/index.rs

+    pub(super) inner: Arc<JupyterIndexInner>,
+}
+
+impl JupyterIndex {


Maybe: Create newtype wrappers for Cell and Row (or use OneIndexed?) Newtype wrappers can encode the information that cell returns a Cell and prevents users from mixing cell with rows.

Newtype wrappers makes sense but let me see if OneIndexed works better.

Actually, I'm not sure if Newtype wrappers would be correct as the cell and row numbers are generated internally and it's only returned via the public interface. It's used directly when emitting the diagnostic message and it won't have any context about these newtypes.

Not sure if I understand fully, but I trust your judgment. My main intention of introducing them would be to make it harder to mix up the two.

crates/ruff/src/jupyter/notebook.rs

MichaReiser · 2023-06-02T07:06:38Z

crates/ruff/src/autofix/mod.rs

+    /// The set of [`Edit`] that were applied.
+    pub(crate) edits: BTreeSet<&'a Edit>,


Would it instead be possible to return a SourceMap that maps source positions to target positions in the FixResult. I would prefer that because it has a more narrow API and allows better re-use if we have other mapped types (e.g. python code in markdown files)

I would support this -- could you describe in a bit more detail how it would work?

I think it would roughly do the same as update_cell_offsets does today:

The source map is defined as a Vec<SourceMapMarker> where each marker stores the source to destination position:

https://github.com/charliermarsh/ruff/blob/d1c7a07d2ae56f4ff56b81c7f2a398e2262c42b9/crates/ruff_formatter/src/lib.rs#L285-L290

Insertion:

One marker at the start: insertion_offset -> output.text_len()

One marker at the end: insertion_offset -> output.text_len()

Deletion:

One marker at the start: deletion_start -> output.text_len()

One marker at the end: deletion_end -> output.text_len()

Replacement:

One marker at the start: replacement_start -> output.text_len()

One marker at the end: replacement_end -> output.text_len()

The entries are guaranteed to be sorted (at least per "apply fixes" iteration). We can now search the start and end of each cell (or use an iterator and make use of the sorted property to avoid cell count binary searches):

cell start: Find the last source map entry that is smaller or equal to the cell start. The output position of the start is: marker.dest + cell.start - marker.source I think we need to use the first entry if multiple source marker start RIGHT at the start of the cell.

cell end: Find the last source map entry that ends before or at cel.end. You can map the end as: marker.dest + marker.source - cell.end

There are probably some corner cases that I didn't consider.

One downside of this is that returning the BTreeMap is free because we build it anyway whereas building the source map has the cost of writing the source mark entries for each edit.

crates/ruff/src/jupyter/index.rs

konstin

needs tests, otherwise it's looking good

crates/ruff/src/jupyter/index.rs

konstin · 2023-06-02T07:56:44Z

crates/ruff/src/jupyter/notebook.rs

+            .collect(),
+    };
+    // Ignore a cell if it contains a magic command. There could be valid
+    // Python code as well, but we'll ignore that for now.


it's fine for this PR, but i believe we do need to keep the python code or we'll see some undefined symbol false positives because they were defined in cell that starts with a % line

Yes, we'll have to roll out our own magic transformer :)

crates/ruff/src/jupyter/notebook.rs

charliermarsh · 2023-06-02T15:18:21Z

My main suggestion for resolving (1), at least for import sorting (which, I think, is the only edit that spans multiple statements in this way), is to find a way to make the BlockBuilder aware of cell boundaries. The BlockBuilder already kind of supports this behavior, because it has to support isort's split directives:

import sys
import os
# isort: split
import abc
import collections

In this case, the first two imports will be treated as an import block, and the second two imports will be treated as an independent import block. Conceptually, this is identical (?) to the splits we want to enforce between cells. I'd suggest finding a way to feed those splits into the BlockBuilder!

dhruvmanila · 2023-06-03T17:17:14Z

i'd make this a source code comment

(There's no reply section for this comment, so putting it out here)

I'm not sure what do you mean by "source code comment". Do you want me to comment the reason for the trailing_newline?

would also make this a code comment

Same as above.

needs tests, otherwise it's looking good

Yes, I wanted to wait for the initial feedback incase there were some changes although I've already started writing some basic test cases.

zanieb · 2023-06-03T17:21:00Z

(There's no reply section for this comment, so putting it out here)

fyi GitHub does weird things with threaded conversations — the threads are up higher at #4665 (comment) / #4665 (comment)

dhruvmanila · 2023-06-09T13:16:20Z

Updates since last review

SourceMap have been added and the cell offsets are updated using that
Empty code cells are handled while creating the index
Basic unit tests have been added to tests the above improvements
Import sorting panic has been fixed by considering each cell as an individual block using the cell offsets

charliermarsh

This is really excellent work.

crates/ruff/src/linter.rs

crates/ruff/src/autofix/source_map.rs

charliermarsh · 2023-06-11T02:28:49Z

crates/ruff/src/rules/isort/block.rs

+                    break;
+                }
+            }
+        }


charliermarsh · 2023-06-11T02:29:07Z

crates/ruff/src/source_kind.rs

+pub enum SourceKind {
+    Python(String),
+    Jupyter(Notebook),
+}


charliermarsh · 2023-06-11T02:40:19Z

crates/ruff/src/jupyter/notebook.rs

+    ///
+    /// The index is built only once when required. This is only used to
+    /// report diagnostics, so by that time all of the autofixes must have
+    /// been applied if `--fix` was passed.


I'm loving all the documentation you're adding as you go. This is a very, very good habit. Props.

crates/ruff/src/jupyter/notebook.rs

konstin

Is there any integration test that shows a jupyter notebook before and after being fixed?

konstin · 2023-06-12T08:32:32Z

crates/ruff/src/jupyter/notebook.rs

+        }
+        // Ignore a cell if it contains a magic command. There could be valid
+        // Python code as well, but we'll ignore that for now.
+        // TODO(dhruvmanila): https://github.com/psf/black/blob/main/src/black/handle_ipynb_magics.py


my concern before setting this to on by default would be that this causes a bunch of undefined symbols when the symbols do exist. but i'd say let's merge this and check with notebooks from github whether this is even actually a problem.

find . -name "*.ipynb" | wc -l in the ecosystem checkouts says 7639, so we have more than enough notebooks to check

Yes, I do think this will be a problem, I've looked at how black handles it. I'll open an issue detailing out what needs to happen and we can then implement it.

dhruvmanila · 2023-06-12T14:05:31Z

Is there any integration test that shows a jupyter notebook before and after being fixed?

Currently, I've tested it out on the repositories mentioned in the PR description. I'm trying to think how to have E2E testing here as just the snapshots might not be enough. I'll run it on the ecosystem notebooks.

konstin · 2023-06-12T14:27:38Z

i was thinking about a notebook where we have before fixing and after fixing both in git and check that the fixed version looks like the one we have stored

dhruvmanila · 2023-06-12T15:51:53Z

Right, so do you mean to compare the 2 JSON objects? They both will be available in source control. I think we should rather compare the source code content as the JSON will not be identical:

The reason to choose String over StringArray is that the latter should contain the newline as part of each element which means that str.lines would not work and we would either need a custom iterator with strings including the newline character or add newlines to each element individually.

#4665 (comment)

So, 2 notebooks where one is the original content and the other what we expect Ruff to transform into.

Read the notebook
Pass the source code to Ruff
Update the internal state
Read the expected notebook
Compare

## Summary Add roundtrip support for Jupyter notebook. 1. Read the notebook 2. Extract out the source code content 3. Use it to update the notebook itself (should be exactly the same [^1]) 4. Serialize into JSON and print it to stdout ## Test Plan `cargo run --all-features --bin ruff_dev --package ruff_dev -- round-trip <path/to/notebook.ipynb>` <details><summary>Example output:</summary> ``` { "cells": [ { "cell_type": "markdown", "id": "f3c286e9-fa52-4440-816f-4449232f199a", "metadata": {}, "source": [ "# Ruff Test" ] }, { "cell_type": "markdown", "id": "a2b7bc6c-778a-4b07-86ae-dde5a2d9511e", "metadata": {}, "source": [ "Markdown block before the first import" ] }, { "cell_type": "code", "id": "5e3ef98e-224c-450a-80e6-be442ad50907", "metadata": { "tags": [] }, "source": "", "execution_count": 1, "outputs": [] }, { "cell_type": "code", "id": "6bced3f8-e0a4-450c-ae7c-f60ad5671ee9", "metadata": {}, "source": "import contextlib\n\nwith contextlib.suppress(ValueError):\n print()\n", "outputs": [] }, { "cell_type": "code", "id": "d7102cfd-5bb5-4f5b-a3b8-07a7b8cca34c", "metadata": {}, "source": "import random\n\nrandom.randint(10, 20)", "outputs": [] }, { "cell_type": "code", "id": "88471d1c-7429-4967-898f-b0088fcb4c53", "metadata": {}, "source": "foo = 1\nif foo < 2:\n msg = f\"Invalid foo: {foo}\"\n raise ValueError(msg)", "outputs": [] } ], "metadata": { "kernelspec": { "display_name": "Python (ruff-playground)", "name": "ruff-playground", "language": "python" }, "language_info": { "codemirror_mode": { "name": "ipython", "version": 3 }, "file_extension": ".py", "mimetype": "text/x-python", "name": "python", "pygments_lexer": "ipython3", "nbconvert_exporter": "python", "version": "3.11.3" } }, "nbformat": 4, "nbformat_minor": 5 } ``` </details> [^1]: The type in JSON might be different (#4665 (comment)) Part of #1218

## Summary Add support for applying auto-fixes in Jupyter Notebook. ### Solution Cell offsets are the boundaries for each cell in the concatenated source code. They are represented using `TextSize`. It includes the start and end offset as well, thus creating a range for each cell. These offsets are updated using the `SourceMap` markers. ### SourceMap `SourceMap` contains markers constructed from each edits which tracks the original source code position to the transformed positions. The following drawing might make it clear: ![SourceMap visualization](https://github.com/astral-sh/ruff/assets/67177269/3c94e591-70a7-4b57-bd32-0baa91cc7858) The center column where the dotted lines are present are the markers included in the `SourceMap`. The `Notebook` looks at these markers and updates the cell offsets after each linter loop. If you notice closely, the destination takes into account all of the markers before it. The index is constructed only when required as it's only used to render the diagnostics. So, a `OnceCell` is used for this purpose. The cell offsets, cell content and the index will be updated after each iteration of linting in the mentioned order. The order is important here as the content is updated as per the new offsets and index is updated as per the new content. ## Limitations ### 1 Styling rules such as the ones in `pycodestyle` will not be applicable everywhere in Jupyter notebook, especially at the cell boundaries. Let's take an example where a rule suggests to have 2 blank lines before a function and the cells contains the following code: ```python import something # --- def first(): pass def second(): pass ``` (Again, the comment is only to visualize cell boundaries.) In the concatenated source code, the 2 blank lines will be added but it shouldn't actually be added when we look in terms of Jupyter notebook. It's as if the function `first` is at the start of a file. `nbqa` solves this by recording newlines before and after running `autopep8`, then running the tool and restoring the newlines at the end (refer nbQA-dev/nbQA#807). ## Test Plan Three commands were run in order with common flags (`--select=ALL --no-cache --isolated`) to isolate which stage the problem is occurring: 1. Only diagnostics 2. Fix with diff (`--fix --diff`) 3. Fix (`--fix`) ### https://github.com/facebookresearch/segment-anything ``` ------------------------------------------------------------------------------- Jupyter Notebooks 3 0 0 0 0 |- Markdown 3 98 0 94 4 |- Python 3 513 468 4 41 (Total) 611 468 98 45 ------------------------------------------------------------------------------- ``` ```console $ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/segment-anything/**/*.ipynb --fix ... Found 180 errors (89 fixed, 91 remaining). ``` ### https://github.com/openai/openai-cookbook ``` ------------------------------------------------------------------------------- Jupyter Notebooks 65 0 0 0 0 |- Markdown 64 3475 12 2507 956 |- Python 65 9700 7362 1101 1237 (Total) 13175 7374 3608 2193 =============================================================================== ``` ```console $ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/openai-cookbook/**/*.ipynb --fix error: Failed to parse /path/to/openai-cookbook/examples/vector_databases/Using_vector_databases_for_embeddings_search.ipynb:cell 4:29:18: unexpected token '-' ... Found 4227 errors (2165 fixed, 2062 remaining). ``` ### https://github.com/tensorflow/docs ``` ------------------------------------------------------------------------------- Jupyter Notebooks 150 0 0 0 0 |- Markdown 1 55 0 46 9 |- Python 1 402 289 60 53 (Total) 457 289 106 62 ------------------------------------------------------------------------------- ``` ```console $ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/tensorflow-docs/**/*.ipynb --fix error: Failed to parse /path/to/tensorflow-docs/site/en/guide/extension_type.ipynb:cell 80:1:1: unexpected token Indent error: Failed to parse /path/to/tensorflow-docs/site/en/r1/tutorials/eager/custom_layers.ipynb:cell 20:1:1: unexpected token Indent error: Failed to parse /path/to/tensorflow-docs/site/en/guide/data.ipynb:cell 175:5:14: unindent does not match any outer indentation level error: Failed to parse /path/to/tensorflow-docs/site/en/r1/tutorials/representation/unicode.ipynb:cell 30:1:1: unexpected token Indent ... Found 12726 errors (5140 fixed, 7586 remaining). ``` ### https://github.com/tensorflow/models ``` ------------------------------------------------------------------------------- Jupyter Notebooks 46 0 0 0 0 |- Markdown 1 11 0 6 5 |- Python 1 328 249 19 60 (Total) 339 249 25 65 ------------------------------------------------------------------------------- ``` ```console $ cargo run --all-features --bin ruff -- check --no-cache --isolated --select=ALL /path/to/tensorflow-models/**/*.ipynb --fix ... Found 4856 errors (2690 fixed, 2166 remaining). ``` resolves: #1218 fixes: #4556

## Summary Add roundtrip support for Jupyter notebook. 1. Read the notebook 2. Extract out the source code content 3. Use it to update the notebook itself (should be exactly the same [^1]) 4. Serialize into JSON and print it to stdout ## Test Plan `cargo run --all-features --bin ruff_dev --package ruff_dev -- round-trip <path/to/notebook.ipynb>` <details><summary>Example output:</summary> ``` { "cells": [ { "cell_type": "markdown", "id": "f3c286e9-fa52-4440-816f-4449232f199a", "metadata": {}, "source": [ "# Ruff Test" ] }, { "cell_type": "markdown", "id": "a2b7bc6c-778a-4b07-86ae-dde5a2d9511e", "metadata": {}, "source": [ "Markdown block before the first import" ] }, { "cell_type": "code", "id": "5e3ef98e-224c-450a-80e6-be442ad50907", "metadata": { "tags": [] }, "source": "", "execution_count": 1, "outputs": [] }, { "cell_type": "code", "id": "6bced3f8-e0a4-450c-ae7c-f60ad5671ee9", "metadata": {}, "source": "import contextlib\n\nwith contextlib.suppress(ValueError):\n print()\n", "outputs": [] }, { "cell_type": "code", "id": "d7102cfd-5bb5-4f5b-a3b8-07a7b8cca34c", "metadata": {}, "source": "import random\n\nrandom.randint(10, 20)", "outputs": [] }, { "cell_type": "code", "id": "88471d1c-7429-4967-898f-b0088fcb4c53", "metadata": {}, "source": "foo = 1\nif foo < 2:\n msg = f\"Invalid foo: {foo}\"\n raise ValueError(msg)", "outputs": [] } ], "metadata": { "kernelspec": { "display_name": "Python (ruff-playground)", "name": "ruff-playground", "language": "python" }, "language_info": { "codemirror_mode": { "name": "ipython", "version": 3 }, "file_extension": ".py", "mimetype": "text/x-python", "name": "python", "pygments_lexer": "ipython3", "nbconvert_exporter": "python", "version": "3.11.3" } }, "nbformat": 4, "nbformat_minor": 5 } ``` </details> [^1]: The type in JSON might be different (#4665 (comment)) Part of #1218

dhruvmanila marked this pull request as draft May 26, 2023 08:33

konstin reviewed May 26, 2023

View reviewed changes

dhruvmanila commented May 31, 2023

View reviewed changes

crates/ruff/src/jupyter/notebook.rs Show resolved Hide resolved

dhruvmanila force-pushed the feat/jupyter-notebook-autofix branch from 1f234d9 to 28f09de Compare June 1, 2023 05:29

dhruvmanila commented Jun 1, 2023

View reviewed changes

dhruvmanila force-pushed the feat/jupyter-notebook-autofix branch from bf72009 to 7483383 Compare June 1, 2023 08:47

dhruvmanila commented Jun 1, 2023

View reviewed changes

crates/ruff/src/jupyter/index.rs Outdated Show resolved Hide resolved

crates/ruff/src/jupyter/index.rs Outdated Show resolved Hide resolved

dhruvmanila marked this pull request as ready for review June 1, 2023 10:44

dhruvmanila requested a review from konstin June 1, 2023 10:44

charliermarsh self-requested a review June 1, 2023 15:17

MichaReiser reviewed Jun 2, 2023

View reviewed changes

konstin reviewed Jun 2, 2023

View reviewed changes

dhruvmanila force-pushed the feat/jupyter-notebook-autofix branch from afe9a2e to b10e940 Compare June 4, 2023 10:43

dhruvmanila changed the title ~~Initial support for auto-fix in Jupyter notebooks~~ Add support for auto-fix in Jupyter notebooks Jun 9, 2023

charliermarsh approved these changes Jun 11, 2023

View reviewed changes

konstin approved these changes Jun 12, 2023

View reviewed changes

dhruvmanila added 9 commits June 12, 2023 19:24

Initial support for auto-fix in Jupyter notebooks

4002a45

Doc, publicity, fix test

1bf22ca

Ignore cells which contain magic, shell or help command

2d44a70

Ignore cell which could contain magic commands in any line

c940c56

Consider trailing newline when building the index

7c0587b

Fix failing test

19ec200

Add Jupyter notebook support to DisplayParseError

3b495cc

Create FixResult struct

3881dc5

Use FixResult in tests

eed20f5

dhruvmanila added 10 commits June 12, 2023 19:25

Generate source map, use it to update cell offsets

af0ef44

Add docs to sourcemap implementation

b036cd7

Update is_valid_code_cell tests

727da16

Update notebook docs

fc145b6

Update autofix tests to check source map markers

6f13982

Consider empty cell for line count

51ce63c

Add test case for insertion edit

10cc2cb

Move comments as doc notes

182205c

Simplify index building function

e29f986

Use pub(crate), move code logically

be2f295

dhruvmanila force-pushed the feat/jupyter-notebook-autofix branch from b08c7c2 to be2f295 Compare June 12, 2023 13:55

Fix import to use the updated name

7813cc7

dhruvmanila enabled auto-merge (squash) June 12, 2023 14:11

dhruvmanila merged commit d8f5d2d into astral-sh:main Jun 12, 2023

dhruvmanila deleted the feat/jupyter-notebook-autofix branch June 12, 2023 14:15

chenrui333 mentioned this pull request Jun 20, 2023

ruff 0.0.273 Homebrew/homebrew-core#134306

Merged

dhruvmanila mentioned this pull request Jul 26, 2023

Complete Jupyter notebook integration #5188

Closed

26 tasks

renovate bot mentioned this pull request Aug 9, 2023

Update all non-major dependencies fedora-infra/koji-fedoramessaging-messages#11

Merged

1 task

dhruvmanila mentioned this pull request Nov 19, 2024

Fixes on Jupyter notebooks cause panics when fix replaces/deletes multiple cells #14445

Open

		/// The set of [`Edit`] that were applied.
		pub(crate) edits: BTreeSet<&'a Edit>,

Conversation

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

Uh oh!

Summary

Solution

SourceMap

Limitations

1 (resolved d721923)

Possible solutions:

2

Test Plan

To-Do:

Uh oh!

github-actions bot commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

konstin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhruvmanila Jun 1, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

konstin left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charliermarsh commented Jun 2, 2023

Uh oh!

dhruvmanila commented Jun 3, 2023

Uh oh!

zanieb commented Jun 3, 2023

Uh oh!

dhruvmanila commented Jun 9, 2023

Updates since last review

Uh oh!

charliermarsh left a comment

dhruvmanila commented May 26, 2023 •

edited

Loading

1 (resolved `d721923`)

github-actions bot commented May 26, 2023 •

edited

Loading

dhruvmanila Jun 1, 2023 •

edited

Loading

konstin left a comment •

edited

Loading