-
-
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?
Changes from all commits
a18f7aa
887817a
2f8021d
07c9557
0d74097
5498b2c
b8d8537
c9caa1c
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" | ||
# - "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: | ||||||||||||||
pull_request: | ||||||||||||||
types: | ||||||||||||||
- opened | ||||||||||||||
- synchronize | ||||||||||||||
- reopened | ||||||||||||||
|
||||||||||||||
jobs: | ||||||||||||||
bc_lint: | ||||||||||||||
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
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') }} | ||||||||||||||
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' | ||||||||||||||
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
|
||||||||||||||
|
||||||||||||||
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.
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.