perf(lint): schedule the lint fan-out around its long pole - #36276
perf(lint): schedule the lint fan-out around its long pole#36276mateo-berri wants to merge 9 commits into
Conversation
Profiling `make pre-commit` on a staged litellm/*.py change showed basedpyright was ~90% of it: 137s of a 157s type-check gate, on a run that took ~152s total. Three changes, all measured: Run basedpyright across worker threads instead of one. The width is pinned to a constant rather than following the host's core count, because partitioning files across threads reorders a few order-dependent inferences: serial, 2-thread and 4-thread passes disagree on a handful of diagnostics, while passes at the same width are byte-identical run after run. An auto width would make a 16-core laptop and a 4-core runner report different totals for the same tree, so the width joins the dependency-group set in the environment fingerprint and any cache entry or CI artifact recorded at another width is recomputed, never matched. 137s -> 87s over this tree. Memoize the path resolution in all three budget gates. Each called `Path.resolve()`, a filesystem round trip, once per violation rather than once per file: 149k realpath walks for 2.2k files in the basedpyright gate alone, 6.6s -> 0.9s there, 5.0s -> 2.8s for the ruff strict gate. Split `bootstrap` into `bootstrap-python` and `bootstrap-dashboard`, and have `pre-commit` take only the Python half. The dashboard's npm install is seconds a Python-only commit has no use for, and on a machine whose node predates the dashboard's engines floor it fails outright, which blocked `make pre-commit` entirely for changes that never touch the dashboard. The script now tops the dashboard up itself for the commits that reach it, after the Python block forks so the install overlaps that lint and before both node blocks fork so two npm installs cannot race. Also fixes a temp file the dashboard job leaked on Ctrl-C: bash skips EXIT traps on an uncaught fatal signal, so the eslint report survived the interrupt. Measured over 3 reps each, staged litellm/*.py change, 4-core box: before 178.9 / 148.8 / 152.0s (median 152.0) after 106.2 / 99.2 / 94.8s (median 99.2) Cold path, where the base tree has to be measured too, 5m33s -> 3m17s.
|
|
Greptile SummaryThe PR reorganizes local lint scheduling, conditionally provisions dashboard dependencies, and memoizes repeated path normalization.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| Makefile | Splits bootstrap targets and changes lint setup and fan-out scheduling. |
| scripts/pre_commit_lint.sh | Provisions dashboard dependencies only for scopes that execute Node-based checks. |
| scripts/type_check_gate.py | Memoizes diagnostic path normalization while retaining single-threaded basedpyright execution. |
| scripts/ruff_strict_gate.py | Memoizes Ruff diagnostic path normalization. |
| scripts/type_discipline_gate.py | Memoizes checker path normalization without rebinding the supplied root. |
| tests/test_litellm/test_makefile_lint.py | Adds regression coverage for concurrent setup and longest-first lint ordering. |
| tests/test_litellm/test_pre_commit_lint.py | Covers scoped dashboard provisioning, single provisioning, and failure handling. |
Reviews (4): Last reviewed commit: "test: accept the parallel setup sub-make..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
|
Generated by Claude Code |
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.
Splitting bootstrap introduced a failure path the old target could not reach:
`make bootstrap-dashboard` can now fail on its own while the run continues.
It set status=1 and fell through into the dashboard lint and gen:api blocks
anyway, which then ran against a stale or absent node_modules and printed a
second failure ("format with npm run format") on top of the real cause. On a
box whose node is below the dashboard's engines floor, that misleading advice
is the last thing on screen and the actual reason has scrolled away.
Skip both node blocks when provisioning fails and say so once. The Python
block shares nothing with the node toolchain, so it still runs.
make pre-commit wall time by ~35%`make pre-commit` became `make check` on staging, with a working-tree fallback when nothing is staged, so this branch's three changes move onto that shape: - `check` depends on `bootstrap-python`, not `bootstrap` - the script provisions the dashboard off `ui_prettier_changed` / `ui_eslint_changed` / `spec_files`, matching the new scope variables, so a deleted dashboard file still provisions before the lint that inspects it - the skip-on-failed-provisioning guard rides on the same variables Staging's rewrite of `lint_dashboard` dropped the traps that keep the eslint report from outliving a Ctrl-C, so those come back with the merge.
lint-checks declared its cheapest checks first, so at narrow widths basedpyright queued behind them and pushed the makespan out by their runtime. Declare the prerequisites longest-first instead, measured on this tree: basedpyright 124.4s, type-discipline 34.4s, e2e 7.0s, import-safety 3.7s, gate 3.4s, the rest under a tenth of a second. Cap the fan-out at LINT_JOBS ?= 2 rather than one job per core. Every check other than basedpyright totals 48.6s against its 124.4s, so a single spare slot absorbs all of them and further slots only take cores from the critical path. ?= keeps it overridable and dropping the sysctl/nproc shell-out saves a subprocess per Makefile parse. Run lint-install and lint-fetch-base under their own -j 2 sub-make. One waits on the disk and the other on the network, so paying their sum bought nothing. Nine paired trials, arms alternating: 131.04s before, 127.12s after, 3.0% faster, 8/9 trials favouring after. The floor is basedpyright's 124.4s, so the run is now 2.2% above it.
…laude/pre-commit-performance-m1fspt
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 24 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 24 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
…laude/pre-commit-performance-m1fspt Staging moved the heavy entrypoints behind scripts/gate_slot_lock.py, so `lint` and `check` now only queue for a machine-wide slot and delegate to `lint-inner` / `check-inner`. Keep that indirection and hang this branch's changes off the inner targets: the parallel setup sub-make moves to lint-inner, and check-inner depends on bootstrap-python rather than the full bootstrap. test_lint_overlaps_env_sync_with_base_fetch drives the real `lint` target, so its sandbox now carries gate_slot_lock.py and runs with LITELLM_GATE_SLOTS=0.
The slot-lock test pinned lint-install and lint-fetch-base as declared prerequisites of lint-inner. This branch runs them from lint-inner's recipe instead, under their own -j 2 sub-make, so the pair overlaps while lint-checks keeps its own width. Assert the setup is reached from lint-inner either way, which is what the test is actually for: nothing runs before the slot is held.
|
Last review was two commits back. Since then: a staging merge, plus a test assertion that now accepts the parallel setup sub-make. Generated by Claude Code |
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a contributor with a one-file Python change cannot run
make checkat all unless their node satisfies the dashboard's engines floorgit add litellm/_uuid.pymake checknpm installinui/litellm-dashboard, even though no dashboard file is stagedAfter: the same contributor gets a full lint run, on two jobs rather than every core, and node's version has nothing to do with a change that never touches the dashboard
git add litellm/_uuid.pymake checknpm installrunsRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review)
Screenshots / Proof of Fix
This PR changes no runtime surface, so there is no proxy route to curl and no LLM call to bill. What a contributor runs here is
make check, so that command is the proof, alongside themake lintfan-out underneath it. Both sides ran back to back on the same 4-core box with 15 GB RAM and node 22.22.2, which sits below the dashboard's engines floor:$ nproc; node --version 4 v22.22.2The
make checkcase has exactly one Python file staged and nothing else, on both sides:The
make lintcase runs on a clean tree on both sides. Both arms resolve the same merge base,973329e98, and both found its basedpyright counts already cached on disk, so neither one pays the cold recompute. The two runs below are one trial each, captured minutes apart, so read them as a demonstration rather than as the measurement. The multi-trial paired figures are in the benchmark sectionBefore (973329e)
make check with one Python file staged
npm install, node is too old for it, and the run stops there having linted nothing:make lint on a clean tree
After (7671b79)
make check with one Python file staged
npm installruns, and the lint pass the before arm never reached completes:make lint on a clean tree
-j 2sub-make, and the checks fan out at two jobs with basedpyright declared first:Type
🚄 Infrastructure
Benchmark
Method
One 4-core box, 15 GB RAM, node 22.22.2, warm caches, same tree throughout. Both arms were captured against
340ee50f, differing only in theMakefileunder test: the before arm restores that file to its base-branch scheduling (nprocwidth, serial setup, cheap checks declared first) and the after arm is this PR's. Holding the tree fixed and swapping only the scheduling is the point, since anything else would change what the linters actually doOne disclosure about the after arm. The
Makefileit ran hadcheck-import-safetydeclared ahead oflint-e2e-basedpyright, and0ce1359eswaps them, because the measurements in the next table put e2e at 7.03s and import-safety at 3.69s. That swap cannot move any number here. At two jobs both checks are third and fourth in line, they start only oncelint-type-disciplinefrees the second slot at 34.4s, and the 14.2s of work behind that slot finishes with 76s of basedpyright still to run either wayEvery end-to-end case is paired: the two arms alternate inside a trial and the arm that runs first flips on even trials, so page-cache warmth and background drift land on both arms equally. Percentages come from per-trial differences, not from two independently collected medians, because run-to-run spread on this box is 5 to 7%, wide enough to swamp the effect being measured. Absolute seconds are comparable within a case and not across cases collected in different sessions
Reproducing any row is one loop, for example the end-to-end case:
Where the wall clock goes
Each check run on its own, back to back, median of two:
lint-basedpyrightlint-type-disciplinelint-e2e-basedpyrightcheck-import-safetylint-gatelint-ruffcheck-circular-importslint-format-check-changedThose sum to 173.02s, and a measured
-j 1run oflint-checksis 174.11s, so the breakdown accounts for the whole run. Two things follow. basedpyright is 72% of the serial work, and once the checks fan out it is the entire critical path, because everything else totals 48.6s and fits inside its 124s shadow twice over. So the makespan is basedpyright's runtime plus whatever the schedule wastes around it, and that is both what these changes attack and the ceiling on what they can winBefore and after
make lint, end to endlint-checksfan-out,-j 4against-j 2The end-to-end row is the headline, and 3.0% is the honest size of it: 3.9s off a 131s run, mean paired difference 4.3s with a standard deviation of 3.6s. It is small because it has to be. A run whose critical path is one 124.4s single-threaded process cannot be shortened by scheduling below 124.4s, and
make lintnow finishes in 127.1s, which is 2.2% above that floor. The remaining 2.7s is make's own startup, the setup phase, the 98 MB of--outputjsonthe gate parses, and contention from the one check sharing the box. Anything further has to come out of basedpyright itselfThe single pair in the proof section above, captured fresh at the current tip rather than in that batch, came in at 148.15s against 141.44s, so 4.5%. One trial each cannot separate 4.5% from 3.0% on a box with this much spread, which is the whole reason the table is paired and repeated. Take 3.0% as the number and that pair as a sanity check that the tip still behaves like the branch that was measured
All of that 3.9s is the ordering change, and it is arithmetic rather than statistics. The old line declared
lint-checks: lint-format-check-changed lint-ruff lint-gate lint-type-discipline lint-basedpyright ..., so at two jobs basedpyright sat fifth and could not start until a slot freed afterlint-gate, which is 3.42s. Add the 0.15s the warm setup overlap saves and the predicted win is 3.6s, against a measured paired median of 3.60s. Declaring the long pole first removes that delay by construction, on any host, at any width where basedpyright would otherwise queue behind something cheapThe width row is reported as inconclusive because that is what the paired trials showed.
-j 4came in at 130.15, 132.95, 136.42, 143.75 and 149.90s,-j 2at 127.09, 126.39, 133.85, 145.45 and 169.23s. Median of medians favours-j 2by 1.9%, the mean paired difference favours-j 4by 1.3%, standard deviation is 10.25s, and only 3 of 5 trials favour-j 2. The box also drifted monotonically slower across the run, 130s to 150s on the-j 4arm alone, which is most of that spread. SoLINT_JOBS ?= 2is not claimed as a speedup. It is a structural argument: every check other than basedpyright totals 48.6s against basedpyright's 124.4s, so a single spare slot absorbs all of them with 76s to spare, and every slot past the second can only take a core away from the critical path while heating the machine.?=leaves it overridable for anyone who disagrees on their own hardwareThe setup row is a large percentage of a tiny number and is worth exactly that warm. Its real payoff is the cold path: on a fresh clone or in CI the two steps measured 5.65s and 7.38s, and running them together bounds the pair at the slower one instead of their sum
How often each of those applies
The 3.0% is the common case rather than the whole story, so here is how the runs divide up. Only the dashboard share is measured: 477 of the last 2122 commits touch
ui/litellm-dashboardor the API spec, which is 22.5%. The rest are estimates from how the cache paths behave, not measurements, and they are rounded to sum to 100%uv syncand the Prisma client, where the setup overlap pays mostGate memoization
Measured on the step it changes rather than end to end. Each gate resolved a path once per violation, and all three report far more violations than the tree has files, so
Path.resolve()was making tens of thousands of filesystem round trips for the same couple of thousand paths. Best of three against one real 98 MB basedpyright--outputjsoncapture and live ruff and checker runs, cache off against cache on, identical input both ways:type_check_gate.count_basedpyrighttype_discipline_gate._checkruff_strict_gate.head_violationsThose savings do not add up in wall clock and it would be wrong to quote their sum. The gates are separate make targets running concurrently, and only the type-check gate is on the critical path, so 5.77s of the 9.13s is real and the other two finish inside basedpyright's shadow either way
Measured and not landed
basedpyright --level error. The gate counts onlyseverity == "error"diagnostics, so filtering at the source looked free. It is also pointless: the payload went from 98,239,712 bytes to 98,178,783, per-rule counts came back byte-identical, and wall clock was 121.80s against 122.55s across two paired runs, well inside noise. Nearly every diagnostic the tree emits is already an error, so there is nothing to filterThreading. An earlier revision of this branch ran basedpyright across four pinned worker threads and claimed a 35% cut. That claim did not survive review, and the revert is in this branch. Recording why, so the next person does not spend the afternoon on it:
It changes the answer. Splitting files across threads reorders order-dependent inferences, so the tree's totals move with the width, 149330 serial against 149325 threaded. A gate whose entire job is counting violations must not have its verdict depend on the host
There is no portable width. A pinned constant keeps counts comparable across machines but tunes the gate for one of them. Following
nprocis correct per host, yet the width has to live in the cache fingerprint, so every distinct core count mints its own key, nobody can reuse CI's precomputed base artifact, and everyone pays the cold path insteadThe speed was noise. On four cores,
--threads 4ranged 84-114s against 137-151s serial, a spread wider than the median it was supposed to beat. On a contributor's laptop, width 8 took 724s against 113s serial, roughly 6x slower, and made the machine sluggish while it ranscripts/type_check_gate.pydocuments this, and a test pins the absence of the flag, because a passing run shows neither the count drift nor the slowdownThe one lever left, deliberately not in this PR
basedpyright is 72% of the serial work and the whole critical path, so the only change that moves the number materially is not running it. Its head counts could be cached the way its base counts already are, keyed on the same environment fingerprints plus a content hash of
litellm/**/*.{py,pyi}. 1147 of the last 2122 commits, 54.1%, touch nothing underlitellm/, and hashing that tree costs 0.240s warm, so on more than half of runsmake lintwould drop from ~127s to roughly 37s. That is a much bigger change than anything here and it carries a real failure mode, since a wrong key means the gate certifies stale counts, so it belongs in its own PR with its own test proving a mutatedlitellm/file always forces a recomputeChanges
Makefileschedules the lint fan-out around its long pole.lint-checksnow declares its prerequisites longest-first, because make starts prerequisites in declared order as slots free, so a cheap check sitting ahead oflint-basedpyrightpushes the makespan out by its own runtime at narrow widths. The width itself defaults toLINT_JOBS ?= 2instead of one job per core: every check other than basedpyright totals 48.6s against its 124.4s, so one spare slot hides all of them, and slots beyond that only take cores away from the critical path.?=keeps it overridable, soLINT_JOBS=8 make lintstill does what it says, and dropping the old$(shell sysctl ... || nproc ...)also drops a subprocess from every parse of the Makefile. Finallylintrunslint-installandlint-fetch-baseunder their own-j 2sub-make; one waits on the disk and the other on the network, so paying their sum bought nothingtests/test_litellm/test_makefile_lint.pypins both of those, because reverting either still produces a completely correct lint run, just a slower one, and nothing but a test that inspects the schedule will notice. The overlap test copies the real Makefile into a sandbox whose PATH holds stubuvandgitthat each announce themselves and then block until the other has announced, so a serial setup deadlocks and fails. The ordering test walksmake -nand asserts the declared order matches the measured onescripts/type_check_gate.py,scripts/ruff_strict_gate.pyandscripts/type_discipline_gate.pymemoize their path resolution. The type-discipline gate stopped rebinding itsrootparameter along the way, which LIT011 bans anywayMakefilealso splitsbootstrapintobootstrap-pythonandbootstrap-dashboard.make bootstrapstill runs both and prints the same line, so provisioning a fresh worktree is unchanged, andchecknow depends on the Python half only.scripts/pre_commit_lint.shprovisions the dashboard itself when dashboard or api-spec files are in scope, placed after the Python block forks so the install overlaps that lint, and before both node blocks fork so twonpm installs cannot race in one directory. When that provisioning fails, both node blocks are skipped and the run reports the real reason, rather than the misleading "format with npm run format" the tools emit when they run against an unprovisioned toolchainThis branch was merged with
litellm_internal_stagingaftermake pre-commitwas renamed tomake checkthere, so the provisioning trigger reads the newui_prettier_changed/ui_eslint_changed/spec_filesscope variables. Reading the_changedsets rather than the_filesones matters: a scope containing only a deleted dashboard file still runs the whole-folder eslint budget step, so it still needs a provisioned toolchainThat merge also brought
scripts/gate_slot_lock.pyand its test, which pinnedlint-installandlint-fetch-baseas declared prerequisites oflint-inner. Here they run from that target's recipe instead, under the-j 2sub-make above, so the assertion now accepts either shape. What the test exists for is untouched:lintstill declares no prerequisites of its own, so nothing runs before the slot is heldOne drive-by fix that fell out of the above: the dashboard job leaked its eslint report on Ctrl-C. bash skips EXIT traps on an uncaught fatal signal, and
on_interruptreaches that job as a SIGTERM, so the temp file outlived the run. The existing interrupt test started failing once the dashboard block forked slightly later, which is how it surfaced. Staging's rewrite oflint_dashboardlanded without those traps, so the merge restores themWorth flagging separately, and deliberately left alone:
check-circular-importsis a no-op. It runscd litellm && python ../tests/documentation_tests/test_circular_imports.py, and that script scans a hardcoded./litellm/, which from insidelitellm/resolves tolitellm/litellm/and does not exist. It always prints "No LiteLLM type hints found" and exits 0, in CI as well as locally, which is also why it clocks 0.04s in the table above. Fixing it belongs in its own PR, since it would turn a green check red rather than make anything fasterSame category, also left alone: provisioning goes through
scripts/with_dashboard_node.sh, which enforces the dashboard's engines floor, whilelint_dashboardthen calls barenpx prettier,npx eslintandnode. So the install refuses to run on old node and the lint that follows it does not care. That predates this PRCaveats (if any)
LINT_JOBS ?= 2is structural, not a measured speedupcheck-circular-importsstays a no-op; fixing it needs its own PRFinal Attestation