-
Notifications
You must be signed in to change notification settings - Fork 102
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
base: main
Are you sure you want to change the base?
Add bc decorator #6949
Changes from 2 commits
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 |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import pathlib | ||
import tempfile | ||
from collections.abc import Iterable, Mapping, Sequence | ||
import ast | ||
|
||
import api | ||
import api.ast | ||
|
@@ -71,10 +72,25 @@ def check( | |
before_api = api.ast.extract(before) | ||
after_api = api.ast.extract(after) | ||
|
||
before_raw = api.ast.extract_raw(before) | ||
after_raw = api.ast.extract_raw(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: | ||
|
@@ -320,3 +336,43 @@ def _check_type_compatibility( | |
return False | ||
|
||
return True | ||
|
||
|
||
def _decorator_disables(node: ast.FunctionDef) -> bool: | ||
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. Looks like the current decorator behavior is "opt-out" — the default behavior (public function is checked) is changed only when:
Is this the intended behavior? If yes, can't we just simplify the opt-out syntax to something like 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. Yes, I was checking https://github.com/search?q=repo%3Apytorch%2Fpytorch%20torch.fx._compatibility.compatibility&type=code, but adding skip is a very good idea! |
||
"""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.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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
"""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: | ||
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. If I'm understanding the intention here correctly, this decorator should be defined in the client code (unless we require bc-linter clients to import its codebase in runtime, which we should not!). The current implementation appears to support this (i.e. any decorator in VLLM codebase that sets I'd suggest to change this:
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. Oh sounds good, I was thinking about importing this actually, but write the logic in client code makes more sense actually, thanks for the suggestion! |
||
setattr(func, "_bc_linter_enable", enable) | ||
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. hm, actually, looks like |
||
return func | ||
|
||
return decorator | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# BC Linter for vLLM | ||
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. discussed with @izaitsevfb offline, we will try to make bc decorator more generalized by:
|
||
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 |
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.
extract
already callsextract_raw
internally.Ideally instead of parsing AST two times we need to refactor
extract
andextract_raw
to:alternatively, some use some kind of (short-term) memoization?