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

Migrate linter from pylint to ruff #1665

Merged
merged 9 commits into from
Nov 20, 2023
Merged

Conversation

simon-mo
Copy link
Collaborator

@simon-mo simon-mo commented Nov 14, 2023

The ruleset is roughly the same. ruff actually implement at lot more other rules that's super useful.

ruff yields 300x speedup for linting the files.

(base) xmo@simon-dev-l4x2:~/vllm$ time pylint vllm
real    0m10.963s
user    0m40.860s
sys     0m0.864s
(base) xmo@simon-dev-l4x2:~/vllm$ time ruff .

real    0m0.036s
user    0m0.007s
sys     0m0.052s

Currently I have disabled import sorting rules as the changes are going to be more drastic.

Additionally, ruff can automatically fixes the linting issue, saving developer time: https://docs.astral.sh/ruff/linter/#fixes

Copy link
Member

@zhuohan123 zhuohan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work, Simon! LGTM in general. I have some dumb questions:

  • Does this new style linter strictly follow google python style guide? If not, which standard does it follow?
  • Does this linter forces line width at all? If so, what's the line width it forced?

pyproject.toml Outdated
"E731",
# line too long, handled by black formatting
"E501",
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: new line

@simon-mo
Copy link
Collaborator Author

Does this new style linter strictly follow google python style guide? If not, which standard does it follow?
It follows the styles checks we listed in pyproject.toml: pycodestyle, Pyflakes, flake8-bugbear, flake8-simplify, which I believes to have if not all overlap with Google style.

Does this linter forces line width at all? If so, what's the line width it forced?

No I explicitly turned it down by ignoring error E501. For long line with pure code, the formatter will be able to rewrite it into shorter line, or we can enforce it there. The long line with prompt, we are adding ignore anyway.

@simon-mo simon-mo merged commit 5ffc0d1 into vllm-project:main Nov 20, 2023
2 checks passed
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
sjchoi1 pushed a commit to casys-kaist-internal/vllm that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants