Skip to content

Add bc decorator #6949

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 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions tools/stronghold/src/api/ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,28 @@ def extract(path: pathlib.Path) -> Mapping[str, api.Parameters]:
* ClassName.method_name
* ClassName.SubClassName.method_name
"""
raw_api = extract_raw(path)
return {
name: _function_def_to_parameters(function_def)
for name, function_def in raw_api.items()
}
api_map, _ = extract_all(path)
return api_map


def extract_raw(path: pathlib.Path) -> Mapping[str, ast.FunctionDef]:
"""Extracts the API as ast.FunctionDef instances."""
_, raw = extract_all(path)
return raw

def extract_all(
path: pathlib.Path,
) -> tuple[Mapping[str, api.Parameters], Mapping[str, ast.FunctionDef]]:
"""Extracts both parsed parameters and raw ``ast.FunctionDef`` nodes."""
out: dict[str, ast.FunctionDef] = {}
_ContextualNodeVisitor(out, context=[]).visit(
ast.parse(path.read_text(), os.fspath(path))
)
return out

api_map = {
name: _function_def_to_parameters(function_def)
for name, function_def in out.items()
}
return api_map, out

def _function_def_to_parameters(node: ast.FunctionDef) -> api.Parameters:
"""Converts an ast.FunctionDef to api.Parameters."""
Expand Down
59 changes: 57 additions & 2 deletions tools/stronghold/src/api/compatibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import pathlib
import tempfile
from collections.abc import Iterable, Mapping, Sequence
import ast

import api
import api.ast
Expand Down Expand Up @@ -68,13 +69,25 @@ def check(
before: pathlib.Path, after: pathlib.Path
) -> Sequence[api.violations.Violation]:
"""Identifies API compatibility issues between two files."""
before_api = api.ast.extract(before)
after_api = api.ast.extract(after)
before_api, before_raw = api.ast.extract_all(before)
after_api, after_raw = api.ast.extract_all(after)

disabled_funcs = {
name
for name, node in before_raw.items()
if _decorator_disables(node)
} | {
name
for name, node in after_raw.items()
if _decorator_disables(node)
}

violations: list[api.violations.Violation] = []
for name, before_def in before_api.items():
if any(token.startswith("_") for token in name.split(".")):
continue
if name in disabled_funcs:
continue

after_def = after_api.get(name)
if after_def is None:
Expand Down Expand Up @@ -320,3 +333,45 @@ def _check_type_compatibility(
return False

return True


def _decorator_disables(node: ast.FunctionDef) -> bool:
Copy link
Contributor

@izaitsevfb izaitsevfb Jul 23, 2025

Choose a reason for hiding this comment

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

Looks like the current decorator behavior is "opt-out" — the default behavior (public function is checked) is changed only when:

@bc_linter.check_compat(enable=False)

Is this the intended behavior? If yes, can't we just simplify the opt-out syntax to something like @bc_linter.skip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"""Returns True if the bc_linter.check_compat decorator disables checks."""

for deco in node.decorator_list:
name = _decorator_name(deco)
if name == "bc_linter.skip":
return True
if name != "bc_linter.check_compat":
continue

enable = True
if isinstance(deco, ast.Call):
# Look for keyword argument ``enable`` first
for kw in deco.keywords:
if kw.arg == "enable" and isinstance(kw.value, ast.Constant):
enable = bool(kw.value.value)
break
else:
if len(deco.args) == 1 and isinstance(deco.args[0], ast.Constant):
enable = bool(deco.args[0].value)

return not enable

return False


def _decorator_name(expr: ast.expr) -> str | None:
"""Returns dotted name of decorator if easily determined."""

if isinstance(expr, ast.Call):
expr = expr.func

if isinstance(expr, ast.Name):
return expr.id
if isinstance(expr, ast.Attribute):
value = _decorator_name(expr.value)
if value is None:
return None
return value + "." + expr.attr
return None
34 changes: 34 additions & 0 deletions tools/stronghold/tests/api/test_compatibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import api.violations
import pytest
from testing import git, source
import tests.bc_linter_example as bc_linter


def test_deleted_function(tmp_path: pathlib.Path) -> None:
Expand Down Expand Up @@ -520,3 +521,36 @@ def will_be_deleted():
api.violations.FunctionDeleted(func="will_be_deleted", line=1)
],
}


def test_check_disable_decorator(tmp_path: pathlib.Path) -> None:
@bc_linter.skip
def func(x: int) -> None:
pass # pragma: no cover

before = source.make_file(tmp_path, func)

@bc_linter.skip
def func(x: int, y: int) -> None: # type: ignore[no-redef]
pass # pragma: no cover

after = source.make_file(tmp_path, func)

assert api.compatibility.check(before, after) == []

def test_check_enable_decorator(tmp_path: pathlib.Path) -> None:
@bc_linter.check_compat(enable=True)
def func(x: int) -> None:
pass # pragma: no cover

before = source.make_file(tmp_path, func)

@bc_linter.check_compat(enable=True)
def func(x: int, y: int) -> None: # type: ignore[no-redef]
pass # pragma: no cover

after = source.make_file(tmp_path, func)

assert api.compatibility.check(before, after) == [
api.violations.ParameterNowRequired(func=func.__name__, parameter="y", line=2)
]
25 changes: 25 additions & 0 deletions tools/stronghold/tests/bc_linter_example.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
"""Utilities for marking API compatibility checks."""

from __future__ import annotations

from typing import Callable, TypeVar, Any

F = TypeVar("F", bound=Callable[..., Any])


def check_compat(*, enable: bool = True) -> Callable[[F], F]:
"""Decorator used by stronghold to toggle API compatibility checks.

When ``enable`` is ``False`` the decorated function will be skipped by the
backward compatibility linter.
"""

def decorator(func: F) -> F:
# Not used in the linter, but useful for debugging.
setattr(func, "_bc_linter_enable", enable)
return func

return decorator

# Alias decorator to unconditionally disable the backward compatibility linter.
skip: Callable[[F], F] = check_compat(enable=False)
13 changes: 13 additions & 0 deletions tools/stronghold/tests/bc_linter_vllm.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# BC Linter for vLLM
Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed with @izaitsevfb offline, we will try to make bc decorator more generalized by:

  • adding support for custom rules, where users can provide code path, black/white list
  • explore checking bc in classes

PR: https://github.com/vllm-project/vllm/pull/21234

## Code Path
Cover the following code path:
- vllm/v1/attetion/**
- vllm/v1/core/**

Additionally, we should have flexibility to cover other code path in the future.

## Lint Rules
- Check backward compatibility for dataclasses/functions defined python files in code path above
- The default behavior for linter is to check all the dataclasses/public functions in the code path, but we provide an option to skip bc-linter for some experimental dataclasses/functions with `@bc_linter_skip` decorator
Loading