Skip to content

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 <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
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.

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@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)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@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?

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
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?)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
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
westbrook-ai pushed a commit to westbrook-ai/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 <19981378+hmellor@users.noreply.github.com>
Signed-off-by: 0xThresh.eth <0xthresh@protonmail.com>
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