[`pycodestyle`] Avoid blank line rules for the first logical line in cell by hoel-bagard · Pull Request #10291 · astral-sh/ruff

hoel-bagard · 2024-03-08T03:41:54Z

Summary

The PR makes the blank lines rules keep track of the cell status when running on a notebook, and makes the rules not trigger when the line is the first of the cell.

Test Plan

The example given in #10228 is added as a fixture, along with a few tests from the main blank lines fixtures.

… being taken into account.

github-actions · 2024-03-08T03:58:30Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -111 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

mlflow/mlflow (+0 -111 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- docs/source/deep-learning/keras/quickstart/quickstart_keras.ipynb:cell 21:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/deep-learning/keras/quickstart/quickstart_keras.ipynb:cell 22:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/deep-learning/pytorch/quickstart/pytorch_quickstart.ipynb:cell 14:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/deep-learning/pytorch/quickstart/pytorch_quickstart.ipynb:cell 16:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/deep-learning/pytorch/quickstart/pytorch_quickstart.ipynb:cell 23:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/deep-learning/pytorch/quickstart/pytorch_quickstart.ipynb:cell 25:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/deep-learning/pytorch/quickstart/pytorch_quickstart.ipynb:cell 27:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/deep-learning/tensorflow/quickstart/quickstart_tensorflow.ipynb:cell 30:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/deep-learning/tensorflow/quickstart/quickstart_tensorflow.ipynb:cell 9:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/getting-started/logging-first-model/notebooks/logging-first-model.ipynb:cell 14:3:1: E305 [*] Expected 2 blank lines after class or function definition, found (1)
- docs/source/llms/custom-pyfunc-for-llms/notebooks/custom-pyfunc-advanced-llm.ipynb:cell 11:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/custom-pyfunc-for-llms/notebooks/custom-pyfunc-advanced-llm.ipynb:cell 9:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/langchain/notebooks/langchain-retriever.ipynb:cell 11:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/langchain/notebooks/langchain-retriever.ipynb:cell 13:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/langchain/notebooks/langchain-retriever.ipynb:cell 19:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/langchain/notebooks/langchain-retriever.ipynb:cell 21:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/langchain/notebooks/langchain-retriever.ipynb:cell 9:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/llm-evaluate/notebooks/rag-evaluation-llama2.ipynb:cell 14:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/llm-evaluate/notebooks/rag-evaluation-llama2.ipynb:cell 16:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/llm-evaluate/notebooks/rag-evaluation.ipynb:cell 10:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/llm-evaluate/notebooks/rag-evaluation.ipynb:cell 13:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (1)
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 16:2:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 18:2:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 22:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 24:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 28:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 30:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 32:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 33:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 35:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-code-helper.ipynb:cell 36:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/openai/notebooks/openai-embeddings-generation.ipynb:cell 13:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/openai/notebooks/openai-embeddings-generation.ipynb:cell 15:3:1: E305 [*] Expected 2 blank lines after class or function definition, found (1)
- docs/source/llms/openai/notebooks/openai-embeddings-generation.ipynb:cell 9:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/mlflow-e2e-evaluation.ipynb:cell 22:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/mlflow-e2e-evaluation.ipynb:cell 26:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/mlflow-e2e-evaluation.ipynb:cell 28:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/mlflow-e2e-evaluation.ipynb:cell 30:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/rag/notebooks/question-generation-retrieval-evaluation.ipynb:cell 22:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/question-generation-retrieval-evaluation.ipynb:cell 23:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/rag/notebooks/question-generation-retrieval-evaluation.ipynb:cell 28:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/question-generation-retrieval-evaluation.ipynb:cell 29:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/rag/notebooks/question-generation-retrieval-evaluation.ipynb:cell 53:1:1: E302 [*] Expected 2 blank lines, found 0
- docs/source/llms/rag/notebooks/question-generation-retrieval-evaluation.ipynb:cell 8:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/rag/notebooks/retriever-evaluation-tutorial.ipynb:cell 22:2:1: E302 [*] Expected 2 blank lines, found 0
... 37 additional changes omitted for rule E302
- docs/source/llms/rag/notebooks/retriever-evaluation-tutorial.ipynb:cell 24:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/rag/notebooks/retriever-evaluation-tutorial.ipynb:cell 45:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/sentence-transformers/tutorials/paraphrase-mining/paraphrase-mining-sentence-transformers.ipynb:cell 8:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/sentence-transformers/tutorials/semantic-search/semantic-search-sentence-transformers.ipynb:cell 8:1:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
- docs/source/llms/sentence-transformers/tutorials/semantic-similarity/semantic-similarity-sentence-transformers.ipynb:cell 8:2:1: E305 [*] Expected 2 blank lines after class or function definition, found (0)
... 61 additional changes omitted for project

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
E302	62	0	62	0	0
E305	49	0	49	0	0

MichaReiser · 2024-03-08T07:54:47Z

crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs


+                    if self
+                        .cell_offsets
+                        .is_some_and(|cell_offsets| cell_offsets.contains(&self.line_start))


You may want to use containing_range here. contains is O(n), containing_range is O(log(n)). But I wonder if we could do better by keeping an iterator with the cell offsets (they're sorted) and peek at the first offset and:

while it is smaller than the line_start, call next

if it is not None, you know its inside of the cell.
This gives you O(n) performance

@MichaReiser It's not exactly what you said, but I gave it a try in 3f703af. What do you think ?

MichaReiser · 2024-03-08T07:56:31Z

crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs

+                    if self
+                        .cell_offsets
+                        .is_some_and(|cell_offsets| cell_offsets.contains(&self.line_start))
+                    {
+                        self.is_cell_first_non_comment_line = true;
+                    } else if !line_is_comment_only {
+                        self.is_cell_first_non_comment_line = false;
+                    }
+


Is it okay that we set is_cell_first_non_comment_line to true even if line_is_comment_only is true?

It's fine since if a cell starts with a comment, we don't want to run the check on that comment (since it's the first line).
However the naming is pretty bad indeed, I'll try to improve it.

Edit: renamed in f29a53d.

MichaReiser · 2024-03-18T13:38:24Z

@dhruvmanila would you mind taking a look

crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs

dhruvmanila · 2024-03-18T14:44:08Z

(Sorry, will look at it today.)

dhruvmanila

I've started looking into it and testing out the code for the E30 rules. I've noticed a few things:

`E303`

For the following two cells:

# There are two blank lines after `function1`
def function1():
	pass

 
def function2():
	pass
# There is one blank line before `function2`

This is still raising the E303 error. I guess this is because we continue without checking for the cell boundary here:

ruff/crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs

Lines 430 to 437 in 461cdad

    
           // An empty line 
        
           if token_kind == TokenKind::NonLogicalNewline { 
        
               blank_lines.add(*range); 
        
               self.line_start = range.end(); 
        
               continue; 
        
           }

I think the intent for the fix should be to reset the count of blank lines such that the number of empty lines before the function2 gives us 1 instead of 3.

`E304`

I don't really have a good way of testing this unless we actually define the decorator and function definition in separate cells like so:

# One empty line after the decorator
@decorator

def function():
	pass

But this is highly unlikely to come up in a real world and in the future we would be making sure that this raises a syntax error (#10264). Nevertheless this suffers from the same problem as mentioned earlier. I'm assuming the root cause is the same.

hoel-bagard · 2024-03-22T13:51:34Z

@dhruvmanila Thanks for the review. I've fixed the E303 issue you found.

dhruvmanila

Thank you!

hoel-bagard added 4 commits March 8, 2024 00:02

fix: Do not add blank lines at the start of cells in notebooks.

122136e

fix: move cell start detection to line preprocessor, fix comments not…

1d6a35d

… being taken into account.

add fixtures

89964b9

refactor: move cell check to before the blank rules checks.

a2842a3

Merge branch 'main' into hoel/fix_10228

8d0776b

hoel-bagard marked this pull request as ready for review March 8, 2024 04:34

dhruvmanila self-requested a review March 8, 2024 04:35

MichaReiser reviewed Mar 8, 2024

View reviewed changes

hoel-bagard added 2 commits March 8, 2024 22:10

refactor: rename is_cell_first_non_comment_line to is_beginning_of_cell

f29a53d

perf: use an iterator for cell_offsets in order to make loop-ups faster.

3f703af

hoel-bagard requested a review from MichaReiser March 11, 2024 13:25

MichaReiser approved these changes Mar 18, 2024

View reviewed changes

crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs Outdated Show resolved Hide resolved

hoel-bagard added 2 commits March 18, 2024 23:26

fix potential panic for empty files.

05a7473

merge with main

1374e23

dhruvmanila requested changes Mar 19, 2024

View reviewed changes

hoel-bagard added 5 commits March 20, 2024 20:14

add notebook ok fixture

6196e91

reset blank lines counter at the start of a new cell

dd8c32e

also reset local blank_lines variable.

d543e72

add E303 notebook test case

bce6989

fix first line of each cell being ignored for all rules in notebooks.

559b030

hoel-bagard force-pushed the hoel/fix_10228 branch from c5c648f to 559b030 Compare March 22, 2024 13:25

deduplicate code.

a2cea4a

hoel-bagard force-pushed the hoel/fix_10228 branch from 5216bcb to a2cea4a Compare March 22, 2024 13:35

MichaReiser requested a review from dhruvmanila March 25, 2024 07:58

dhruvmanila approved these changes Mar 25, 2024

View reviewed changes

dhruvmanila changed the title ~~[pycodestyle] Fix blank line rules adding blank lines at the beginning of cells.~~ [pycodestyle] Avoid blank line rules for the first logical line in cell Mar 25, 2024

dhruvmanila added the rule Implementing or modifying a lint rule label Mar 25, 2024

Merge branch 'main' into hoel/fix_10228

a0946de

dhruvmanila enabled auto-merge (squash) March 25, 2024 11:11

dhruvmanila merged commit 9512bd6 into astral-sh:main Mar 25, 2024

hoel-bagard deleted the hoel/fix_10228 branch March 25, 2024 13:09

BrewTestBot mentioned this pull request Apr 1, 2024

ruff 0.3.5 Homebrew/homebrew-core#167712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`pycodestyle`] Avoid blank line rules for the first logical line in cell#10291

[`pycodestyle`] Avoid blank line rules for the first logical line in cell#10291
dhruvmanila merged 16 commits intoastral-sh:mainfrom
hoel-bagard:hoel/fix_10228

hoel-bagard commented Mar 8, 2024

Uh oh!

github-actions bot commented Mar 8, 2024 •

edited

Loading

Uh oh!

MichaReiser Mar 8, 2024

Uh oh!

hoel-bagard Mar 9, 2024

Uh oh!

MichaReiser Mar 8, 2024

Uh oh!

hoel-bagard Mar 8, 2024 •

edited

Loading

Uh oh!

MichaReiser commented Mar 18, 2024

Uh oh!

Uh oh!

dhruvmanila commented Mar 18, 2024

Uh oh!

dhruvmanila left a comment

Uh oh!

hoel-bagard commented Mar 22, 2024

Uh oh!

dhruvmanila left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// An empty line
	if token_kind == TokenKind::NonLogicalNewline {
	blank_lines.add(*range);

	self.line_start = range.end();

	continue;
	}

Conversation

hoel-bagard commented Mar 8, 2024

Summary

Test Plan

Uh oh!

github-actions bot commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

hoel-bagard Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

hoel-bagard Mar 8, 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!

MichaReiser commented Mar 18, 2024

Uh oh!

Uh oh!

dhruvmanila commented Mar 18, 2024

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

E303

E304

Uh oh!

hoel-bagard commented Mar 22, 2024

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 8, 2024 •

edited

Loading

`ruff-ecosystem` results

hoel-bagard Mar 8, 2024 •

edited

Loading

`E303`

`E304`