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

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

Merged
merged 16 commits into from
Mar 25, 2024

Conversation

hoel-bagard
Copy link
Contributor

Summary

Closes #10228

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.

Copy link
Contributor

github-actions bot commented Mar 8, 2024

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

@hoel-bagard hoel-bagard marked this pull request as ready for review March 8, 2024 04:34
@dhruvmanila dhruvmanila self-requested a review March 8, 2024 04:35
@@ -505,6 +517,15 @@ impl<'a> Iterator for LinePreprocessor<'a> {
// Set the start for the next logical line.
self.line_start = range.end();

if self
.cell_offsets
.is_some_and(|cell_offsets| cell_offsets.contains(&self.line_start))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 520 to 528
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;
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hoel-bagard hoel-bagard Mar 8, 2024

Choose a reason for hiding this comment

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

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.

@hoel-bagard hoel-bagard requested a review from MichaReiser March 11, 2024 13:25
@MichaReiser
Copy link
Member

@dhruvmanila would you mind taking a look

@dhruvmanila
Copy link
Member

(Sorry, will look at it today.)

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

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:

// 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
Copy link
Contributor Author

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

@MichaReiser MichaReiser requested a review from dhruvmanila March 25, 2024 07:58
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thank you!

@dhruvmanila 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 dhruvmanila added the rule Implementing or modifying a lint rule label Mar 25, 2024
@dhruvmanila dhruvmanila enabled auto-merge (squash) March 25, 2024 11:11
@dhruvmanila dhruvmanila merged commit 9512bd6 into astral-sh:main Mar 25, 2024
17 checks passed
@hoel-bagard hoel-bagard deleted the hoel/fix_10228 branch March 25, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E302 and E305 cause the linter and formatter to make conflicting changes in notebooks
3 participants