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

Add pre-commit based linting and formatting #35

Merged
merged 22 commits into from
Jan 29, 2025

Conversation

hmellor
Copy link
Member

@hmellor hmellor commented Jan 29, 2025

The hooks added are as follows:

  • actionlint - GitHub Actions
  • check-{json/toml/yaml} - files with these extensions
  • end-of-file-fixer and trailing-whitespace- all files
  • requirements-txt-fixer - Python requirements files
  • hadolint - Dockerfiles (currently disabled)
  • helmlint - Helm charts (currently disabled)
  • black and isort - Python
  • shellcheck - shell (currently disabled)
  • markdownlint - markdown
  • codespell - spelling

Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Copy link
Collaborator

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

Is it possible to install pre-commit locally, like vLLM? We could document it if it is possible.

@hmellor
Copy link
Member Author

hmellor commented Jan 29, 2025

Is it possible to install pre-commit locally, like vLLM? We could document it if it is possible.

Yes, of course. I added requirements-lint.txt to make this a bit clearer to users. I'll add it to the contributing section of the README too

@hmellor
Copy link
Member Author

hmellor commented Jan 29, 2025

@ApostaC since the repo has been unformatted until now, there are lots of changes that the hooks want to make. Would you prefer if I do this in this PR, or in a follow up? (meaning that this PR would merge with pre-commit failing)

@KuntaiDu
Copy link
Collaborator

I would prefer fixing the pre-commit in this PR to make sure the CI is always green, @ApostaC what's your thought?

ApostaC
ApostaC previously approved these changes Jan 29, 2025
Copy link
Collaborator

@ApostaC ApostaC 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 effort! Looks good to me!

In the meantime, can you add something in the README.md about how to run pre-commit check locally (something like pre-commit run --all-files maybe?)

Copy link
Collaborator

@ApostaC ApostaC left a comment

Choose a reason for hiding this comment

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

LGTM!

@ApostaC ApostaC merged commit 3da6238 into vllm-project:main Jan 29, 2025
4 checks passed
@hmellor hmellor deleted the add-pre-commit branch January 29, 2025 18:03
0xThresh pushed a commit to 0xThresh/vllm-production-stack that referenced this pull request Jan 30, 2025
* Add pre-commit workflow

* Add actionlint

* Add generic hooks

* Add black, isort, shellcheck

* Add requirements and markdown linting

* Add toml

* Add Dockerfile

* Add codespell

* Use Node.js version of `markdownlint`

* Add `requirements-lint.txt`

* Use CLI version of Node.js `markdownlint`

* Add `pre-commit` instructions to `Contributing`

* `pre-commit run -a` automatic fixes

* Exclude helm templates from `check-yaml`

* Comment hooks that require installed tools

* Make `codespell` happy

* Make `actionlint` happy

* Disable `shellcheck` until it can be installed properly

* Make `markdownlint` happy

* Add note about running pre-commit

---------

Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: 0xThresh.eth <[email protected]>
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.

4 participants