Skip to content

[CI] Move linting system to pre-commits hooks #1241

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Potabk
Copy link
Contributor

@Potabk Potabk commented Jun 16, 2025

What this PR does / why we need it?

  • As the file .pre-commit-config.yaml you can see, we list all the lint check to the file, before submitting your commit, you can run pip install -r requirements-lint.txt to have pre-commit installed and pre-commit install to install the pre-commit hooks, and then linters will run automatically before each commit
  • Made some clean code changes to keep the new CI passing

Does this PR introduce any user-facing change?

How was this patch tested?

@github-actions github-actions bot added documentation Improvements or additions to documentation ci/build module:tools labels Jun 16, 2025
@@ -0,0 +1,24 @@
name: pre-commit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this to vllm_ascend_test.yaml as deps of e2e test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if pre-commits runs parallel with e2e test is batter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

e2e resource is limited so do pre check then run e2e is better

Copy link
Collaborator

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

Please keep format.sh content and lint type to help merge first versioon

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: wangli <wangli858794774@gmail.com>
Potabk added 5 commits June 16, 2025 19:02
Signed-off-by: wangli <wangli858794774@gmail.com>
Signed-off-by: wangli <wangli858794774@gmail.com>
Signed-off-by: wangli <wangli858794774@gmail.com>
Signed-off-by: wangli <wangli858794774@gmail.com>
Signed-off-by: wangli <wangli858794774@gmail.com>
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

wangxiyuan pushed a commit that referenced this pull request Jul 10, 2025
### What this PR does / why we need it?

Follow vllm-project/vllm lint way:
https://github.com/vllm-project/vllm/blob/main/.pre-commit-config.yaml

Enable pre-commit to avoid some low level error  AMAP.

This pr is one step of #1241, The purpose is make linting system more
clear and convenient, on this step, Mainly did the following things:
yapf, actionlint, ruff, typos, isort, mypy, png-lint, signoff-commit,
enforce-import-regex-instead-of-re.

TODO: 
- clang-format(check for csrc with google style)
need clean code, disable for now 
- pymarkdown
need clean code, disable for now 
- shellcheck
need clean code, disable for now 

### Does this PR introduce _any_ user-facing change?

Only developer UX change:

https://vllm-ascend--1256.org.readthedocs.build/en/1256/developer_guide/contributing.html#run-lint-locally

```
pip install -r requirements-lint.txt && pre-commit install
bash format.sh
```

### How was this patch tested?

CI passed with new added/existing test.

Co-authored-by: Yikun [yikunkero@gmail.com](mailto:yikunkero@gmail.com)
Co-authored-by: wangli
[wangli858794774@gmail.com](mailto:wangli858794774@gmail.com)
- vLLM version: v0.9.1
- vLLM main:
vllm-project/vllm@5358cce

---------

Signed-off-by: wangli <wangli858794774@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants