Enable ruff in lintrunner by justinchuby · Pull Request #99280 · pytorch/pytorch

justinchuby · 2023-04-16T23:56:18Z

This change implements the ruff linter in pytorch lintrunner. It is adapted from https://github.com/justinchuby/lintrunner-adapters/blob/main/lintrunner_adapters/adapters/ruff_linter.py. It does both linting and fixing.

This change also migrated all flake8 configs to the ruff config.

We plan to enable ruff in torch.onnx.

Fixes #94737

@huydhn @Skylion007

`🤖 Generated by Copilot at 8537d06`

Summary

🧹,🛠️,🚨,

This pull request adds support for the ruff code formatter and linter to the pytorch project. It modifies the .lintrunner.toml and pyproject.toml files to configure the ruff tool, and adds two new adapter files to the tools/linter/adapters directory to run the ruff tool with or without autofix capabilities using the lintrunner tool. The purpose of this pull request is to introduce a new code style and quality standard for the pytorch codebase, and to enable developers to easily check and fix their code using the lintrunner tool.

Ruff lints and formats
Improving pytorch codebase
Autumn of quality

Walkthrough

Add ruff formatter and linter to pytorch code quality checks (link, link, link, link)

Update .lintrunner.toml to include RUFF and RUFF-FIX linters with code patterns, commands, and options (link)
Add tool.ruff section to pyproject.toml to specify target Python version for ruff (link)
Create ruff_fix_linter.py and ruff_linter.py in tools/linter/adapters to run ruff commands and return lint messages (link, link)

cc @soumith @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @desertfire

pytorch-bot · 2023-04-16T23:56:21Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99280

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

ROCm jobs are failing with No space left on device

❌ 45 Failures

As of commit c8fb114:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Skylion007 · 2023-04-17T16:22:10Z

This is fantastic as it would eventually allow adding a lot of autofixes from flake8 fixes and could potentially subsume other lint checks in a faster framework. (It would also allow us to individually enable checks from pyupgrade which would be useful).

I split ruff into two because I couldn't find a good way to make lintrunner display inline messages and produce a fix at the same time.

Have you opened an issue on ruff? I am sure if there is not this behavior in ruff yet, it could be easily added and would reduce our maintenance, and require 1 fewer run of ruff. Are the --show-fixes and --show-source flags insufficient for use case?

Skylion007 · 2023-04-17T16:24:17Z

I would actually go as far as trying to enable it globally if possible as we should at least be able to run all the checks we have enabled in our flake8 rules.

justinchuby · 2023-04-17T16:29:26Z

Have you opened an issue on ruff? I am sure if there is not this behavior in ruff yet, it could be easily added and would reduce our maintenance, and require 1 fewer run of ruff. Are the --show-fixes and --show-source flags insufficient for use case?

I think I just haven't figured out how to parse to outputs properly, since the fixed code and the violations will appear together in stdout? I understand lintrunner needs to read the fixed file from a pipe and the fix cannot happen inplace.

Skylion007 · 2023-04-17T19:54:49Z

Have you opened an issue on ruff? I am sure if there is not this behavior in ruff yet, it could be easily added and would reduce our maintenance, and require 1 fewer run of ruff. Are the --show-fixes and --show-source flags insufficient for use case?

I think I just haven't figured out how to parse to outputs properly, since the fixed code and the violations will appear together in stdout? I understand lintrunner needs to read the fixed file from a pipe and the fix cannot happen inplace.

You can have ruff output it's violations in JSON format, just use that: https://beta.ruff.rs/docs/configuration/#command-line-interface

justinchuby · 2023-04-17T20:41:02Z

I used that. Looks like I need to figure out how to apply the edits from the json properly. Do you have any suggestions?

Skylion007 · 2023-04-17T21:05:04Z

I used that. Looks like I need to figure out how to apply the edits from the json properly. Do you have any suggestions?

Can't you just allow ruff to do it with --fix allow, or does lintrunner not allow that?

Skylion007 · 2023-04-17T21:08:23Z

