[Bootcamp][lint] Add usort to the lintrunner linters list #80257
Conversation
Summary: Adding [usort](https://pypi.org/project/usort/) linter to lintrunner to ensure the imports are sorted consistently. Using https://github.com/pytorch/torchrec as the reference for the linter integration. Test Plan: lintrunner init lintrunner lintrunner f lintrunner -a lintrunner torch/jit/_script.py torch/jit/_trace.py <img width="930" alt="Lintrunner results screenshot" src="https://user-images.githubusercontent.com/108101595/175748204-0f675c99-6122-4ace-be1d-075ab9142a82.png"> Reviewers: osalpekar, suo Subscribers: osalpekar, suo Tasks: T123437457 Tags: lint, usort, bootcamp
🔗 Helpful links
❌ 2 New FailuresAs of commit bf98801 (more details on the Dr. CI page): Expand to see more
🕵️ 2 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
|
I'm not sure that reformatting the whole codebase is a good idea. Perhaps, it's possible to configure CI to run |
| is_formatter = true | ||
|
|
||
| [[linter]] | ||
| code = 'USORT' |
There was a problem hiding this comment.
my impression was that usort and black would fight with one another depending on which one would run first, hence why stuff like ufmt exists.
Can we do a similar thing? e.g. have a PYFMT linter that first runs usort, then runs black
There was a problem hiding this comment.
I'll look into it.
There was a problem hiding this comment.
@joshuadeng, you've replaced ufmt with lintrunner in torchrec, could you please provide any insight into whether there is any interference between black and usort when called from lintrunner?
There was a problem hiding this comment.
I never implemented applying suggestions to the same file from two different linters: https://github.com/suo/lintrunner/blob/main/src/lib.rs#L55-L59
So it's probably better to combine them into a single linter, since I think quite frequently they will both suggest changes.
We don't do this in general, as for some linters (e.g. mypy) it introduces inconsistency in the results between PR CI and trunk CI. My recommendation would be to run it on the codebase and commit a big codemod PR after confirming that changes in import order didn't have weird side effects. |
|
|
||
|
|
||
| [tool.usort] | ||
| first_party_detection = false |
There was a problem hiding this comment.
curious: what motivated this config?
There was a problem hiding this comment.
I decided to add it, as both torchrec and torchvision use it, so I thought pytorch should follow their style for consistency.
Here is some information on what this setting does.
This reworks [80257](#80257) a bit to use ufmt: * ufmt https://ufmt.omnilib.dev/en/stable/ unifies both black and usort to automatically format the file in the "most Pythonic" way * Also make a demo run for all files in `tools/linter/**/*.py` Pull Request resolved: #81157 Approved by: https://github.com/suo
Summary: This reworks [80257](#80257) a bit to use ufmt: * ufmt https://ufmt.omnilib.dev/en/stable/ unifies both black and usort to automatically format the file in the "most Pythonic" way * Also make a demo run for all files in `tools/linter/**/*.py` Pull Request resolved: #81157 Approved by: https://github.com/suo Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/ef035d083ea8ec1048f3f90562e9e761cc6a32ac Reviewed By: DanilBaibak Differential Revision: D37781945 Pulled By: huydhn fbshipit-source-id: e5a0037f670a5276e3fc9eb22881fcf6b5d0063e
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
3 participants
Summary:
Adding usort linter to lintrunner to ensure the imports are sorted consistently. Using https://github.com/pytorch/torchrec as the reference for the linter integration.
Test Plan:
lintrunner init
lintrunner
lintrunner f
lintrunner -a
lintrunner torch/jit/_script.py torch/jit/_trace.py
Reviewers: osalpekar, suo
Subscribers: osalpekar, suo
Tasks: T123437457
Tags: lint, usort, bootcamp