-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
[CI/Build] Add bc-linter to vLLM CI #21234
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
base: main
Are you sure you want to change the base?
Conversation
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
👋 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 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 🚀 |
@@ -0,0 +1,23 @@ | |||
name: BC Lint | |||
|
|||
on: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to only turn on for specific folders only?
i think remind folks of BC breaking changes in core/ and attention/ are generally helpful. while it might feel a bit spammy in other folders.
.github/workflows/bc-lint.yml
Outdated
- vllm/attetion/** | ||
- vllm/core/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v0 is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Can we also update the lint message so folks know how to suppress the check?
cc: @huydhn @seemethere @simon-mo @WoosukKwon to also take a look!
.github/workflows/bc-lint.yml
Outdated
# branches-ignore: | ||
# - <branches to ignore bc-linter> | ||
paths: | ||
- vllm/attetion/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we have all the files covered here to ensure the BC guard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it will check all the files under this path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me, how the BC checking works? Do we need to tag some function to be BC guaranteed?
Discussed offline, will check if we can use some annotations to restrict linter coverage for a selection of tests
@houseroad here is documentation of bc-linter: https://github.com/pytorch/test-infra/wiki/BC-Linter
like some warnings in https://github.com/vllm-project/vllm/pull/21235/files#diff-bafd6bd048c5ca7bad726e6f509fe4409f4033342e2b5b0c3016156e2f708b12 |
repo: ${{ github.event.pull_request.head.repo.full_name }} | ||
base_sha: ${{ github.event.pull_request.base.sha }} | ||
head_sha: ${{ github.event.pull_request.head.sha }} | ||
suppression: ${{ contains(github.event.pull_request.labels.*.name, 'suppress-bc-linter') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that using labels like suppress-bc-linter
is a common self-serve mechanism in PyTorch PR, but we would need a maintainer to do it here, like adding the ready
label, which is ok
- vllm/v1/core/** | ||
|
||
jobs: | ||
bc_lint: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want this line https://github.com/pytorch/pytorch/blob/main/.github/workflows/lint-bc.yml#L19C5-L19C45 to run this job only on PR submitted to vllm
bc_lint: | |
bc_lint: | |
if: github.repository_owner == 'vllm-project' |
base_sha: ${{ github.event.pull_request.base.sha }} | ||
head_sha: ${{ github.event.pull_request.head.sha }} | ||
suppression: ${{ contains(github.event.pull_request.labels.*.name, 'suppress-bc-linter') }} | ||
docs_link: 'https://github.com/pytorch/test-infra/wiki/BC-Linter' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the concurrency rule is important https://github.com/pytorch/pytorch/blob/main/.github/workflows/lint-bc.yml#L31-L33 to avoid multiple bc linter jobs running at the same time on the same PR
docs_link: 'https://github.com/pytorch/test-infra/wiki/BC-Linter' | |
docs_link: 'https://github.com/pytorch/test-infra/wiki/BC-Linter' | |
concurrency: | |
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} | |
cancel-in-progress: true |
Have you been able to run this https://github.com/pytorch/test-infra/blob/main/.github/actions/bc-lint/action.yml#L62-L68 locally? At a high level, the BC linter uses https://docs.python.org/3/library/ast.html to parse the python changes in the PR and check for those changes https://github.com/pytorch/test-infra/blob/main/tools/stronghold/src/api/violations.py. It has some PyTorch-specific logic that might not be applicable to vLLM https://github.com/pytorch/test-infra/blob/main/tools/stronghold/src/api/compatibility.py#L20-L43, but we could easily update that if needed, for example, it ignores anything start with |
Signed-off-by: zhewenli <[email protected]>
Signed-off-by: zhewenli <[email protected]>
Signed-off-by: zhewenli <[email protected]>
Signed-off-by: zhewenli <[email protected]>
Summary:
Add bc-linter to vLLM CI, in this PR we are piloting adding this only to vllm/v1/core/sched/output.py
Config is defined in
.bc-linter.yml
, where https://github.com/pytorch/test-infra/blob/main/tools/stronghold/docs/bc_linter_config.md has more examplesClass support, config support added in pytorch/test-infra#6953, pytorch/test-infra#7016
Purpose
bc-linter(https://github.com/pytorch/test-infra/wiki/BC-Linter) is tool in pytorch test infra to test backward compatibility on public pytorch functions used externally. this PR enables the check in vLLM where we can use
.bc-linter.yml
with@bc_linter_include
/@bc_linter_exclude
annotations to protect bc on functionsTest Plan
Test Plan:
CI
Test Result
https://github.com/vllm-project/vllm/pull/23855/files#diff-cafd89ce8a698a56acb24ada62831cbc7a980782f78a52d1742ba238031f296c
