Skip to content

[Misc] Using ruff-format for smaller sets of directories #14485

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Misc] Using ruff-format for smaller sets of directories #14485

wants to merge 2 commits into from

Conversation

aarnphm
Copy link
Collaborator

@aarnphm aarnphm commented Mar 8, 2025 •
edited by github-actions bot
Loading

We will incrementally introduce ruff-format into the codebase, and slowly deprecate yapf and isort.

Currently, follows #14492 delta for directories options.

  • csrc
  • vllm/assets
  • vllm/inputs
  • vllm/multimodal
  • vllm/usage

Signed-off-by: Aaron Pham [email protected]

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@aarnphm aarnphm requested a review from DarkLight1337 March 8, 2025 09:49
@DarkLight1337
Copy link
Member

After editing the pre-commit config, I suggest running pre-commit with --all-files option so we know which files will actually be updated.

@aarnphm
Copy link
Collaborator Author

aarnphm commented Mar 8, 2025

After editing the pre-commit config, I suggest running pre-commit with --all-files option so we know which files will actually be updated.

Yep I make a separate commit, so that we can revert it if needed, but yeah it changes a lot of files.

@mergify mergify bot added documentation Improvements or additions to documentation ci/build frontend multi-modality Related to multi-modality (#4194) structured-output speculative-decoding v1 labels Mar 8, 2025
@DarkLight1337
Copy link
Member

I think we can follow #13971 and only update testing/new code to use ruff format.

@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from 1d9808f to 8d3bc6f Compare March 8, 2025 09:56
@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from fd782bc to 75b8ea9 Compare March 17, 2025 02:15
@mergify Mergify
Copy link

mergify bot commented Mar 17, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @aarnphm.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Mar 17, 2025
@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from 95c38d9 to 652d9e5 Compare March 17, 2025 02:46
@mergify mergify bot removed the needs-rebase label Mar 17, 2025
@mergify Mergify
Copy link

mergify bot commented Mar 17, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @aarnphm.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Mar 17, 2025
@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from 652d9e5 to f88458a Compare March 17, 2025 04:54
@mergify mergify bot removed the needs-rebase label Mar 17, 2025
@mergify Mergify
Copy link

mergify bot commented Mar 17, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @aarnphm.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Mar 17, 2025
@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from 4895846 to 2a229ad Compare March 17, 2025 05:10
@mergify mergify bot removed the needs-rebase label Mar 17, 2025
@DarkLight1337
Copy link
Member

Just to get the configs in, can you open a separate PR that changes one of the smaller directories?

@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from 2a229ad to b68e254 Compare March 17, 2025 06:12
@aarnphm aarnphm changed the title [Misc] Unify formatter and linter to use ruff [Misc] Added ruff format to pre-commit for smaller directories Mar 17, 2025
@aarnphm aarnphm changed the title [Misc] Added ruff format to pre-commit for smaller directories [Misc] Using ruff-format for smaller sets of directories Mar 17, 2025
@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch 2 times, most recently from 758376d to 6c6f719 Compare March 17, 2025 06:54
hmellor
hmellor reviewed Mar 17, 2025
View reviewed changes
.pre-commit-config.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

ruff's isort currently also ignores these directories

.pre-commit-config.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could this exclude be moved to the pyproject.toml? Generally we prefer to avoid configuration that only works if the formatting tools are run via pre-commit (i.e. we would like format on save to work correctly)

.pre-commit-config.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could this be move to pyproject.toml? Reason is same as above.

.pre-commit-config.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Is this line necessary?

pyproject.toml Outdated
Comment on lines +60 to +61
Copy link
Member

Choose a reason for hiding this comment

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

  1. 4 is the default indent width anyway
  2. The docs for target-version say to use
[project]
requires-python ...

which we are already doing

Suggested change
indent-width = 4
target-version = "py39"

pyproject.toml Outdated
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

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

These are also defaults so do not need to be specified

Suggested change
quote-style = "double"
indent-style = "space"

pyproject.toml Outdated
Comment on lines +120 to +122
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this we should adopt the black line length of 88. Currently this file sets it to 80

@aarnphm aarnphm force-pushed the chore/unify-formatter-ruff branch from 6c6f719 to 81f3904 Compare March 19, 2025 11:03
@mergify Mergify
Copy link

mergify bot commented Mar 19, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @aarnphm.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Mar 19, 2025
@aarnphm aarnphm closed this May 26, 2025
@aarnphm aarnphm deleted the chore/unify-formatter-ruff branch May 26, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmellor hmellor hmellor left review comments

@DarkLight1337 DarkLight1337 Awaiting requested review from DarkLight1337 DarkLight1337 is a code owner

Assignees
No one assigned
Labels
ci/build documentation Improvements or additions to documentation misc multi-modality Related to multi-modality (#4194) needs-rebase speculative-decoding v1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aarnphm @DarkLight1337 @russellb @hmellor