I used that. Looks like I need to figure out how to apply the edits from the json properly. Do you have any suggestions?

You can reference the ruff-iSP server code. I think it applies to the edit from the JSON in a Python framework. See this relevant PR that touches the relevant code path: astral-sh/ruff-lsp#90

huydhn · 2023-04-18T00:29:16Z

I quickly look at ruff documentation. Here are some thoughts:

lintrunner expects a LintMessage with the original and the replacement text https://docs.rs/lintrunner/latest/lintrunner/lint_message/struct.LintMessage.html from the adapter. It will handle the rest. So in this case, the ruff_fix_linter seems out of place as it actually does the formatting. I agree that lintrunner and ruff doesn't fit together well here.
ruff --diff shows the diff. Is there a way to take that diff output and apply it to the original text read from the file to get the replacement text? That could be a work-around

I had a similar issue integrating lintrunner with ufmt before and needed to use uftm Python API to get it working https://github.com/pytorch/pytorch/blob/main/tools/linter/adapters/ufmt_linter.py#L67-L78. It looks like that people is also asking for the same API from ruff here astral-sh/ruff#659. That will probably be the solution

justinchuby · 2023-04-18T02:21:15Z

We could run two passes: one using ruff on all file to get the lints (fast), the second using treads to get replacement texts for each file (10x slower but still fast, a few seconds). This way we don't need to manually parse and apply the patches from the json, ensuring accuracy. The total runtime for this will be a few seconds.

The current ruff_fix_linter is a similar implementation as black.

Alternatively as @Skylion007 pointed out we could run ruff on a files and apply patches manually. This way we only need to use python to read and modified the files with fixes. The challenge would be to ensure correctness of the patch application process. But it will be the fastest way I can see.

justinchuby · 2023-04-22T01:35:24Z

I did the two pass method, and migrated all flake8 configs to the ruff config. It is very fast:

[2023-04-22T01:35:09Z DEBUG lintrunner] Running linters: ["RUFF"]
[2023-04-22T01:35:09Z DEBUG lintrunner::git] Running paths_cmd: git grep -Il .
[2023-04-22T01:35:10Z DEBUG lintrunner::linter] Running linter RUFF: python3 'tools/linter/adapters/ruff_linter.py' '--config=pyproject.toml' '--show-disable' '--' '@/var/folders/59/5mtp2rns59n7n99ndkm5x6gc0000gn/T/.tmp2yDttl'
[2023-04-22T01:35:11Z DEBUG lintrunner::linter] Linter RUFF took: 1.615648916s

justinchuby · 2023-04-22T02:33:46Z

Replaced by #99785

justinchuby · 2023-04-22T02:55:11Z

@Skylion007 enabled in #99785 . Looking good so far! Fixed a few issues flake8 didn't detect

### This change - Implements the ruff linter in pytorch lintrunner. It is adapted from https://github.com/justinchuby/lintrunner-adapters/blob/main/lintrunner_adapters/adapters/ruff_linter.py. It does **both linting and fixing**. 🔧 - Migrated all flake8 configs to the ruff config and enabled it for the repo. ✅ - **`ruff` lints the whole repo in under 2s** 🤯 Fixes #94737 Replaces #99280 huydhn Skylion007  ### <samp>🤖 Generated by Copilot at 6b982dd</samp> ### Summary 🧹🛠️🎨  Add `[tool.ruff]` section to `pyproject.toml` to configure `ruff` code formatter and linter. This change aims to improve code quality and consistency with a single tool. > _`ruff` cleans the code_ > _like a spring breeze in the fields_ > _`pyproject.toml`_ ### Walkthrough * Configure `ruff` code formatter and linter for the whole project ([link](https://github.com/pytorch/pytorch/pull/99785/files?diff=unified&w=0#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R22-R79)) [ghstack-poisoned]

@huydhn

