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

jupyter: ruff v0.1.4 ignores variables with the same name as magic commands #8526

Closed
njzjz opened this issue Nov 6, 2023 · 7 comments · Fixed by #9653
Closed

jupyter: ruff v0.1.4 ignores variables with the same name as magic commands #8526

njzjz opened this issue Nov 6, 2023 · 7 comments · Fixed by #9653
Assignees
Labels
bug Something isn't working

Comments

@njzjz
Copy link

njzjz commented Nov 6, 2023

This is the previous Jupyter Notebook file, and it had no lint errors with ruff v0.1.3.

image

When ruff was upgraded to v0.1.4 (deepmodeling/dpdata#573), import dpdata got removed,

image

Besides, F821 was complained:

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

docs/nb/try_dpdata.ipynb:cell 4:1:1: F821 Undefined name `system`
docs/nb/try_dpdata.ipynb:cell 5:1:1: F821 Undefined name `system`
docs/nb/try_dpdata.ipynb:cell 6:1:1: F821 Undefined name `system`
Found 3 errors.

It's obvious that ruff doesn't consider variables in the other cells.

@zanieb zanieb added the bug Something isn't working label Nov 6, 2023
@charliermarsh
Copy link
Member

In this case, it's because system is a Jupyter magic which can be used as an automagic. In Jupyter, you can do system ls in a cell (without any percent signs), and it will treat the command as an ls. We started ignoring cells that look like automagics, because it can be hard (impossible) to understand their behavior, since Jupyter will actually treat them differently depending on what other variables are defined, i.e., the order in which you run your cells.

If you use any other variable name, it will work as expected, but we'll consider how to make this more targeted.

@charliermarsh
Copy link
Member

That explanation is probably unclear, here's an image that demonstrates what I'm describing -- the first and third cells are identical, but their behaviors entirely depend on the other of execution:

Screen Shot 2023-11-06 at 3 26 41 PM

@charliermarsh
Copy link
Member

\cc @dhruvmanila since we discussed this a bit at the time

@dhruvmanila
Copy link
Member

Yeah, this is tricky. We could implement the simple solution that we discussed earlier about having support for only pip but then the same problem would appear.

The far fetched solution would be to not ignore such cells and perform some semantic analysis on the parsed code but then it could potentially raise a syntax error (cd ..). For any potential automagic command, the semantic model would probably check if the identifier (system, cd, etc.) is already defined in the scope. If it is, continue as Python code otherwise continue as an automagic.

I'd actually want to spend some time figuring out how IPython does it as it could lead to some insights.

@dhruvmanila
Copy link
Member

So, IPython is doing exactly that: https://github.com/ipython/ipython/blob/de97f0032dd96e0109780396e9219e8d73073e29/IPython/core/prefilter.py#L428-L467

For something like pwd = "foo", the AssignmentChecker will run it normally as Python code and then for any other usage of pwd, the AutoMagicChecker checks if it's shadowed or not and runs it as a magic command accordingly.

@charliermarsh
Copy link
Member

Hmm yeah. We could apply some similar heuristics though they'd need to be more expansive... We could also try parsing, then fallback to automagic if the code doesn't parse, but that will also be wrong in some cases. I'd just really like to avoid doing semantic analysis on the code block to understand how to parse it.

@njzjz njzjz changed the title jupyter: ruff v0.1.4 doesn't consider variables in the other cells jupyter: ruff v0.1.4 ignores variables with the same name as magic commands Nov 8, 2023
@charliermarsh
Copy link
Member

This is fixed by #9653.

@charliermarsh charliermarsh self-assigned this Jan 26, 2024
charliermarsh added a commit that referenced this issue Jan 29, 2024
## Summary

Given a statement like `colors = 6`, we currently treat the cell as an
automagic (since `colors` is an automagic) -- i.e., we assume it's
equivalent to `%colors = 6`. This PR adds some additional detection
whereby if the statement is an _assignment_, we avoid treating it as
such. I audited the list of automagics, and I believe this is safe for
all of them.

Closes #8526.

Closes #9648.

## Test Plan

`cargo test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants