-
-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[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
Changes from 7 commits
a18f7aa
887817a
2f8021d
07c9557
0d74097
5498b2c
b8d8537
c9caa1c
43697a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # doc: https://github.com/pytorch/test-infra/blob/main/tools/stronghold/docs/bc_linter_config.md | ||
| version: 1 | ||
| paths: | ||
| # We temporarily disable globally, and will only enable with `annotations.include` | ||
| # include: | ||
| # - "vllm/v1/attetion/*.py" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo here?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is intentional, we want to disable globally and pilot on few functions/dataclasses to bootstrap |
||
| # - "vllm/v1/core/*.py" | ||
| exclude: | ||
| - "**/*.py" | ||
|
|
||
| scan: | ||
| functions: true # check free functions and methods | ||
| classes: true # check classes/dataclasses | ||
| public_only: true # ignore names starting with "_" at any level | ||
|
|
||
| annotations: | ||
| include: # decorators that force‑include a symbol | ||
| - name: "bc_linter_include" # matched by simple name or dotted suffix | ||
| propagate_to_members: false # for classes, include methods/inner classes | ||
| exclude: # decorators that force‑exclude a symbol | ||
| - name: "bc_linter_skip" # matched by simple name or dotted suffix | ||
| propagate_to_members: true # for classes, exclude methods/inner classes | ||
|
|
||
| excluded_violations: [] # e.g. ["ParameterRenamed", "FieldTypeChanged"] | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,26 @@ | ||||||||||||||
| name: BC Lint | ||||||||||||||
|
|
||||||||||||||
| on: | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||
| pull_request: | ||||||||||||||
| types: | ||||||||||||||
| - opened | ||||||||||||||
| - synchronize | ||||||||||||||
| - reopened | ||||||||||||||
|
|
||||||||||||||
| jobs: | ||||||||||||||
| bc_lint: | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in fact we need this in all of our actions 😆 |
||||||||||||||
| if: github.repository_owner == 'vllm-project' | ||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||
| steps: | ||||||||||||||
| - name: Run BC Lint Action | ||||||||||||||
| uses: pytorch/test-infra/.github/actions/bc-lint@main | ||||||||||||||
| with: | ||||||||||||||
| 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') }} | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note that using labels like |
||||||||||||||
| docs_link: 'https://github.com/pytorch/test-infra/wiki/BC-Linter' | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe create a vllm-specific doc and point this to it? |
||||||||||||||
|
|
||||||||||||||
| concurrency: | ||||||||||||||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} | ||||||||||||||
| cancel-in-progress: true | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | ||
| # vllm/_bc_linter.py | ||
| from __future__ import annotations | ||
|
|
||
| from typing import Any, Callable, TypeVar, overload | ||
|
|
||
| T = TypeVar("T") | ||
|
|
||
|
|
||
| @overload | ||
| def bc_linter_skip(obj: T) -> T: | ||
| ... | ||
|
|
||
|
|
||
| @overload | ||
| def bc_linter_skip(*, reason: str | None = ...) -> Callable[[T], T]: | ||
| ... | ||
|
|
||
|
|
||
| def bc_linter_skip(obj: Any = None, *, reason: str | None = None): | ||
| """ | ||
| No-op decorator to mark symbols/files for BC-linter suppression. | ||
|
|
||
| Usage: | ||
| @bc_linter_skip | ||
| def legacy_api(...): ... | ||
| """ | ||
|
|
||
| def _wrap(x: T) -> T: | ||
| return x | ||
|
|
||
| return _wrap if obj is None else obj | ||
|
|
||
|
|
||
| @overload | ||
| def bc_linter_include(obj: T) -> T: | ||
| ... | ||
|
|
||
|
|
||
| @overload | ||
| def bc_linter_include(*, reason: str | None = ...) -> Callable[[T], T]: | ||
| ... | ||
|
|
||
|
|
||
| def bc_linter_include(obj: Any = None, *, reason: str | None = None): | ||
| """ | ||
| Usage: | ||
| @bc_linter_include | ||
| def public_api(...): ... | ||
| """ | ||
|
|
||
| def _wrap(x: T) -> T: | ||
| return x | ||
|
|
||
| return _wrap if obj is None else obj | ||
|
|
||
|
|
||
| __all__ = ["bc_linter_skip", "bc_linter_include"] |
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.
can we move this out of root dir? and move it into say
.github