### This change - Implements the ruff linter in pytorch lintrunner. It is adapted from https://github.com/justinchuby/lintrunner-adapters/blob/main/lintrunner_adapters/adapters/ruff_linter.py. It does **both linting and fixing**. 🔧 - Migrated all flake8 configs to the ruff config and enabled it for the repo. ✅ - **`ruff` lints the whole repo in under 2s** 🤯 Fixes #94737 Replaces #99280 @huydhn @Skylion007  ### <samp>🤖 Generated by Copilot at 6b982dd</samp> ### Summary 🧹🛠️🎨  Add `[tool.ruff]` section to `pyproject.toml` to configure `ruff` code formatter and linter. This change aims to improve code quality and consistency with a single tool. > _`ruff` cleans the code_ > _like a spring breeze in the fields_ > _`pyproject.toml`_ ### Walkthrough * Configure `ruff` code formatter and linter for the whole project ([link](https://github.com/pytorch/pytorch/pull/99785/files?diff=unified&w=0#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R22-R79)) Pull Request resolved: #99785 Approved by: https://github.com/malfet, https://github.com/Skylion007

Apparently #78142 made torch.JIT allow for simple generator expressions which allows us to enable rules that replace unnecessary list comprehensions with generators in any/all. This was originally part of #99280 but I split it off into this PR so that it can be easily reverted should anything break. Pull Request resolved: #99890 Approved by: https://github.com/justinchuby, https://github.com/kit1980, https://github.com/malfet

Enable ruff in lintrunner

7a03e5c

justinchuby requested review from Skylion007, ZainRizvi and suo April 16, 2023 23:56

pytorch-bot bot added the topic: not user facing topic category label Apr 16, 2023

justinchuby added better-engineering Relatively self-contained tasks for better engineering contributors module: lint Issues related to our Python/C++ lint rules (run by Travis) labels Apr 16, 2023

pytorchbot added the open source label Apr 17, 2023

Windows

8537d06

justinchuby requested a review from huydhn April 17, 2023 15:17

justinchuby added 3 commits April 21, 2023 18:19

Single ruff!

9b01335

Add ruff rules

6ef9ea1

config

6afb7e3

justinchuby added 2 commits April 21, 2023 19:13

Fix

ae721a1

Merge branch 'main' into justinchu/ruff

0ead571

justinchuby requested review from H-Huang, mrshenli, rohan-varma and zhaojuanmao as code owners April 22, 2023 02:14

github-actions bot added module: dynamo module: inductor release notes: quantization release notes category labels Apr 22, 2023

pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) labels Apr 22, 2023

justinchuby closed this Apr 22, 2023

justinchuby mentioned this pull request Apr 22, 2023

Enable ruff in lintrunner #99785

Closed

Skylion007 mentioned this pull request Apr 24, 2023

[BE] Enable C419 rule for any all shortcircuiting #99890

Closed

Comments

Conversation

justinchuby commented Apr 16, 2023 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Generated by Copilot at 8537d06

Summary

Walkthrough

Uh oh!

pytorch-bot bot commented Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99280

❗ 1 Active SEVs

❌ 45 Failures

Uh oh!

Skylion007 commented Apr 17, 2023

Uh oh!

Skylion007 commented Apr 17, 2023

Uh oh!

justinchuby commented Apr 17, 2023

Uh oh!

Skylion007 commented Apr 17, 2023

Uh oh!

justinchuby commented Apr 17, 2023

Uh oh!

Skylion007 commented Apr 17, 2023

Uh oh!

Skylion007 commented Apr 17, 2023

Uh oh!

huydhn commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinchuby commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinchuby commented Apr 22, 2023

Uh oh!

justinchuby commented Apr 22, 2023

Uh oh!

justinchuby commented Apr 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinchuby commented Apr 16, 2023 •

edited by pytorch-bot bot

Loading

`🤖 Generated by Copilot at 8537d06`

pytorch-bot bot commented Apr 16, 2023 •

edited

Loading

huydhn commented Apr 18, 2023 •

edited

Loading

justinchuby commented Apr 18, 2023 •

edited

Loading