Skip to content

support custom dir#7145

Merged
izaitsevfb merged 3 commits intopytorch:mainfrom
zhewenl:support-custom-dir
Sep 11, 2025
Merged

support custom dir#7145
izaitsevfb merged 3 commits intopytorch:mainfrom
zhewenl:support-custom-dir

Conversation

@zhewenl
Copy link
Copy Markdown
Contributor

@zhewenl zhewenl commented Sep 10, 2025

Summary

This PR adds an option to specify BC linter config path, which will be used in vllm-project/vllm#21234

Test plan

Locally test against PR: vllm-project/vllm#24614

Before:

../test-infra/tools/stronghold/bin/check-api-compatibility --base-commit=b5e383cd8b62975dec605bed05e22d273c296c7a --head-commit=38ce6fa81660f0be6e0e61672fdc499bcd7fabc4

::group::fetch github.event.pull_request.base.sha
From ssh://github.com/zhewenl/vllm
 * branch                b5e383cd8b62975dec605bed05e22d273c296c7a -> FETCH_HEAD
::endgroup::
fatal: path 'vllm/_bc_linter.py' exists on disk, but not in 'b5e383cd8b62975dec605bed05e22d273c296c7a'
::warning file=vllm/v1/core/sched/output.py,line=99::Function CachedRequestData: num_computed_tokens changed from list[int] to list[float]
::warning file=vllm/v1/core/sched/scheduler.py,line=1::Function Scheduler.get_grammar_bitmask: function deleted
::warning file=vllm/v1/core/sched/scheduler.py,line=1::Function Scheduler.update_draft_token_ids: function deleted

After:

../test-infra/tools/stronghold/bin/check-api-compatibility --base-commit=b5e383cd8b62975dec605bed05e22d273c296c7a --head-commit=38ce6fa81660f0be6e0e61672fdc499bcd7fabc4 --config-dir=.github

::group::fetch github.event.pull_request.base.sha
From ssh://github.com/zhewenl/vllm
 * branch                b5e383cd8b62975dec605bed05e22d273c296c7a -> FETCH_HEAD
::endgroup::
BC-linter: Using .bc-linter.yml (parsed successfully)
fatal: path 'vllm/_bc_linter.py' exists on disk, but not in 'b5e383cd8b62975dec605bed05e22d273c296c7a'
::warning file=vllm/v1/core/sched/output.py,line=99::Function CachedRequestData: num_computed_tokens changed from list[int] to list[float]
> ../test-infra/tools/stronghold/bin/check-api-compatibility --base-commit=b5e383cd8b62975dec605bed05e22d273c296c7a --head-commit=38ce6fa81660f0be6e0e61672fdc499bcd7fabc4

Note the difference in scheduler.py, before the we specifying the local config the bc linter yml was not respected - so the default rules are applied and we check ALL python files; after specifying the yml the warnings are restricted to ONLY the functions with annotations

@vercel
Copy link
Copy Markdown

vercel bot commented Sep 10, 2025

@zhewenl is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 10, 2025
def load_config_with_status(repo_root: pathlib.Path) -> tuple[Config, str]:
"""Loads configuration from `.bc-linter.yml` in the given repository root.
def load_config_with_status(
repo_root: pathlib.Path, *, config_dir: pathlib.Path | str | None = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a reason to pass these as two separate params, instead of a single concatenated path to load config from?

def load_config(repo_root: pathlib.Path) -> Config:
"""Loads configuration from `.bc-linter.yml` in the given repository root.
def load_config(
repo_root: pathlib.Path, *, config_dir: pathlib.Path | str | None = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Copy Markdown
Contributor

@izaitsevfb izaitsevfb left a comment

Choose a reason for hiding this comment

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

lgtm overall, but please check the question in the comments

@izaitsevfb izaitsevfb merged commit 79baf2b into pytorch:main Sep 11, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants