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

F401 false positive when using Jupyter line magic (%timeit imported_thing()) #10454

Open
flying-sheep opened this issue Mar 18, 2024 · 10 comments
Labels
documentation Improvements or additions to documentation linter Related to the linter notebook Related to (Jupyter) notebooks

Comments

@flying-sheep
Copy link
Contributor

I searched for “timeit F401” and didn’t find anything relevant.

Reproducer:

import os

%timeit os.getcwd()

Will show a false positive of F401 (Unused import) for the import os line:

image
@AlexWaygood AlexWaygood added rule Implementing or modifying a lint rule notebook Related to (Jupyter) notebooks linter Related to the linter labels Mar 18, 2024
@MichaReiser
Copy link
Member

@dhruvmanila I assume this is due to today's limitation where ruff doesn't parse out anything coming after the magic command because it could, literally, be anything (a shell command). Do you know if this is documented somewhere, if not, should we add it to the FAQ section?

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Mar 18, 2024

That’s not true for all magics, some contain statements. E.g. %time only has a statement following, and %timeit is defined like

%timeit [-n<N> -r<R> [-t|-c] -q -p<P> -o] statement

So the statement could be parsed out.

Apart from %time and %timeit, the same applies only to %prun and %debug if I didn’t miss anything.

So all in all not too daunting.

@dhruvmanila
Copy link
Member

I assume this is due to today's limitation where ruff doesn't parse out anything coming after the magic command because it could, literally, be anything (a shell command).

Yes, that's correct.

Do you know if this is documented somewhere, if not, should we add it to the FAQ section?

I don't think it's documented anywhere except for in my comment here (#8354 (comment)).

We did introduce support for transparent cell magics (#8911), which means we only ignore the line where the magic command is defined, not the subsequent lines which might contain valid Python code.

That’s not true for all magics, some contain statements. E.g. %time only has a statement following, and %timeit is defined like

Thank you for pointing that out. We could potentially support selectively parsing out the code after a line magic but only if it doesn't contain any options. This means that we could support %time but not %timeit for the reasons mentioned in the linked comment above.

Apart from %time and %timeit, the same applies only to %prun and %debug if I didn’t miss anything.

%prun would be difficult because it can contain options, and the same goes for %debug, although it only has a single option (--breakpoint).

I quickly scanned through the line magic lists and the ones you've mentioned are the only ones which can have a statement or expression after the magic command.

While we could potentially support %time, it would necessitate significant changes in the AST representation to store the parsed-out statement or expression, as well as adjustments to the semantic model and any affected lint rules. However, I don't believe it's worth the effort for a single command, especially one typically used as a one-off command.

@dhruvmanila
Copy link
Member

We should update the documentation to make this clear.

@dhruvmanila dhruvmanila added documentation Improvements or additions to documentation and removed rule Implementing or modifying a lint rule labels Mar 19, 2024
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Mar 19, 2024

but only if it doesn't contain any options

why add this limitation if we could just parse the options?

@charliermarsh
Copy link
Member

We can do that, but it requires writing a parser for the options and keeping that up-to-date with any changes in Jupyter.

@flying-sheep
Copy link
Contributor Author

That could end up being easy, depending on two factors:

  1. are we able to reliably grab a magic expression from the Python parser and then call a function on the resulting slice? Then we can just run one of Rust’s existing high-quality CLI parsers, let it tell us how long the prefix of the slice is that’s options, and then try to parse the remainder as Python.
  2. Does jupyter change these magics frequently enough that that’s a concern? And even if so, they probably just add options, so we could simply wait for someone running into a problem and then updating the option parser.

@charliermarsh
Copy link
Member

I support doing it, it's just work that someone needs to do :)

@dhruvmanila
Copy link
Member

One concern with this approach, and maybe it's not really a big problem, is that how do we differentiate between an option and something which looks similar to an option in the Python statement (%timeit -n 10 x-n).

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Mar 20, 2024

By doing what I proposed 😉

there's no ambiguity if we know which options exist and which ones take a parameter and which ones don't.

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

No branches or pull requests

5 participants