diff --git a/Makefile b/Makefile index 493828571b7..6813c875953 100644 --- a/Makefile +++ b/Makefile @@ -9,12 +9,14 @@ lint-ruff-budget lint-ruff-budget-update lint-budget-update lint-gate \ install-dev install-proxy-dev install-test-deps install-hooks \ install-helm-unittest check-circular-imports check-import-safety pre-commit \ - lint-install lint-fetch-base bootstrap + lint-install lint-fetch-base bootstrap bootstrap-python bootstrap-dashboard # Default target help: @echo "Available commands:" @echo " make bootstrap - Provision a fresh clone/worktree" + @echo " make bootstrap-python - Provision only the Python env (no dashboard npm install)" + @echo " make bootstrap-dashboard - Provision only the dashboard's node_modules" @echo " make install-dev - Install development dependencies" @echo " make install-proxy-dev - Install proxy development dependencies" @echo " make install-dev-ci - Install dev dependencies (CI-compatible, pins OpenAI)" @@ -72,17 +74,24 @@ info: install-dev: $(UV) sync --inexact --frozen -bootstrap: +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 - cd ui/litellm-dashboard && ../../scripts/with_dashboard_node.sh npm install --no-audit --no-fund @main_root=$$(git worktree list --porcelain | head -1 | sed 's/^worktree //'); \ if [ "$$main_root" != "$$(git rev-parse --show-toplevel)" ] && [ -f "$$main_root/.env" ] && [ ! -f .env ]; then \ cp "$$main_root/.env" .env && echo "bootstrap: copied .env from $$main_root"; \ else \ echo "bootstrap: .env left untouched"; \ fi - @echo "bootstrap: done" + +bootstrap-dashboard: + cd ui/litellm-dashboard && ../../scripts/with_dashboard_node.sh npm install --no-audit --no-fund install-proxy-dev: $(UV) sync --frozen --group proxy-dev --extra proxy @@ -240,7 +249,7 @@ lint-dev: lint-format-changed check-circular-imports check-import-safety # test-linting.yml (Python), test-litellm-ui-build.yml's frontend-lint (dashboard), and # check-ui-api-types.yml (API-type drift), skipping any whose files you didn't stage. # Not auto-installed as a git hook so it never slows an unrelated human commit. -pre-commit: bootstrap +pre-commit: bootstrap-python ./scripts/pre_commit_lint.sh # Testing targets diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index af8335e0e84..ad39a5fe550 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -98,9 +98,12 @@ 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 node scripts/check-lint-budgets.mjs "$report" eslint-budgets.json || rc=1 - rm -f "$report" exit $rc ) } @@ -156,6 +159,15 @@ 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 +fi + dashboard_checks() { echo "pre-commit: linting dashboard (prettier + eslint + lint budgets)" if [ ! -d ui/litellm-dashboard/node_modules ]; then diff --git a/scripts/ruff_strict_gate.py b/scripts/ruff_strict_gate.py index 507077ddf25..70adcce508d 100644 --- a/scripts/ruff_strict_gate.py +++ b/scripts/ruff_strict_gate.py @@ -10,6 +10,7 @@ branch point (the merge-base). """ import argparse +import functools import json import re import shutil @@ -77,18 +78,27 @@ 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: + name = Path(filename) + return ( + (name if name.is_absolute() else REPO_ROOT / name) + .resolve() + .relative_to(REPO_ROOT) + .as_posix() + ) + + def head_violations() -> list: - out = [] - for item in _ruff_json(REPO_ROOT, STRICT_CONFIG): - name = Path(item["filename"]) - rel = ( - (name if name.is_absolute() else REPO_ROOT / name) - .resolve() - .relative_to(REPO_ROOT) - .as_posix() + return [ + Violation( + _relative_to_repo(item["filename"]), item["location"]["row"], item["code"] ) - out.append(Violation(rel, item["location"]["row"], item["code"])) - return out + for item in _ruff_json(REPO_ROOT, STRICT_CONFIG) + ] def count_by_rule(violations: list) -> dict: diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index c9f774c6113..ccfb1d93d9e 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -24,6 +24,18 @@ set by construction; re-syncs of an up-to-date env are a near-instant no-op. The group set is folded into the cache and artifact fingerprint, so counts recorded under a different set are never matched, only recomputed. +Checking is parallelized across a *pinned* number of worker threads rather than +the host's core count. Threading is what makes the pass roughly 1.6x faster +(137s -> 87s over this tree on four cores), but partitioning files across +threads also reorders a handful of order-dependent inferences, so a few +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. + 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 process OOMs at the ~4 GB default, and when callers had to remember the flag, @@ -52,6 +64,7 @@ carries an unambiguous ``rule`` field. import argparse import contextlib +import functools import hashlib import io import json @@ -91,6 +104,10 @@ 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. UNCODED = "" @@ -107,6 +124,10 @@ 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: path = Path(raw) absolute = path if path.is_absolute() else root / path @@ -217,9 +238,10 @@ def run_basedpyright(cwd: Path = REPO_ROOT, env_dir: Path = TYPECHECK_ENV_DIR) - the only pin that works, because basedpyright auto-detects a `.venv` in the project root and that beats both PATH order and VIRTUAL_ENV, silently measuring the caller's fatter venv (whose extra typed packages flip - diagnostics) whenever the repo has one. Exit 0 (clean) and 1 (errors - found) are both output-bearing runs; anything else is a crash and fails - loudly instead of reading as zero errors.""" + diagnostics) whenever the repo has one. `--threads` is pinned for the same + reason: it is a measurement parameter, not a local tuning knob. Exit 0 + (clean) and 1 (errors found) are both output-bearing runs; anything else is + a crash and fails loudly instead of reading as zero errors.""" bin_dir: Final = env_dir / "bin" proc = subprocess.run( [ @@ -227,6 +249,8 @@ def run_basedpyright(cwd: Path = REPO_ROOT, env_dir: Path = TYPECHECK_ENV_DIR) - "--outputjson", "--pythonpath", str(bin_dir / "python"), + "--threads", + str(BASEDPYRIGHT_THREADS), ], cwd=cwd, capture_output=True, @@ -301,6 +325,7 @@ def over_ceiling( def environment_fingerprints( dep_groups: tuple[str, ...] = TYPECHECK_DEP_GROUPS, + threads: int = BASEDPYRIGHT_THREADS, ) -> tuple[str, ...]: return ( *( @@ -309,6 +334,7 @@ def environment_fingerprints( if path.exists() ), "groups:" + ",".join(dep_groups), + f"threads:{threads}", ) diff --git a/scripts/type_discipline_gate.py b/scripts/type_discipline_gate.py index cc97ce0f46e..b019dca1500 100644 --- a/scripts/type_discipline_gate.py +++ b/scripts/type_discipline_gate.py @@ -28,6 +28,7 @@ otherwise be misread as "fixed", collapsing the deliberate headroom to zero. """ import argparse +import functools import json import re import shutil @@ -88,21 +89,30 @@ 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: + name = Path(filename) + full = name if name.is_absolute() else root / name + return full.resolve().relative_to(root).as_posix() + + def _check(root: Path, checker: Path) -> list: # Resolve root first: on macOS tempfile dirs (/var/...) resolve to /private/var/..., # and the checker prints already-resolved absolute paths, so relative_to would fail. - root = root.resolve() - out = _run([sys.executable, str(checker), str(root / TARGET)], cwd=root) - found = [] - for line in out.splitlines(): - m = _LINE.match(line) - if m is None: - continue - name = Path(m.group("file")) - full = name if name.is_absolute() else root / name - rel = full.resolve().relative_to(root).as_posix() - found.append(Violation(rel, int(m.group("line")), m.group("code"))) - return found + resolved_root = root.resolve() + out = _run([sys.executable, str(checker), str(resolved_root / TARGET)], cwd=resolved_root) + return [ + Violation( + _relative_to_root(m.group("file"), resolved_root), + int(m.group("line")), + m.group("code"), + ) + for line in out.splitlines() + if (m := _LINE.match(line)) is not None + ] def head_violations() -> list: diff --git a/tests/test_litellm/test_pre_commit_lint.py b/tests/test_litellm/test_pre_commit_lint.py index 274d3c517f6..4230bfd110f 100644 --- a/tests/test_litellm/test_pre_commit_lint.py +++ b/tests/test_litellm/test_pre_commit_lint.py @@ -29,6 +29,7 @@ BARRIER_HELPER = """barrier_sync() { MAKE_STUB = """#!/bin/sh . "$STUB_BIN/barrier.sh" +[ -n "${STUB_MAKE_LOG:-}" ] && echo "$*" >> "$STUB_MAKE_LOG" case "$*" in lint) [ "${STUB_FAIL:-}" = "make-lint" ] && exit 1 @@ -85,7 +86,7 @@ def _write_executable(path: Path, body: str) -> None: path.chmod(0o755) -def _sandbox(tmp_path: Path) -> tuple[Path, Path]: +def _sandbox(tmp_path: Path, stage: tuple[str, ...] = (".",)) -> tuple[Path, Path]: repo = tmp_path / "repo" (repo / "litellm" / "proxy").mkdir(parents=True) (repo / "litellm" / "foo.py").write_text("x = 1\n") @@ -95,7 +96,7 @@ def _sandbox(tmp_path: Path) -> tuple[Path, Path]: (dashboard / "node_modules").mkdir() (dashboard / "src" / "app.ts").write_text("export {}\n") subprocess.run(["git", "init", "-q"], cwd=repo, check=True) - subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "add", *stage], cwd=repo, check=True) bin_dir = tmp_path / "bin" bin_dir.mkdir() @@ -148,6 +149,29 @@ def test_all_blocks_passing_exits_zero(tmp_path: Path) -> None: assert proc.returncode == 0, proc.stdout + proc.stderr +def _make_invocations(tmp_path: Path, repo: Path, bin_dir: Path) -> list[str]: + log = tmp_path / "make.log" + proc = _run(repo, bin_dir, {"STUB_MAKE_LOG": str(log)}) + assert proc.returncode == 0, proc.stdout + proc.stderr + return log.read_text().split() + + +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 + assert "bootstrap-dashboard" not in invocations + + +@pytest.mark.parametrize("staged", ["ui/litellm-dashboard/src/app.ts", "litellm/proxy/spec.py"]) +def test_a_commit_that_reaches_the_dashboard_provisions_it(tmp_path: Path, staged: str) -> None: + repo, bin_dir = _sandbox(tmp_path, stage=(staged,)) + assert "bootstrap-dashboard" in _make_invocations(tmp_path, repo, bin_dir) + + 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 abdeb6feecc..45b1f710d55 100644 --- a/tests/test_litellm/test_ruff_strict_gate.py +++ b/tests/test_litellm/test_ruff_strict_gate.py @@ -16,6 +16,15 @@ def rule(name, limit): return {name: {"limit": 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"} + assert gate._relative_to_repo.cache_info().misses == 1 + + def test_under_ceiling_passes(): assert gate.evaluate({"ANN001": 100}, {"ANN001": 100}, rule("ANN001", 110)) == [] diff --git a/tests/test_litellm/test_type_check_gate.py b/tests/test_litellm/test_type_check_gate.py index d104e4ca0c8..282110a8f15 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -46,6 +46,18 @@ def test_basedpyright_error_without_a_rule_is_bucketed(): assert gate.count_basedpyright(payload) == {gate.UNCODED: 1} +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( + {"generalDiagnostics": [_bpr(same_file, "error", "reportAny")] * 50} + ) + assert gate.count_basedpyright(payload) == {"reportAny": 50} + assert gate._to_relative.cache_info().misses == 1 + + def test_paths_outside_repo_are_skipped(): payload = json.dumps( { @@ -122,6 +134,20 @@ def test_run_basedpyright_pins_import_resolution_to_the_owned_env(tmp_path): assert argv[argv.index("--pythonpath") + 1] == str(env_dir / "bin" / "python") +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, + f'echo "$@" > "{captured}"\necho \'{{"generalDiagnostics": []}}\'', + ) + gate.run_basedpyright(cwd=tmp_path, env_dir=env_dir) + argv = captured.read_text().split() + assert argv[argv.index("--threads") + 1] == str(gate.BASEDPYRIGHT_THREADS) + + def test_run_basedpyright_fails_loudly_on_a_crash_exit_code(tmp_path): import pytest @@ -275,9 +301,14 @@ def test_fingerprints_carry_the_dependency_group_set(): assert gate.environment_fingerprints( dep_groups=("proxy-dev",) ) != gate.environment_fingerprints(dep_groups=("proxy-dev", "e2e-dev")) - assert gate.environment_fingerprints()[-1] == "groups:" + ",".join( - gate.TYPECHECK_DEP_GROUPS - ) + assert "groups:" + ",".join(gate.TYPECHECK_DEP_GROUPS) in gate.environment_fingerprints() + + +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) def test_fingerprints_cover_the_prisma_schema(): diff --git a/tests/test_litellm/test_type_discipline_gate.py b/tests/test_litellm/test_type_discipline_gate.py index 1832668e7c3..ddab1e3e77d 100644 --- a/tests/test_litellm/test_type_discipline_gate.py +++ b/tests/test_litellm/test_type_discipline_gate.py @@ -19,6 +19,17 @@ def _budget(limit): return {"LIT006": {"limit": 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") + assert {gate._relative_to_root(same_file, root) for _ in range(20)} == {"litellm/a.py"} + assert gate._relative_to_root.cache_info().misses == 1 + + def test_over_ceiling_flags_only_counts_above_the_limit(): budget = _budget(12) assert gate.over_ceiling({"LIT006": 12}, budget) == frozenset() # at limit