Skip to content

[1/N][CI] Move linting system to pre-commits hooks #1256

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 27 commits into
base: main
Choose a base branch
from

Conversation

Potabk
Copy link
Contributor

@Potabk Potabk commented Jun 17, 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.

@github-actions github-actions bot added documentation Improvements or additions to documentation ci/build module:tests module:ops module:tools and removed documentation Improvements or additions to documentation module:tests module:ops labels Jun 17, 2025
@Potabk Potabk force-pushed the lint-2 branch 2 times, most recently from 3313968 to b6b8bf3 Compare June 17, 2025 09:08
@github-actions github-actions bot added documentation Improvements or additions to documentation module:tests module:ops labels Jun 17, 2025
@Potabk Potabk force-pushed the lint-2 branch 2 times, most recently from 0ff0fa9 to e196c98 Compare June 18, 2025 01:35
@Potabk
Copy link
Contributor Author

Potabk commented Jun 18, 2025

cc @Yikun @wangxiyuan

Copy link

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

@Yikun
Copy link
Collaborator

Yikun commented Jun 19, 2025

c8bb90f

  • Update format.sh to have two cmd format.sh and format.sh ci
  • Update the vllm-ascend doc
  • Add some note

After consideration, I didn't add mypy lint to format.sh, but added format.sh ci

@Yikun
Copy link
Collaborator

Yikun commented Jun 19, 2025

Need a rebase after #1293 resolved

Potabk added 7 commits June 20, 2025 01:03
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Potabk and others added 20 commits June 20, 2025 01:03
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: Yikun Jiang <[email protected]>
@@ -18,8 +18,6 @@
name: 'test'

on:
schedule:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we remove the schedule here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, currently this is a per PR check, we can enable push trigger in future

@Yikun Yikun added the ready read for review label Jun 20, 2025
@github-actions github-actions bot added merge-conflicts and removed ready read for review labels Jun 20, 2025
Copy link

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

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