-
Notifications
You must be signed in to change notification settings - Fork 169
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
add ruff lint rule to remove unused imports via ruff #969
Conversation
jimexist
commented
Sep 28, 2024
•
edited
Loading
edited
- include stricter linter rules for the allowlisted files
- add more comments
- use python 11 for ruff run
- fix ruff to 0.6.8 to make it consistent
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/969
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9006929 with merge base c0a81f9 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
So for linting feel free to add a new file or folder here and make all the changes you need https://github.com/pytorch/ao/blob/main/ruff.toml It's tricky for us to merge something like this since eveyrone would need to rebase their code |
thank you for the reference. that makes sense. |
Ok cool! Can you remove unused imports in files erroring in CI and we can merge this |
3356a2a
to
822a79c
Compare
turns out the ruff linter is configured to:
my change is to include a stricter linter rule but only applies to the allowlist, kind of between 1 and 2 |
- name: Analyzing the code with ruff | ||
run: | | ||
ruff check . | ||
- name: Check all Python files for syntax errors (E999) and undefined vars (F821) | ||
- name: Check *all* Python files for F821, F823, and W191 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e999 was deprecated and by default applied
@@ -25,13 +25,18 @@ jobs: | |||
- name: Install dependencies | |||
run: | | |||
python -m pip install --upgrade pip | |||
pip install ruff | |||
pip install ruff==0.6.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule set changes with versions so this is to avoid sudden breakage if they release a newer version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drisspg any preference on this?
Cause btw now locally vs remotely there'll be ruff deviations https://github.com/pytorch/ao/blob/main/dev-requirements.txt#L22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine; I think we just need to also pin in dev-requirments.txt
I kept it loose since it's a pretty common dependency and didn't want to thrash environments, but pinning is better practice
.github/workflows/ruff_linter.yml
Outdated
@@ -15,7 +15,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python-version: ["3.9"] | |||
python-version: ["3.11"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python-version: ["3.11"] | |
python-version: ["3.9"] |
4b402fb
to
9006929
Compare
remove unused imports via ruff