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

False positive F401 due to %%ipytest line in notebook #13718

Closed
tovrstra opened this issue Oct 11, 2024 · 9 comments · Fixed by #13745
Closed

False positive F401 due to %%ipytest line in notebook #13718

tovrstra opened this issue Oct 11, 2024 · 9 comments · Fixed by #13745
Labels
notebook Related to (Jupyter) notebooks

Comments

@tovrstra
Copy link

Keywords to find existing issue: F401 and ipytest. The following is very similar, but still different, it seems: #10454

A minimal working example would be a Jupyter notebook with the following two cells:

import ipytest
import pytest

ipytest.autoconfig()
%%ipytest

def test_simple():
    with pytest.raises(TypeError):
        print(1 + "a")

Ruff will report F401 (unused-import) for the line import pytest.

To reproduce the issue, create a notebook with the above cells, called bug.ipynb and run the command:

ruff check demos/bug.ipynb --fix

I've tested with with ruff 0.6.3 and 0.6.9. I have the following in my pyproject.toml, but I don't think these settings matter:

[tool.ruff]
line-length = 100
target-version = "py311"

[tool.ruff.lint]
select = ["E", "F", "UP", "B", "I", "PGH", "PL", "RUF"]
ignore = ["E741", "PLR2004", "PLR0913", "PLR0912", "PLW2901", "PLR0915"]

As a workaround, one can (obviously) add a comment # noqa: F401 to the import line

When removing the line %%ipytest, the problem goes away, but it that doesn't help because the line is doing something useful.

Given the easy workaround, this is probably not urgent.

By the way, thanks for creating Ruff!

@AlexWaygood AlexWaygood added the notebook Related to (Jupyter) notebooks label Oct 11, 2024
@MichaReiser
Copy link
Member

I don't fully understnad yet what's happening but the code works as expected when I change %%ipytest to %ipytest

@MichaReiser
Copy link
Member

@dhruvmanila do you have an idea why using the %%magic form makes the diagnostic appear?

@dhruvmanila
Copy link
Member

Yeah, the double percent like %%magic are cell magics which apply to the entire cell and so we ignore those cells except for a few:

// These cell magics are special in that the lines following them are valid
// Python code and the variables defined in that scope are available to the
// rest of the notebook.
//
// For example:
//
// Cell 1:
// ```python
// x = 1
// ```
//
// Cell 2:
// ```python
// %%time
// y = x
// ```
//
// Cell 3:
// ```python
// print(y) # Here, `y` is available.
// ```
//
// This is to avoid false positives when these variables are referenced
// elsewhere in the notebook.
!matches!(
command,
"capture" | "debug" | "prun" | "pypy" | "python" | "python3" | "time" | "timeit"
)

While, the single percent like %magic are line magics that considers only the line on which it is defined.

@MichaReiser
Copy link
Member

Oh that's good to know. Thanks. So the solution here would be to move the %%ipytest command to its own cell?

@dhruvmanila
Copy link
Member

I think it is in its own cell. It's just that the entire cell is currently ignored by Ruff because ipytest is being special cased. I think we could do it because the lines following the ipytest magic should contain valid Python code. @tovrstra I'm not familiar with the magic command so just want to confirm if that's valid. I can check as well.

@MichaReiser
Copy link
Member

What I mean is that a possible workaround is to change the notebook to have three cells:

import ipytest
import pytest

ipytest.autoconfig()
%%ipytest
def test_simple():
    with pytest.raises(TypeError):
        print(1 + "a")

@dhruvmanila
Copy link
Member

I think then the magic command won't work because it won't be applied to the cell it was previously targeting.

@dhruvmanila
Copy link
Member

I just tested it, it's safe to include ipytest in the allowed cell magic in the following code. And, the implementation of %%ipytest also confirms that it runs the cell first and then runs pytest on top of it. So, the cell can be considered as being without the magic itself.

!matches!(
command,
"capture" | "debug" | "prun" | "pypy" | "python" | "python3" | "time" | "timeit"
)

Screenshot 2024-10-14 at 15 14 42

dhruvmanila added a commit that referenced this issue Oct 14, 2024
## Summary

fixes: #13718 

## Test Plan

Using the notebook as mentioned in
#13718 (comment),
this PR does not give the "F821 Undefined name `test_sorted`"
diagnostic.
@tovrstra
Copy link
Author

Thank you all for the comments and the fix. My apologies for commenting late. Indeed, the %%ipytest should be on the first line of the cell containing unit tests. It means "this cell contains unit tests". If it is put in a separate cell or further down, the tests will not get executed when running the cell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook Related to (Jupyter) notebooks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants