From 23b4a674ca528a1da883ffb3ef720c35c0d5bedb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 10:02:52 +0000 Subject: [PATCH] refactor(pre-commit): move added rationale out of source comments Greptile flagged the explanatory comments this branch added as a violation of the repo convention against new source comments, and it is right. The Python rationale moves into docstrings on the three memoized helpers, which is what the rest of these gates already use, and the thread-width constant is described in the module docstring that documents every other measurement parameter. The Makefile and shell comments go entirely. The one constraint a comment was carrying that nothing else did, that the dashboard is provisioned once rather than once per node block, is now a test instead: it fails if the provisioning moves inside dashboard_checks and genapi_checks, where two npm installs would race in the same directory. The header line listing what a dashboard-staged commit runs is updated rather than removed, so it does not go stale. --- Makefile | 3 --- scripts/pre_commit_lint.sh | 8 +------- scripts/ruff_strict_gate.py | 8 +++++--- scripts/type_check_gate.py | 18 ++++++++++-------- scripts/type_discipline_gate.py | 8 +++++--- tests/test_litellm/test_pre_commit_lint.py | 9 ++++++--- tests/test_litellm/test_ruff_strict_gate.py | 2 -- tests/test_litellm/test_type_check_gate.py | 7 ------- .../test_litellm/test_type_discipline_gate.py | 3 --- 9 files changed, 27 insertions(+), 39 deletions(-) diff --git a/Makefile b/Makefile index 6813c875953..39713a9f724 100644 --- a/Makefile +++ b/Makefile @@ -77,9 +77,6 @@ install-dev: bootstrap: bootstrap-python bootstrap-dashboard @echo "bootstrap: done" -# The halves are separate targets so a caller can provision only what it needs: -# `make pre-commit` on a Python-only change takes bootstrap-python and never pays -# for (nor hard-fails on) the dashboard's node toolchain. bootstrap-python: $(UV) sync --inexact --frozen --extra proxy --group proxy-dev --group e2e-dev $(UV_RUN) python scripts/prisma_generate_if_needed.py diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index ad39a5fe550..f8ddb528729 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -6,7 +6,7 @@ # - litellm/ Python staged -> `make lint` (test-linting.yml's lint job) # - tests/e2e Python staged -> `make lint-e2e-basedpyright` (test-linting.yml's e2e type-check step) # + raw HTTP client ban (test-code-quality.yml's check_e2e_no_raw_requests) -# - dashboard staged -> prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint) +# - dashboard staged -> `make bootstrap-dashboard` + prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint) # - proxy/types staged -> regenerate dashboard API types and fail on drift (check-ui-api-types.yml) # # Each block is skipped when no matching files are staged, so unrelated commits stay @@ -98,8 +98,6 @@ EOF # counts are not diff-scoped, so a local pass here means the budget step will # pass in CI too. report=$(mktemp) - # Ctrl-C reaches this job as a SIGTERM from on_interrupt; bash skips EXIT - # traps on an uncaught fatal signal, so catch it and exit through one. trap 'rm -f "$report"' EXIT trap 'exit 130' INT TERM npx eslint . -f json -o "$report" || true @@ -159,10 +157,6 @@ if [ -n "$e2e_py_files" ]; then || { echo "✗ Raw HTTP client import in tests/e2e. Route the call through tests/e2e/e2e_http.py, then re-run make pre-commit." >&2; status=1; } fi -# `make pre-commit` provisions the Python env only, so top up the dashboard's -# node_modules for the commits that reach it. Placed after the Python block forked -# (so the install overlaps that lint) and before both node blocks fork (so two npm -# installs never race in the same directory). if [ -n "$ui_prettier_files" ] || [ -n "$ui_eslint_files" ] || [ -n "$spec_files" ]; then echo "pre-commit: provisioning the dashboard toolchain (make bootstrap-dashboard)" make bootstrap-dashboard || status=1 diff --git a/scripts/ruff_strict_gate.py b/scripts/ruff_strict_gate.py index 70adcce508d..ada845e8a1b 100644 --- a/scripts/ruff_strict_gate.py +++ b/scripts/ruff_strict_gate.py @@ -78,11 +78,13 @@ def _ruff_json(cwd: Path, config: Path) -> list: return json.loads(raw or "[]") -# Memoized because `resolve()` is a filesystem round trip and ruff reports many -# more violations than there are files: the cache makes it one realpath walk per -# file rather than one per violation. @functools.lru_cache(maxsize=None) def _relative_to_repo(filename: str) -> str: + """`filename` as a repo-relative path. + + Memoized because `resolve()` is a filesystem round trip and ruff reports far + more violations than there are files, so the cache makes it one realpath walk + per file rather than one per violation.""" name = Path(filename) return ( (name if name.is_absolute() else REPO_ROOT / name) diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index ccfb1d93d9e..8c5bf63c0e0 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -32,9 +32,11 @@ diagnostics differ between a serial pass, a two-thread pass and a four-thread pass; passes at the same width agree exactly, run after run. Width is therefore part of the measurement in exactly the way the installed package set is, and letting it follow ``nproc`` would make a 16-core laptop and a 4-core CI runner -report different totals for the same tree. It is a constant here and is folded -into the fingerprint alongside the group set, so a cache entry or CI artifact -recorded at another width is never matched, only recomputed. +report different totals for the same tree. It is the constant +``BASEDPYRIGHT_THREADS`` here, set to the GitHub-hosted runner's vCPU count so +CI gets full parallelism, and it is folded into the fingerprint alongside the +group set, so a cache entry or CI artifact recorded at another width is never +matched, only recomputed. The gate runs basedpyright itself, for both the head and the base pass, with ``NODE_OPTIONS`` raised to the heap this repo needs: basedpyright's node @@ -104,8 +106,6 @@ PRISMA_SCHEMA = REPO_ROOT / "litellm" / "proxy" / "schema.prisma" # caller-set value while preserving the caller's other NODE_OPTIONS flags. NODE_HEAP_OPTION = "--max-old-space-size=8192" -# Worker threads every pass is measured at. Four is the GitHub-hosted runner's -# vCPU count, so CI gets full parallelism and any dev box measures what CI does. BASEDPYRIGHT_THREADS: Final = 4 # Bucket for a basedpyright diagnostic with no `rule`. Counted so it's gated. @@ -124,11 +124,13 @@ class Breach(NamedTuple): added: int -# Memoized because it is called once per diagnostic, and `resolve()` is a -# filesystem round trip: this tree reports ~149k errors across ~2.2k files, so -# the cache turns ~149k realpath walks into one per file (~6s -> ~0.3s). @functools.lru_cache(maxsize=None) def _to_relative(raw: str, root: Path) -> str | None: + """`raw` as a path relative to `root`, or None when it falls outside. + + Memoized because it is called once per diagnostic while `resolve()` is a + filesystem round trip: this tree reports ~149k errors across ~2.2k files, so + the cache turns ~149k realpath walks into one per file (6.6s -> 0.9s).""" path = Path(raw) absolute = path if path.is_absolute() else root / path try: diff --git a/scripts/type_discipline_gate.py b/scripts/type_discipline_gate.py index b019dca1500..f39fa4b3b1a 100644 --- a/scripts/type_discipline_gate.py +++ b/scripts/type_discipline_gate.py @@ -89,11 +89,13 @@ def resolve_base_point(base_ref: str, cwd: Path = REPO_ROOT) -> str: return merge_point if older == head_point else head_point -# Memoized because `resolve()` is a filesystem round trip and the checker reports -# many more violations than there are files: the cache makes it one realpath walk -# per file rather than one per violation. @functools.lru_cache(maxsize=None) def _relative_to_root(filename: str, root: Path) -> str: + """`filename` as a path relative to `root`. + + Memoized because `resolve()` is a filesystem round trip and the checker + reports far more violations than there are files, so the cache makes it one + realpath walk per file rather than one per violation.""" name = Path(filename) full = name if name.is_absolute() else root / name return full.resolve().relative_to(root).as_posix() diff --git a/tests/test_litellm/test_pre_commit_lint.py b/tests/test_litellm/test_pre_commit_lint.py index 4230bfd110f..7efbc8c83be 100644 --- a/tests/test_litellm/test_pre_commit_lint.py +++ b/tests/test_litellm/test_pre_commit_lint.py @@ -157,9 +157,6 @@ def _make_invocations(tmp_path: Path, repo: Path, bin_dir: Path) -> list[str]: def test_a_python_only_commit_never_provisions_the_dashboard_toolchain(tmp_path: Path) -> None: - # npm install costs seconds this commit has no use for, and on a machine whose - # node predates the dashboard's engines floor it fails outright, which used to - # block `make pre-commit` for changes that never touch the dashboard. repo, bin_dir = _sandbox(tmp_path, stage=("litellm/foo.py",)) invocations = _make_invocations(tmp_path, repo, bin_dir) assert "lint" in invocations @@ -172,6 +169,12 @@ def test_a_commit_that_reaches_the_dashboard_provisions_it(tmp_path: Path, stage assert "bootstrap-dashboard" in _make_invocations(tmp_path, repo, bin_dir) +def test_the_dashboard_is_provisioned_once_when_both_node_blocks_run(tmp_path: Path) -> None: + repo, bin_dir = _sandbox(tmp_path) + invocations = _make_invocations(tmp_path, repo, bin_dir) + assert invocations.count("bootstrap-dashboard") == 1 + + def test_full_output_is_saved_to_a_log_file_in_the_git_dir(tmp_path: Path) -> None: repo, bin_dir = _sandbox(tmp_path) (repo / "scratch.txt").write_text("") diff --git a/tests/test_litellm/test_ruff_strict_gate.py b/tests/test_litellm/test_ruff_strict_gate.py index 45b1f710d55..5ca9613e650 100644 --- a/tests/test_litellm/test_ruff_strict_gate.py +++ b/tests/test_litellm/test_ruff_strict_gate.py @@ -17,8 +17,6 @@ def rule(name, limit): def test_paths_are_resolved_once_per_file_not_once_per_violation(): - # resolve() is a filesystem round trip and ruff reports ~20k strict violations - # over far fewer files, so resolving per violation is pure overhead. gate._relative_to_repo.cache_clear() same_file = str(gate.REPO_ROOT / "litellm" / "a.py") assert {gate._relative_to_repo(same_file) for _ in range(20)} == {"litellm/a.py"} diff --git a/tests/test_litellm/test_type_check_gate.py b/tests/test_litellm/test_type_check_gate.py index 282110a8f15..fc6d3f76ad4 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -47,8 +47,6 @@ def test_basedpyright_error_without_a_rule_is_bucketed(): def test_counting_resolves_each_file_once_not_once_per_diagnostic(): - # resolve() is a filesystem round trip and this tree reports ~149k errors over - # ~2.2k files, so a resolve per diagnostic costs ~6s of the gate for nothing. gate._to_relative.cache_clear() same_file = ROOT / "litellm" / "a.py" payload = json.dumps( @@ -135,9 +133,6 @@ def test_run_basedpyright_pins_import_resolution_to_the_owned_env(tmp_path): def test_run_basedpyright_pins_the_thread_width_instead_of_inheriting_the_hosts(tmp_path): - # Partitioning files across threads reorders a few order-dependent inferences, - # so counts are only comparable at equal width; letting it follow the core - # count would make a 16-core laptop and a 4-core runner disagree on one tree. captured = tmp_path / "argv.txt" env_dir = _stub_env( tmp_path, @@ -305,8 +300,6 @@ def test_fingerprints_carry_the_dependency_group_set(): def test_fingerprints_carry_the_thread_width(): - # Same reasoning as the group set: a count taken at another width is not - # comparable, so its cache entry and artifact name must not be reachable. assert f"threads:{gate.BASEDPYRIGHT_THREADS}" in gate.environment_fingerprints() assert gate.environment_fingerprints(threads=2) != gate.environment_fingerprints(threads=4) diff --git a/tests/test_litellm/test_type_discipline_gate.py b/tests/test_litellm/test_type_discipline_gate.py index ddab1e3e77d..75962b1223a 100644 --- a/tests/test_litellm/test_type_discipline_gate.py +++ b/tests/test_litellm/test_type_discipline_gate.py @@ -20,9 +20,6 @@ def _budget(limit): def test_paths_are_resolved_once_per_file_not_once_per_violation(): - # resolve() is a filesystem round trip and the checker reports tens of - # thousands of violations over ~2.2k files, so resolving per violation is - # pure overhead on the pre-commit and CI critical path. gate._relative_to_root.cache_clear() root = gate.REPO_ROOT.resolve() same_file = str(root / "litellm" / "a.py")