feat(ci): PR size (non-test churn) pre-commit hook + size-labeling workflow - #445
feat(ci): PR size (non-test churn) pre-commit hook + size-labeling workflow#445arekay-nv wants to merge 5 commits into
Conversation
…orkflow Classify a PR by non-test churn per the Endpoints PR Review Policy: additions + deletions and changed-file count outside tests/**, lockfiles, vendored sources, and generated files, mapped to Normal / Large / Very large (class = higher of the two dimensions). Advisory only -- always exits 0. - scripts/pr_size_check.py: pure classify()/parse_numstat() + git measurement. Terse one-line verdict by default; -v/--verbose adds the threshold grid and per-class review requirements; --github-output emits size_class/churn/files. - pre-commit hook `pr-size-check`: quiet on a normal commit; the breakdown surfaces under `pre-commit run pr-size-check --verbose`. - .github/workflows/pr-size-label.yml: labels PRs size/normal|large|very-large from the same logic. Uses pull_request_target so fork PRs can be labeled, running only the trusted base-repo script over the fetched PR diff (never checks out or executes PR code). - tests/unit/scripts/test_pr_size_check.py: boundary, verbose-gating, and temp-git measurement coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
=======================================
Coverage ? 80.98%
=======================================
Files ? 150
Lines ? 20208
Branches ? 0
=======================================
Hits ? 16366
Misses ? 3842
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Follow-up hardening on the pr-size-check feature: - workflow: drop the inline `gh label create` calls (creating label definitions needs `issues: write`; the job only has `pull-requests: write`). Label application is now warn-only and never fails the advisory check, and stale `size/*` labels are stripped only after the new label applies, so a failed add can't leave a PR unlabeled. Labels are provisioned once out-of-band via `gh label create`. - pre-commit: run the advisory hook with `python3` (not `python`) and set `verbose: true` so the one-line verdict shows on every commit. - size script: exclude root-level `*.lock` (was `**/*.lock`, which git's default pathspec matching never matches against a top-level `uv.lock`). - tests: cover the CI two-ref `--head` path, top-level `*.lock` exclusion, and base-ref auto-detection (candidate scan + `GITHUB_BASE_REF`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nv-alicheng
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Codex + Claude + Code-Quality | Depth: standard
Found 4 issues (0 critical/high). Solid, well-tested tooling PR. Security note: all three reviewers independently cleared the pull_request_target workflow — it checks out the trusted base.sha, fetches the PR head as data only (never executes PR code), validates PR_NUMBER against injection, uses env-var indirection (no untrusted PR fields in run:), minimizes permissions, and pins the action SHA. No token-exfil / script-injection vector found.
| # | File | Line | Severity | Category | Reviewer(s) | Summary |
|---|---|---|---|---|---|---|
| 1 | scripts/pr_size_check.py |
75 | medium | code-quality | Code-Quality | Thresholds 500/1500/20/50 duplicated across classify, the _GRID string, and the docstring — no single source; grid can silently drift from behavior. |
| 2 | scripts/pr_size_check.py |
57 | low | design | Codex | origin/main resolved before upstream/main; stale fork main over-classifies the local hint (documented tradeoff; CI label unaffected). Reconsider default precedence. |
| 3 | scripts/pr_size_check.py |
70 | low | code-quality | Code-Quality | classify() -> str stringly-typed over a closed 3-value set; a Literal would let mypy check _REQUIREMENTS exhaustiveness. |
| 4 | scripts/pr_size_check.py |
129 | low | testing | Claude | Resilience fallbacks (remote-scan base resolution; measure() -> None skip path) untested — the exact CI-degradation paths the design relies on. |
Codex also raised a finding on tests/performance/.../test_token_metrics_perf.py; dropped as out of scope (not in this PR's diff).
|
|
||
| # Base refs tried in order when neither PR_BASE_REF nor GITHUB_BASE_REF is set. | ||
| # Covers the common local-checkout and fork-remote layouts. | ||
| _BASE_CANDIDATES = ("origin/main", "main", "upstream/main") |
There was a problem hiding this comment.
[Codex] low (design): _BASE_CANDIDATES tries origin/main before upstream/main. In the common fork layout (origin = personal fork, upstream = canonical), a stale fork main makes the local pre-commit hint count already-merged commits and over-classify — exactly the contributor workflow this hook exists to help. You already document this tradeoff in the PR description ("Base-branch resolution & forked PRs"), and it never affects the CI label (that measures base.sha directly), so this is not a bug. But since the hint is for contributors and the fork layout is their common case, worth reconsidering whether upstream/main should take precedence by default (or resolving to whichever candidate yields the nearer merge-base).
| if _rev_parse_ok(ref): | ||
| return ref | ||
|
|
||
| for remote in _git(["remote"]).stdout.split(): |
There was a problem hiding this comment.
[Claude] low (testing): the graceful-degradation fallbacks — the whole point of this resilient advisory check — are untested. (1) The remote-scan base fallback here (for remote in _git(["remote"]).stdout.split(): ref = f"{remote}/main", L129-132): every test_main_* resolves the base via PR_BASE_REF/GITHUB_BASE_REF/--base/_BASE_CANDIDATES, so this loop never runs. (2) measure() returning None on a failed merge-base/diff and the corresponding "could not diff … skipping" path in main (L230-235) — test_main_skips_when_base_unresolvable only covers base_ref is None, not the unrelated-history/shallow-clone case. The workflow's "No size class computed … nothing to label" handling depends on these skipping cleanly. A small unit test for each (a repo with only an upstream/main remote; a diff against unrelated history) closes the gap.
Single source of truth for the size thresholds, a typed size class, and
tests for the previously untested graceful-degradation paths.
- Promote thresholds to NORMAL/LARGE_MAX_{LINES,FILES} constants; derive
classify() and the printed _GRID from them, and drop the duplicated
numbers from the module docstring (finding mlcommons#1).
- Add SizeClass = Literal["normal","large","very-large"] and type classify()
plus the build_/write helpers so mypy checks _REQUIREMENTS exhaustiveness
(finding mlcommons#3).
- Test the <remote>/main scan in resolve_base_ref and measure()->None on
unrelated history plus the corresponding main skip path (finding mlcommons#4).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What does this PR do?
Adds a PR-size classifier used two ways, implementing the "CI job for LOC count / label large PRs" item from the Endpoints PR Review Policy:
pr-size-check— prints the one-line size class on every commit (verbose: true). Runpython3 scripts/pr_size_check.py --verbosefor the threshold grid and per-class review requirements..github/workflows/pr-size-label.yml— labels each PRsize/normal | size/large | size/very-largefrom the same logic.Size = non-test churn (additions + deletions and changed-file count) excluding
tests/**, lockfiles, vendored sources, and generated files. Class = the higher of the lines/files dimensions. Advisory only — nothing blocks.Per its own tool, this PR classifies as Normal.
Type of change
Testing
tests/unit/scripts/test_pr_size_check.py(21 cases): classify boundaries, numstat parsing, verbose gating, temp-git measurement,$GITHUB_OUTPUT, the CI two-ref--headpath, top-level*.lockexclusion, and base-ref auto-detection (candidate scan +GITHUB_BASE_REF).Checklist
sched_setaffinityenv issue on unrelated files; green on Linux CI)Notes for reviewers
pull_request_targetsoGITHUB_TOKENcan write labels on fork PRs (a plainpull_requesttoken is read-only for forks). It never checks out or runs PR code — only the trusted base-repo copy ofscripts/pr_size_check.pyover the fetched PR diff.pull_request_targetruns the workflow from the base branch, this very PR won't be auto-labeled; labeling takes effect for PRs opened after this merges.pull-requests: writeto attach labels, whereas creating label definitions needsissues: write; keeping them separate keeps the per-PR token minimally scoped. Create the labels once with:::warning::and never fails the PR, and stalesize/*labels are removed only after the new label is successfully applied (so a failed add never leaves a PR unlabeled).Base-branch resolution & forked PRs
The size class is computed against a base branch. When
--base/PR_BASE_REFare unset,resolve_base_ref()tries, in order:GITHUB_BASE_REF→origin/main→main→upstream/main→ any other<remote>/main.pull_request.base.sha, so the applied label is correct regardless of local remotes.origin= your fork,upstream=mlcommons/endpoints—origin/mainresolves beforeupstream/main. If your fork'sorigin/mainis stale, the local advisory size can count already-merged upstream commits and over-classify. This affects only the local hint, never the CI label. To pin the base locally, either keep your fork'smainsynced with upstream, or set the base explicitly:PR_BASE_REF=upstream/main pre-commit run pr-size-check # or, ad hoc: python3 scripts/pr_size_check.py --base upstream/main --verbose