From 2ee8adf242c6b462e3d50f16e22826f897a4a543 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 10:55:40 +0000 Subject: [PATCH] revert(pre-commit): drop basedpyright --threads from the type-check gate Threading looked like the big win here and did not hold up. Three problems, any one of which disqualifies it: It changes the answer. Partitioning files across threads reorders order-dependent inferences, so the tree totals move with the width (149330 serial, 149325 threaded). A gate whose whole job is counting violations must not have its verdict depend on the host. There is no portable width. Pinning a constant keeps counts comparable but tunes the gate for one machine. Letting it follow nproc is correct per host, yet the width is in the cache fingerprint, so every distinct core count gets its own key and nobody can reuse CI's precomputed base artifact. Everyone then pays the cold path. The speed was noise. On four cores, --threads 4 ranged 84-114s against 137-151s serial, a spread that swallows the median I originally quoted, and on a contributor's laptop width 8 ran 724s against 113s serial, roughly 6x slower, while making the machine unusable. Serial restores the original cache key, so the existing base entry and CI artifact stay valid. The surviving win in this branch is the memoized path resolution, which is measurable in isolation and host-independent. --- scripts/type_check_gate.py | 37 ++++++++-------------- tests/test_litellm/test_type_check_gate.py | 14 ++++---- 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index 8c5bf63c0e0..6f05ed1e5cd 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -24,19 +24,17 @@ 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 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. +Checking runs single-threaded on purpose. basedpyright's ``--threads`` looks +like free speed and is not: partitioning files across threads reorders +order-dependent inferences, so the tree's totals move with the width (149330 +serial, 149325 threaded here), which makes a counting gate's verdict a function +of the host. Pinning a width to keep counts comparable then decouples it from +the machine, and letting it follow ``nproc`` instead gives every core count its +own fingerprint, so no one could reuse CI's precomputed base artifact. The +speed did not survive measurement either: on four cores ``--threads 4`` ranged +84-114s against 137-151s serial, and on one contributor's laptop it was ~6x +*slower* than serial (724s vs 113s at width 8). Serial is the only width that +is both portable and comparable. 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 @@ -106,8 +104,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" -BASEDPYRIGHT_THREADS: Final = 4 - # Bucket for a basedpyright diagnostic with no `rule`. Counted so it's gated. UNCODED = "" @@ -240,10 +236,9 @@ 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. `--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.""" + 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.""" bin_dir: Final = env_dir / "bin" proc = subprocess.run( [ @@ -251,8 +246,6 @@ 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, @@ -327,7 +320,6 @@ def over_ceiling( def environment_fingerprints( dep_groups: tuple[str, ...] = TYPECHECK_DEP_GROUPS, - threads: int = BASEDPYRIGHT_THREADS, ) -> tuple[str, ...]: return ( *( @@ -336,7 +328,6 @@ def environment_fingerprints( if path.exists() ), "groups:" + ",".join(dep_groups), - f"threads:{threads}", ) diff --git a/tests/test_litellm/test_type_check_gate.py b/tests/test_litellm/test_type_check_gate.py index fc6d3f76ad4..40ce0f93ebf 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -132,15 +132,18 @@ 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): +def test_run_basedpyright_stays_single_threaded(tmp_path): + """`--threads` shifts the tree's totals (149330 serial vs 149325 threaded), which + would make this counting gate's verdict a function of the host, and it measured + slower than serial on real hardware. Neither failure is visible from a passing + run, so pin the absence of the flag.""" 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) + assert "--threads" not in captured.read_text().split() def test_run_basedpyright_fails_loudly_on_a_crash_exit_code(tmp_path): @@ -299,11 +302,6 @@ def test_fingerprints_carry_the_dependency_group_set(): assert "groups:" + ",".join(gate.TYPECHECK_DEP_GROUPS) in gate.environment_fingerprints() -def test_fingerprints_carry_the_thread_width(): - 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(): schema_hash = hashlib.sha256(gate.PRISMA_SCHEMA.read_bytes()).hexdigest() assert schema_hash in gate.environment_fingerprints()