362 commits

Author	SHA1	Message	Date
Daniel Han	a2cb0410b9	Stop 13 jobs rebuilding the same frontend on every commit (#9375) ... * Stop 13 jobs rebuilding the same frontend on every commit The uv download cache in this action works: it hits exactly (`Cache hit for: uv-Linux-<hash>`) with one 31 kB straggler still fetched. So what is left in `Install Unsloth (--local, --no-torch)` is not download, it is compute, and the elapsed-second prefix added in #9153 says where it goes: 2s venv 5s overlaying local repo (editable) 13s unsloth installed 16s node 20s bun installed 58s frontend built <- 38s in one phase 75s whisper.cpp prebuilt Measured across 13 distinct Linux jobs on main: the frontend build is a median 36s of a 74s install, 49% of it, and 468s per commit producing byte-identical output. The spread is 31 to 42s, so it is a deterministic compute cost rather than variance. The key, and why it is sound ------------------------------------------------------------------------ studio/setup.sh already decides whether to rebuild, by mtime: it looks for anything under frontend/ (maxdepth 1, minus bun.lock), frontend/src or frontend/public NEWER than frontend/dist, and skips the build when it finds nothing. The cache key hashes exactly those three path groups, so a hit means the build inputs are byte-identical. That is a strictly stronger statement than the mtime test it rides on, and it is what makes a restored dist correct by construction rather than by luck. bun.lock is IN the key even though the staleness check excludes it. The check has to exclude it because the install regenerates it and it would self-trigger every run; the cache has no such problem, and a lockfile change means different dependencies and so a different bundle. Deliberate, and it makes the cache safer than the check it rides on. Three ways this could have looked like it worked ------------------------------------------------------------------------ Each is handled, and each is pinned by tests/studio/test_frontend_dist_cache.py, because all three are silent. 1. restore-keys. The uv cache above wants them: a near-miss download still supplies most of the wheels. A near-miss dist is a bundle built from different source, which is wrong rather than partial, so this cache has none. 2. mtimes. actions/cache restores through tar, which preserves the ORIGINAL mtimes. A dist restored that way is older than the checkout that just wrote every source file, so setup.sh's `find -newer dist` would see the whole tree as newer and rebuild anyway: a download paid for, nothing saved, and a cache hit reported. One `touch` of the directory is what makes the hit count, and it is honest because the key already proved the inputs identical. 3. an empty hashFiles. It returns "" when a glob matches nothing, which collapses every commit onto one key and serves an arbitrary dist, with the restore succeeding and the build skipped. A step refuses that outright. The guard ------------------------------------------------------------------------ The failure that matters is not the cache breaking, it is the cache and setup.sh drifting apart: the key stops covering an input, the cache keeps hitting, and every job downstream tests a stale bundle that passes. So the guard reads setup.sh's own staleness block and asserts the key covers the paths found there, rather than comparing against a list written down in the test. Mutation-tested, each failing exactly one test: drop src from the key; add restore-keys; remove the touch; save off main; and add a directory to setup.sh's check without adding it to the key. A test I had to change rather than route around ------------------------------------------------------------------------ test_the_cache_holds_uvs_downloads_and_not_the_venv asserted every cache step in this action points at .uv-cache, and this is the second cache. Its argument is worth keeping: uv's cache is content-addressed, so a stale entry cannot serve wrong content, and that property is the whole justification. A built frontend does not get that argument and needs its own. It is a directory of static assets with no absolute paths, no interpreter coupling and no console scripts, which is precisely what makes a venv unsafe to cache and this safe. So the test now allows exactly two named paths, each with its reasoning recorded at the list, and keeps the forbidden-install-paths check applying to every cache step regardless. Verified it still has teeth: pointing the new cache at ~/.unsloth/studio/venv fails it. Verification ------------------------------------------------------------------------ 72 passed across test_uv_cache_discipline, test_frontend_dist_cache, test_workflow_guards_run_unfiltered and test_cache_budget_discipline. scripts/lint_workflow_triggers.py: OK across 41 workflow files. The action still parses; step order is restore, touch, key check, install, save. Expected effect: about 36s off each of 13 jobs per commit. Cache size is one built frontend per distinct source state, saved on main only, which is the rule every other cache here follows. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-20 04:22:14 -07:00
Daniel Han	2043c734c4	Take two ubuntu jobs off their own runners: absorb one, delete the other (#9360) ... Over 400 completed main push runs, 25 of 96 job types execute in under 120s: 1116s of work spread across 25 runners, each queuing for about three hours. Two of them are dealt with here. Security audit :: pytest tests/security 72s exec, 11096s queue Unsloth export capability :: capability (ubuntu-latest) 67s exec, 10642s queue Different problems, so different treatments. The security suite MOVED, onto the Workflow trigger lint runner ------------------------------------------------------------------------ Same argument that put the lockfile and load-orchestrator lanes into Lint CI in #9176: work with a narrow trigger, moved into a job that was going to occupy a runner on this commit anyway, can only reduce the slots a commit takes. Here the trigger widens too, since this host has no paths filter and security-audit.yml's pull_request does, so the suite now runs on every pull request rather than on the ones that touch its paths. This host and NOT Lint CI, where the other lanes went, and that is the whole decision. Lint CI installs shellcheck from apt, so its harden-runner has to permit escalation and an apt mirror; a security gate moved there would run under a policy weaker than the one it has today. Workflow trigger lint's harden-runner block is byte-for-byte identical to the one the job carried in security-audit.yml (block, disable-sudo, the same six endpoints), so nothing about its isolation changes. harden-runner binds per runner, not per step, which is what makes that the deciding constraint rather than a detail. Folded into the existing pytest invocation rather than added as a step of its own. I wrote it as a separate step first, so a security regression would not be reported as a workflow-guard failure, and test_the_guards_run_in_one_pytest_invocation rejected it: one step per module costs about 15s of interpreter and conftest startup, measured in this repo at 53.9s as one invocation against 300.8s as one each. The guard is right and the attribution preference is not worth 15s. pytest and PyYAML are now pinned here to the versions security-audit.yml pinned them to. That suite runs scripts/lint_workflow_triggers.py as a SUBPROCESS and asserts on its exit semantics, so a pytest or PyYAML that resolves differently changes what it is asserting against. The capability ubuntu leg was DELETED, because it was already duplicated ------------------------------------------------------------------------ That workflow's own comment already explains why it has no macOS leg: every test in tests/test_export_capability.py goes through _patch(), which monkeypatches _has_torch, get_device and is_apple_silicon, so a real Mac proves nothing a Linux runner does not -- and studio-backend-ci.yml runs the same file on ubuntu-latest as part of `pytest tests/`. That argument reaches one step further than it was taken. If Backend CI covers the file on Linux, the ubuntu leg HERE is the duplicate too. Checked: the file is not in that job's --ignore list. The import-safety test does not need a torch-free image either; it installs its own builtins.__import__ blocker and drops preloaded torch/unsloth from sys.modules, so it proves the same thing inside Backend CI's fully installed environment. Windows stays. Nothing else in CI runs that file there, and _has_torch's import probe is the per-OS behaviour the job exists for. The guard ------------------------------------------------------------------------ tests/studio/test_short_job_absorption.py, wired into the unfiltered job. Both changes fail silently rather than loudly if they regress, which is what it is for: - the suite still runs somewhere, and no longer runs twice - the absorbing job's harden-runner has not widened past the six endpoints the suite came with, since "the policy is identical" is the entire justification for this host - the absorbing job has not gained a paths filter - capability still has its Windows leg - studio-backend-ci.yml still runs the whole tests/ tree and does not name test_export_capability.py, because one line added to that --ignore list would remove the coverage the ubuntu leg was deleted for, and nothing would turn red Mutation-tested, each failing exactly one test: drop tests/security from the invocation; add one endpoint to the allowlist; add --ignore=tests/test_export_capability.py to Backend CI; remove the Windows leg. Also corrected a docstring in tests/security/test_scan_packages.py that named tests-security and what it installs. It was about to become false. Verification ------------------------------------------------------------------------ tests/security under the host's own -n 4: 409 passed, 6 skipped. scripts/lint_workflow_triggers.py: OK across 41 workflow files. All three workflows still parse; security-audit.yml keeps its other 4 jobs. Net: 7 short ubuntu slots per commit, down to 5. A note for whoever extends this. The census that found these 15 candidates was partly stale and I nearly acted on it: the lockfile and load-orchestrator rows were already absorbed by #9176 and their samples were pre-merge tails, and the two Local Agent Guides rows show ~0s because they are if-gated to schedule and dispatch, which is a skip and not a fast job. Read the trigger before ranking by duration. Of the remaining candidates, Scorecard is blocked by its job-level id-token: write, the Kaggle gate by a downstream needs:, npm-provenance by an audit egress policy plus registry.npmjs.org, and the notransport clean-install lane by a container that asserts several common tools are absent.	2026-08-20 00:05:28 -07:00
Daniel Han	1c3dde199b	Drain the sampling debounce long enough for the node CI actually runs (#9332) ... * Stop Frontend CI installing Chromium's system libraries through apt every run `playwright install --with-deps chromium` runs its own `apt-get update` inside itself, so it bypassed everything CI has learned about apt: the shared retry helper's 20s transfer cap, APT_ACQUIRE_RETRIES: '0', and the archive cache. The job failed 3 of 8 runs on main. Job 96072994354 (main, 2026-08-19): 9 packages, 21.1 MB, and `fonts-wqy-zenhei [7472 kB]` alone took 5m50s off azure.archive.ubuntu.com. Both 420s attempts died mid-download. That is the same mirror and the same package that took the webkit shards down in #9289. Attempt 2 logged "Need to get 8833 kB/21.1 MB", so apt does resume partials across attempts and still could not finish. Split the way studio-ui-smoke.yml splits it: download the engine, launch it to find out whether the system libraries are actually missing, and run `install-deps` only if they are. ubuntu-latest is a browser-testing image and usually ships them, so the common path now runs no apt at all. The browser and apt-archive cache keys are deliberately identical to the chromium-only shards in studio-ui-smoke.yml (engine token `c`): same image, same Playwright version, same single engine, so the entry is shared rather than duplicated against a budget measured at 99.3% full. The step's authorised worst case doubles with the second helper call, to 2 x (2 x 420s + 125s) = 1930s, so its timeout goes 17m -> 33m and stays under the job's 40m. Both guarded calls are skipped on the common path. Guard: tests/studio/test_playwright_install_avoids_with_deps.py fails the build if `--with-deps` returns to any workflow, and is wired into workflow-trigger-lint, the only job with no paths filter. * Drain the sampling debounce long enough for the node CI actually runs Frontend CI has been red on main since #9055, not intermittently: eight consecutive main runs failed at `Unit tests`, every one on node v22.23.2. The three suites from #9055 wait for a debounced write with a fixed drain -- three rounds of tick(1000) plus six setImmediate turns -- and then assert. Three rounds is enough on node 24, which is what a dev box happens to have, and is not enough on node 22, which `setup-node: 22` resolves to. The same chain drains far fewer continuations per round there, so the write had not landed when the assertion ran. Reproduced by downloading node 22.23.2 and running the suites against both. Measured on the compat suite: rounds 3 10 30 60 failing 7 5 1 0 The compat suite reported it as a missing value (expected 1.37, actual undefined). The simulation suite reported it as an ORDERING violation -- "chat A temperature: owed 0.6, shows 1.37" -- because a scenario whose write has not landed looks exactly like one that wrote the wrong value, which is why this read as a feature bug rather than a slow test. There were three copies of the drain: one per suite plus the shared thread-sampling-world helper the simulations run through. Fixing only the two suites left the simulations red, since their scenarios drain through the helper. The bound is 200, generous rather than tuned to the observed 60, and settle() now takes an optional `until` predicate: it returns as soon as the caller can see the work, and throws naming itself if the condition never holds, so the next slow runtime reports "settle gave up" instead of an assertion on a missing value. Verified: node 22.23.2 compat 16/16 and simulation 18/18 (both were failing); node 24 full frontend suite 4060/4060. Only the simulation suite imports the helper, and no fixed three-round drain remains in tests/. * Raise the scenario drain to 600: 200 was marginal, and Windows needed more At 200 the simulations were 0 failing on one run and 1 on the next on the same machine, and the Windows runner -- slower again -- still had 2 of 120 orderings short. 600 is 0 failing across three consecutive runs, at 107s against 52s. Also records the adaptive version that was tried and is wrong here, so it is not tried again: the rows only change WHEN the write lands, so 'rows have stopped changing' is precisely the pending state being waited through. Quiescence on that observable stops early by construction and scored 4 failures where the fixed bound scored 0. * Stop the settings smoke asserting a tab count a new page invalidates Unskipping the browser smokes surfaced this immediately: the blocked-panel run failed with 'blocking the data panel took the dialog down' while its own report said dialog: True. Nothing had taken the dialog down. The check was if not state["dialog"] or state["nav"] != 12: and the keyboard-shortcuts page had made the nav 13. A stale constant, reading as an error-handling regression. The nav size is now read before the panel is blocked and compared against itself, which is the invariant that was meant: blocking a panel must not collapse the dialog, whatever size the dialog is. The same drift had a quieter half. The smoke's TABS list still had twelve entries, so keyboard-shortcuts had no browser coverage at all and the smoke stayed green without it. It is added here, and tests/studio/test_settings_smoke_covers_every_tab.py pins both directions against settings-dialog.tsx so the next page cannot go uncovered silently. It also checks the workflow's PW_CHUNK_FAIL names a tab that exists -- that value lives in studio-frontend-ci.yml, not in the smoke, and a rename would leave the run blocking nothing while still reporting PASS. Wired into workflow-trigger-lint, the only job with no paths filter, because it reads a workflow. Mutation-tested both ways: dropping the tab from TABS and restoring the literal count each turn it red. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Drain the sampling suites on the loader too, and fail loudly when it gives up The fixed round count in the previous commit was still a guess, and the Windows job proved it: the SAME commit at 600 rounds passed one run and failed the next with "A1: 2 violation(s) across 120 orderings", reporting stale reads as lost edits. Raising the number again was not the answer. The pending work has a second half nothing was waiting on. The store's thread-scoped write ends in `await import("../utils/chat-history-storage")` (chat-runtime-store.ts:1326 and :1754), and these suites register() a resolver hook, which routes that import through the hooks thread. Three repeat imports of an already-loaded module: v24.14.0 no hook 1, 1, 1 turns hook registered 1, 1, 1 v22.23.2 no hook 1, 1, 1 turns hook registered 6, 3, 35 That is the whole green-locally / red-on-CI split, and it is why a loaded Windows runner fails what the same commit passed an hour earlier: the pending work is a message to another thread, so its cost is scheduling latency, not instructions. No round count is correct for that. Counting the mocked timers alone does not cover it either, which is worth recording since it is the obvious next idea. With the counter installed and 25 consecutive quiet rounds per drain, 150 macrotask turns of nothing, v22.23.2 still lost 7 orderings across A1 and A3, every one a write that had not landed. So drain on both observables. tests/helpers/mock-timer-drain.ts wraps the MOCKED setTimeout with a counter, giving an exact count of timers scheduled and not yet fired or cleared, and each round also issues its own import and waits for it, so the wait scales with the loader instead of guessing at it. The drain returns when no timer is outstanding and three consecutive rounds neither scheduled nor fired one. With the probe, three quiet rounds is green on v22 and v24 alike. The generous bound stays as a BACKSTOP that THROWS and names what was still outstanding, which is the change that matters most here. An under-drain used to be indistinguishable from the store losing an edit, so it sent the investigation into the store for two rounds. Now it says so: settle: drain exhausted after 2 rounds, with no timer pending but work still scheduling or firing within the last 3 rounds. Nothing read after this point is trustworthy: a queued write has not landed, so the store still shows the PREVIOUS value, which reads as a wrong value rather than a missing one. Fix the work or raise the backstop; do not read this as the store losing an edit. It is also much faster, because it stops when the work is done rather than after 600 rounds regardless. A typical drain now takes 4 rounds; instrumented over 840 drains in A1 the maximum was 4. simulation v24 101.7s -> 30s v22.23.2 106.5s -> 29s compat v24 0.54s -> 0.46s v22.23.2 0.61s -> 0.52s The three copies of the drain shape, one in the world helper and one in each test file, are now one helper. The dead end already recorded is kept next to the new measurements: quiescence on the store ROWS is still wrong, because the rows only change WHEN the write lands. Verified: both suites green twice each on v24.14.0 and on v22.23.2, the version setup-node 22 resolves to. Full frontend suite 4080 passed, 0 failed. The exhaustion throw was confirmed by forcing the backstop to 2, which produces the message above and no wrong-value violation. A deliberately broken sanitizeThreadScopedSettings still produces a real ordering violation on both runtimes, so the drain is not exiting early. Test files only. * Read the workflow and the dialog as UTF-8, not as the platform default test_source_read_encoding caught four read_text() calls this PR added with no encoding. It is right and they are a real defect: the guard reads settings-dialog.tsx, playwright_settings_tabs.py and studio-frontend-ci.yml, and on a Windows runner Path.read_text() uses the ANSI code page, so any non-ASCII byte in any of them raises UnicodeDecodeError. The whole point of this guard is that a settings page can be added without anyone noticing; a guard that cannot be collected on Windows fails the same way. Repo tests (CPU) was otherwise clean: 1 failed, 8781 passed. * Tell a pwsh crash apart from install.ps1 losing its exit code test_the_pwsh_filter_keeps_the_log_clean_and_the_exit_code_intact went red on a hosted ubuntu runner with completely empty stdout and pwsh's own banner: "An error has occurred that was not properly handled. Additional information is shown below. The PowerShell process will exit." The interpreter died; the script never ran. Workflow trigger lint is green on the last 8 main runs and this PR touches no PowerShell, so it is the runner, not the repo. Read as an ordinary failure it says "$LASTEXITCODE did not survive the added pipeline stages, so a failing install.ps1 would leave its step green" -- an accusation against the installer, raised by a run that produced no evidence either way. That is the same shape as the drain in this PR: an environment shortfall wearing the costume of a product regression. _run_pwsh retries only that case, and the distinction is what keeps it honest: a run that reaches the `RC=` line is returned on the first attempt whatever the value, so a genuine regression can never be retried into green. Only a run with no RC= AND the crash banner is retried, because it carries no verdict to preserve. If both attempts crash it fails with a message naming the interpreter, not install.ps1. 17 passed. Exercised _run_pwsh against a script that prints the banner and no RC=: it raises, so the branch is not vacuous. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 23:55:01 -07:00
Pascal André	489fab4a71	Support OpenCode V2 in unsloth start (#9275) ... * Support OpenCode V2 in unsloth start * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep stable OpenCode guide coverage * Follow the OpenCode V2 stable release * Honor OpenCode V2 policy and server semantics * Fix OpenCode V2 launch command recipes --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-20 04:42:41 +02:00
Daniel Han	607e8a310d	Cache the .deb set webkit needs, so a dead mirror costs a miss not a shard (#9289) ... #9283 stopped two of the four Chat UI shards asking for webkit's system libraries at all, which is why extra and picker went from 14 minutes failing to under 6 passing. The shards that genuinely drive webkit -- chat, banner and the cross-browser indicator -- still pay the full price, and it is still the thing taking them down: 0 upgraded, 181 newly installed Need to get 102 MB/114 MB of archives Get:2 .../fonts-wqy-zenhei [7472 kB] -> 4m51s, then the attempt died Seen again today on a staging chat shard, after every other apt lever in this repo had already been pulled. Bounding the wait cannot help: the work is a real 102 MB and the mirror was delivering 7 MB in five minutes. So stop re-downloading it. apt keeps what it installed in /var/cache/apt/archives until something runs `apt-get clean`, so the .debs are already sitting there at the end of a good run. This copies them out, caches them on main, and copies them back in before install-deps on later runs. apt checks each file against its index and re-fetches only what does not match, so a stale entry costs a download rather than a wrong install -- the failure mode is slow, not incorrect. Keyed on the runner image as well as the engine set, because which .deb versions satisfy a dependency set is a property of the image. Saved on main only, the same rule as every other cache here: a PR-ref entry can only be restored by re-runs of that same PR while still counting against the shared budget. The engine guard from #9283 caught this change, which is the second time it has earned its place. Its detector read step NAMES as well as run bodies, and this step is named for the .deb set it holds -- webkit's -- so two chromium-only shards suddenly looked like webkit users. Fixed in the detector rather than by renaming the step: a `uses:` step cannot drive a browser, so only steps that run something should count.	2026-08-19 06:57:19 -07:00
Daniel Han	32ba55b578	Run the zoo suite in parallel, minus the two files that cannot share a worker (#9285) ... * Run the zoo suite in parallel, minus the two files that cannot share a worker unsloth runs unsloth_zoo's whole CPU suite in each of three matrix cells, at about nine minutes a cell. Measured on a staging runner, whole suite: cell serial -n 4 --dist loadfile HF=latest + TRL=latest 580s 253s HF=4.57.6 + TRL<1 517s 229s HF=default + TRL=default 568s 246s Two of the three cells agreed exactly, failure sets and skip sets both. The third produced 14 failures serial does not: 8 in test_mlx_generate.py and 6 in test_moe_bnb4bit_per_expert_conversions.py. Both files pass on their own, and pass under xdist on their own. They are self-contained, so what breaks them is another file running first in the same worker -- an ordering serial never produces, because serial is alphabetical. The speedup and the divergence are therefore separable, and the pair runs in a process of its own where they are reliable. Being clear about what this is: a mitigation. The suite has cross-file pollution that alphabetical ordering happens to hide, and the cure belongs in unsloth_zoo, on whichever file leaks. What these two files prove does not change here -- a fresh process is what they already get today. loadfile rather than the default `load`, because 34 of the 236 zoo test files touch sys.modules or importlib.reload, so tests within a file have to stay on one worker and in order. The guard exists because half of this pairing is silent. An ignore with no serial rerun deletes 51 tests from CI and the job stays green, which is strictly worse than the 14 failures, since those at least announce themselves. Same shape as test_backend_ci_parallel_isolation for studio-backend-ci. Mutation-tested four ways: dropping the serial rerun, dropping an ignore, dropping --dist loadfile, and losing a deselect in the split -- that last one matters because the three deselects lived on the single command this change split in two, and losing one turns a deliberate "deselected" into a failure on a GPU-less runner. Listed in workflow-trigger-lint because it reads a workflow, so a workflow-only PR would otherwise never collect it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Correct the evidence: the divergence moves between cells A second A/B run across all three cells changed what the first one meant. First run: HF=default diverged, the other two agreed. Second run: HF=latest diverged, HF=default agreed. Same 14 tests both times, same two files -- 8 in test_mlx_generate.py, 6 in test_moe_bnb4bit_per_expert_conversions.py. So it is not a dependency combination, which is what one run made it look like. It is worker scheduling, and roughly one cell per run draws the losing order. Four of six cell-runs were clean. That matters for anyone who re-runs this and sees green: a single clean run proves nothing here, and would otherwise read as grounds to drop the pin. The comment and the guard docstring now say so, because the previous wording pointed at a cell and would have sent the next person looking at transformers and trl versions. The files being pinned are unchanged; if anything the case for them is stronger, since the divergence lands on exactly the same 14 tests each time rather than wandering. * Give the xet stall detector a quiet process too Staging ran the split for real and produced a failure the A/B never did: FAILED tests/test_hf_xet_fallback.py::test_a_second_buffering_episode_after_a_drain_is_not_a_stall AssertionError: a child refilling after a drain was killed: ['Download appears stalled (xet transport) -- no progress for 0s'] Nothing was stalled. The file drives a stall detector against the real clock -- 27 sub-second sleeps, one of them commented "within the unmeasurable window" -- and under four workers on four cores the test was simply descheduled long enough for the detector to fire. "No progress for 0s" is the detector saying so. That is a different fault from the two files already isolated. Theirs is ordering, which is why a fresh process fixes it; this one is a wall-clock margin, and no ordering fix helps a margin. Same remedy, different reason, and the registry records the difference so nobody later "consolidates" the entries and loses why each is there. Checked for the rest of the class rather than waiting to meet it on the next staging round: exactly two files in the suite use sub-second sleeps, and the other one sleeps 0.01s to widen a race window without asserting on elapsed time, so it is not exposed to this. Found only because the split changes how work is distributed, which is worth noting: the A/B ran the whole suite under xdist and this file passed every time. Measuring the change is not the same as running it. * Record why each pinned file needs the quiet process Both causes are now known and they are not the same, which matters because the remedy differs. The bnb one is a sys.modules stub that was never taken back out, and it is fixed upstream in unsloth_zoo#1076 -- so that pin becomes belt and braces rather than the only thing holding it. The mlx one is an import-order contract stated in the shim's own docstring, and the clean fix would spoof platform.system() suite-wide, so the pin is doing real work there and will keep doing it. * Correct the mlx explanation: it is not established I wrote that test_mlx_generate.py fails because the MLX-on-torch shim has to be installed before any unsloth_zoo MLX module is imported, quoting the shim's own docstring. It fits the symptom -- eight isinstance failures are what two copies of a class look like -- but it is wrong. conftest imports unsloth_zoo, which pulls unsloth_zoo.mlx in before any test module loads. So the precondition is violated on every run, including every run that passes, and the file passes on its own. I found this by asserting the precondition and watching it fail a green run, which is the only reason this comment is not shipping as fact. The pin does not change: eight tests fail under xdist and pass serially, reproducibly, on the same commit. That observation is what justifies it. The explanation was mine and it did not survive being tested, so it now says so -- a wrong lead in a comment costs the next person more than no lead at all. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 06:25:09 -07:00
oobabooga	de102032ee	Desktop: ship a complete Linux AppImage (#9113) ... --------- Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com>	2026-08-19 10:09:29 -03:00
Daniel Han	6c41897833	Cache uv's downloads, which is now the largest single cost in CI (#9271) ... * Cache uv's downloads, which is now the largest single cost in CI Re-profiling after the apt work landed changed the ranking completely. The old top step, Install Playwright browsers at 557-671s, is now 91s. What sits at the top instead: 3588s total 92s x39 Install Unsloth (--local, --no-torch) 1925s total 175s x11 Update banner layout regression (Playwright) 1910s total 478s x 4 unsloth_zoo @ main - full pytest (CPU) 1683s total 99s x17 Perform CodeQL Analysis More total time than any test, in 39 job runs, and all of it uv re-resolving and re-downloading the same wheels: nothing carried its cache between runners. UV_CACHE_DIR appears in this repo only in the two clean-machine workflows, which set it in order to delete it. Caching this is safe because of WHAT is cached. uv's cache is content- addressed by URL and hash, so a stale entry cannot serve wrong content -- the worst it can do is miss. That is the difference between this and caching the venv, which would have to reason about an editable overlay (--local installs the repo with -e, so source changes need no reinstall), a moving `unsloth-zoo @ git+main`, and absolute paths baked into console scripts. I looked at the venv first and it is not worth the hazard. restore-keys is deliberate and is correct only here: a near-miss still supplies almost every wheel, which is most of the win, and it is safe precisely because entries are content-addressed. The same fallback on an install cache would be a bug. Saved on main only, like the HF and Playwright caches and for the reason they record: a PR-scoped entry is restorable only by re-runs of that same PR, while every PR can restore from the default branch, so saving on PRs spends a 50 GiB budget measured at 99.3% full once already and evicts main's copy. `uv cache prune --ci` first, so the key does not grow without bound across 39 jobs. Placed in the composite action, so all 39 call sites get it from one definition -- which is what that action exists for. The cold-install lanes are untouched and a test keeps them that way. They do not use this action today; if one ever did, the composite writes UV_CACHE_DIR to $GITHUB_ENV, which outranks a job-level env: for every later step, so a warm cache would silently replace the cold machine those workflows are named after and they would still go green. Nine tests. All five mutants reintroduced and confirmed red: the cache pointed at the venv, saved on every ref, restore-keys removed, UV_CACHE_DIR set after the install, and a cold lane adopting the action. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 05:47:32 -07:00
Daniel Han	718fe69fb0	Install the browser engines each Chat UI shard actually drives (#9283) ... The four shards install all three engines each. Two of them never open anything but chromium: nothing in `extra` or `picker` names firefox or webkit at all. Installing webkit's system libraries for them is not a rounding error. On 2026-08-19, in the extra shard: 0 upgraded, 181 newly installed, 0 to remove Need to get 102 MB/114 MB of archives Get:2 .../noble/universe amd64 fonts-wqy-zenhei all 0.9.45-8 [7472 kB] -> 4m51s later, attempt 2/2 did not finish within 300s Fonts, X fonts and a soundfont, fetched so that a shard which never launches webkit could time out fetching them. Both attempts died there and the shard reported nothing about the UI it exists to test. That is the last apt exposure in this job, and unlike the previous two rounds it cannot be fixed by bounding anything. Making the index refresh cheap (the retries change) left the download, and no timeout survives a mirror moving 7 MB in five minutes. The fix is to stop asking for 102 MB that nothing uses. chat keeps all three for Cross-browser permission controls, banner keeps all three for its "other engines" step, and the browser cache key carries the engine set: without that the chromium-only cache would be restored by a three-engine shard, report a hit, skip the download, and fail to launch what it never got. The matrix keeps `shard:` as a list and augments it through `include:`, because test_chat_ui_shards_cover_everything reads that list to prove every Playwright step lands on some shard. An include-only matrix passes YAML and silently hides the shards from that guard, which I did first and it caught. test_ui_shard_engines enforces both directions against the steps, never against the comment above. Installing an unused engine is waste; driving an uninstalled one is worse, because Playwright reports it as a launch failure deep inside a suite, minutes after the install step went green, which reads as a flaky test rather than a missing package. Mutation-tested three ways: adding webkit to picker, removing webkit from chat, and collapsing two engine sets onto one cache key. It is listed in workflow-trigger-lint because it reads a workflow, so a workflow-only PR -- exactly the change it exists to reject -- would otherwise never collect it. That guard caught this too.	2026-08-19 04:26:37 -07:00
Daniel Han	51b7d4a155	Give the Colab freeze a pip cache, like the job beside it already has (#9276) ... smoke-install downloads a 709-line pip-freeze on eight matrix legs at once and keeps none of it. api-introspect, two jobs up in the same file, already restores and saves through the action pair; this one was left on a bare setup-python. Keyed on the freeze plus the workflow. The freeze is the pin set, so it is what the downloads depend on, and the workflow comes too because the seed step rewrites those pins in place -- CPU index mapping, skips, spoofs -- so an edit there changes what gets installed without touching the freeze. api-introspect keys on the workflow alone because it pins its dependencies inline; keying this job the same way would rebuild 709 downloads whenever any other job in the file is edited. Saved with always(), because the failure path is when the cache is worth most: if the freeze does not resolve as a set the job falls back to installing 709 pins one at a time, and a partial download set is still a head start. Correcting something I have been assuming about this job: it is `schedule` and `workflow_dispatch` only, so it never runs on a pull request and none of this is on a PR's critical path. It is still worth doing, for a different reason than I had -- eight ubuntu runners holding a 25 minute cap contend for the same pool every PR job queues against, and the queue is repo-wide. The allowlist in test_cache_budget_discipline caught the new entry, which is what it is for: a cache is a claim on a shared 50 GiB budget and a job has to earn it. Listing it there also enrolls it in the checks that were already running for the other nine, so the key scoping, the restore-to-save wiring and the main-only save gate are all now enforced on this job rather than trusted.	2026-08-19 03:22:16 -07:00
Daniel Han	214e24c0ce	Move the two version-incidental macOS jobs onto the emptier pool (#9277) ... * Move the two version-incidental macOS jobs onto the emptier pool Rust unit tests (macos) is this repo's last finisher. The census behind that found minutes of work sitting behind hours of queue, and no execution-time work touches it, so the runner pool is the only lever left. studio-mac-install-matrix runs macos-15 and macos-26 from ONE matrix, which makes them comparable on identical commits and identical triggers rather than across different jobs at different times. Over 163 jobs per leg: exec med exec p90 queue p90 macos-15 160s 713s 20667s macos-26 176s 597s 3866s The medians say these are the same class of machine, which they are: both free Apple Silicon standard runners. The queue p90 says they are not the same pool. macos-15 is 5.3x worse in the tail, five and a half hours against one. Worth being precise about what the tail means here, because the median is zero on both images and hides all of it. macOS queueing is not slow on average; it is occasionally catastrophic, and the occasional case is what sets the wall clock for the commit it lands on. The likely cause is the macos-14 retirement. Everything migrated onto macos-15, this job included, and macos-26 was left comparatively empty. That makes this a fact about today's pool rather than a property of the image, so the comment records the query as much as the result: it is one read of the install matrix and can be re-run whenever the queue looks wrong again. Two jobs, not all of them. These two test our Rust crate and our MLX dispatch, where the OS version is incidental. The install matrices are deliberately testing across OS versions and are left alone, and studio-mac-ui-smoke stays too: it documents a dependency on the frameworks the image ships for Chromium, which is a claim about macos-15 specifically and wants checking before it moves. The guard is for the next retirement rather than this one. macos-14's is recorded in three comments in this repo, each explaining why some job moved off it, and comments do not fail -- so the next one gets discovered the way this one was, by a pool nobody is watching. LIVE_MACOS_IMAGES cannot know GitHub's roadmap, but it forces the retirement to be written down once and then names every job that has to move. Mutation-tested by putting macos-14 back. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 03:21:25 -07:00
Daniel Han	6e57736ad9	Stop blaming the recipe for a turn that never came back (#9266) ... * Stop blaming the recipe for a turn that never came back `connection (codex)` fails on main and on unrelated branches, and the two lines it prints contradict each other: ##[warning] ... judging the turn on its assertions instead of calling it guide drift. ##[error] [guide drift] agent=codex: the documented launch command exited non-zero (rc=124) ... so the documented flow in start.py drifted. Neither is right. run_timed printed the warning unconditionally, but three callers (connection, resume, attribution-ab) have no assertion that can rescue a partial turn and treat a cap as fatal on purpose. So it promised the opposite of what was about to happen, for exactly the callers most likely to hit it. It now states the fact and leaves the verdict to the caller. The error is worse, because it sends the reader to the one file that is not implicated. The transcript shows codex launching perfectly -- correct provider, correct model, correct profile -- and then sitting on ERROR: Reconnecting... 1/5 for the full 600s. Nothing about the documented flow had drifted; the model server never answered. A cap means the launch command was fine and the turn never came back, which is a different failure with a different owner. Still fatal. Waiving a cap here would report "connection OK" for a recipe that printed a banner and then blocked on a headless prompt, which is the failure this job exists to catch, and assert_reply cannot tell a finished reply from a startup banner. Only the attribution changes. A non-zero exit from the launch command is still reported as drift, which is the case guide_fail is right about. Four static guards, since the script needs five agent CLIs and a live model server to run. All four mutants (run_timed speaking for its callers again, the timeout routed back through guide_fail, the timeout waived instead of fatal, a non-zero exit no longer reporting drift) were reintroduced and confirmed red. Wired into workflow-trigger-lint, the only job with no paths filter. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make the waiver guard enforce its own sentence, not a magic number The shell suite went red on this branch: 'exactly the expected TIMED_OUT sites (got 4, want 3)'. Worth deciding which of the two was wrong rather than moving the number. The heading is 'only file-edit turn 1 rescues a soft timeout', and that is the invariant worth having. The assertion under it counted every consultation of TIMED_OUT and pinned the total at 3 -- so it conflated the single waiver with the fatal checks beside it, and its own comment admitted as much ('turn 1's guard, PLUS the resume and attribution-ab fatal checks'). The fourth site this branch adds makes a cap MORE explicitly fatal at connection. Failing a guard named for waivers because a fatal check was added is the assertion being over-specified, not the change being wrong. The neighbouring semantic assertions agree: 'connection guard has no TIMED_OUT escape' passes. So the two shapes are counted separately. The waiver is the || form and is the only one that lets execution continue past a cap; everything else consults TIMED_OUT to stop. That enforces the sentence rather than a number, and keeps all the power: a waiver added anywhere still fails. Mutation-tested against the repaired guard -- a waiver added to the connection guard, resume's fatal check turned into a waiver, and the connection check deleted outright are all caught. 44 pass, 0 fail. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 03:21:20 -07:00
Daniel Han	dccc792374	Ask whether Playwright system libraries are missing before installing them (#9273) ... * Ask whether Playwright's system libraries are missing before installing them `--with-deps` makes playwright run its own `apt-get update` internally. That is the one apt call in this repo that could not be restructured: every other one now tries the image's package lists first and only refreshes them on a miss, but this update happens inside playwright and there is no flag for it. So it still pays full price for a dead mirror. On this branch's own base, 2026-08-19: 07:42:09 Installing dependencies... Switching to root user... 07:42:29 Ign:2 http://azure.archive.ubuntu.com/ubuntu noble InRelease 07:58:22 (step fails, 16m14s) three shards at once, on the critical path of each. Two changes, both of the same shape as the apt work: separate the failures, then ask whether the work is needed. The browser download is a CDN fetch and has nothing to do with apt, so it is its own call now. A stalled mirror can no longer stop browsers being downloaded, and a CDN failure no longer reads as an apt problem. The apt transaction runs only when the libraries are actually missing. ubuntu-latest is a browser-testing image and ships nearly all of them; when it does, `install-deps` is a full apt update and transaction to install nothing. The honest test of that is launching each engine, which is exactly what the suites are about to do -- so the probe is the same question the job asks two steps later, asked cheaply and first. When the probe fails, install-deps runs and the probe runs AGAIN. Without that second check a missing library surfaces later as an opaque per-test timeout in whichever suite happens to run first, which is a much more expensive way to learn the same fact. Rebudgeted for two helper calls rather than one: 2 x 300s plus a lock wait, twice over, is 24m of worst case against the job's 30, so the per-attempt cap comes down from 420 and the step timeout goes 18 to 25. test_apt_steps_are_bounded caught that arithmetic when I first got it wrong -- the two calls doubled the authorised retries past the step's own cut-off. The probe's logic is exercised against a stubbed playwright: exits 0 when all three engines launch, and 1 naming each engine that does not, for one broken engine and for two. The embedded Python is parsed with ast and the surrounding shell with bash -n, in both shards. * Stop apt retrying inside the install-deps attempt The probe landed and worked: banner and the cross-browser shard found the libraries already present and skipped apt entirely, 18m to 7m40s and 12m36s. The other three shards found them genuinely missing, so install-deps ran, and there the premise of the previous commit was wrong. The image does not ship everything all three engines need. install-deps runs its own apt-get update and that update is the one apt call here that cannot be restructured to try the image's lists first. The helper's 20s transfer cap was applied to it and said so in the log: 08:41:45 apt configured to fail fast: 20s transfer timeout, 3 internal retries 08:42:14 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease 08:46:45 attempt 1/2 did not finish within 300s 4m31s with a 20s cap in force. The cap was not the problem; repeating it was. Acquire::Retries defaults to 3, meaning four passes over the index, and with four InRelease URIs stalling to the cap that is 4 x 4 x 20s. The update spent the entire attempt and the packages were never fetched. So the retry moves out of apt and into the loop that already exists for it. One pass is about 80s, leaving 220s of the attempt for the download it is for, and a stall that outlives that is now a failed attempt the outer loop retries under a fresh timeout, with a warning naming which way it went. apt's internal retries are invisible by comparison and are charged to the caller's budget. Guarded, because reverting it looks harmless: the value would go back to apt's default, every other assertion in test_apt_steps_are_bounded would still pass since the step remains bounded and retried, and the symptom would come back as a step that fails slowly against a mirror, which reads as bad luck rather than as a setting. Mutation-tested by restoring the 3.	2026-08-19 02:37:33 -07:00
Daniel Han	2e3cc032f6	Record the venv own requirement digests, not the installers (#9263) ... * Record the venv's own requirement digests, not the installer's Every fresh desktop install on Linux and macOS came up ManagedStale and repaired itself before it would run. From the app's own log: 05:44:22 Managed preflight: install probe result Stale { reason: "studio_install_requirements_changed" } 05:44:22 desktop_preflight completed disposition=ManagedStale 05:44:22 start_managed_repair command called 05:44:24 studio install incomplete -- forcing dependency pass to repair... 05:44:32 Managed preflight: install probe result Ready The manifest recorded digests from REQ_ROOT, which is the requirements directory next to whichever install_python_stack.py ran. A desktop bundle carries its own copy. verify_install reads the INSTALLED package's copy, because at verify time install_manifest.py is imported out of the venv. Two different trees, compared to each other. They agree until a tracked requirement file changes upstream, and then every install performed by that bundle is stale for ever. v0.1.800-beta was cut 2026-08-14 and installs unsloth 2026.8.18; #9148 pinned openai in extras.txt in between. Windows was unaffected only by luck of layout. The digests now describe the tree the verifier will read. A source or editable install has no copy under site-packages and still uses the root it was given, which is what keeps an edited studio.txt invalidating the manifest on the --local path. Also: the CI gate reported "saw: ManagedStale" and nothing else on all four platforms. The reason is logged on its own line, and tail -60 scrolls it away the moment the backend starts logging, so the one fact that explained this had to be recovered from an uploaded artifact by hand. All three log dumps now grep for it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 01:31:09 -07:00
Daniel Han	fdf83cbd6a	Make apt fail fast, so the retry has something to retry (#9260) ... * Make apt fail fast, so the retry has something to retry #9256 bounded the stalls. This says why they happened and stops them. Reading the four logs, every one ends the same way: 04:47:02 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease [126 kB] 05:16:29 ##[error]The operation was canceled. Twenty-nine minutes of silence mid-fetch of a 126 kB index file, and two of the four hung on that same file. What came before it is the other half: azure.archive.ubuntu.com was Ign'd four times over thirty seconds, so apt had already failed over through /etc/apt/apt-mirrors.txt to the public archive, which is not provisioned for this fleet. apt did not treat any of that as an error. Acquire::http::Timeout defaults to 120s and is an idle timeout, so a socket that is open and trickling never trips it. Cutting it to 20s with internal retries turns a 29 minute hang into a ~1 minute failure, and that matters beyond speed: the wall-clock kill is what orphans the dpkg lock, so an apt that gives up on its own is one we never have to kill. Not pinning a mirror. The evidence does not support it: in one stall Azure was dead and the public archive hung, in another Azure was serving fine and the transfer stalled at a 13.6 MB package. Neither is reliably better, and the mirrorlist failover is already the right mechanism. It just needed to be allowed to give up. The written config is validated against apt itself (apt-config dump round-trips all four options), and the helper's retry, timeout, giving up, recovery and no-command paths are exercised end to end. * Wait on all four apt locks, and stop racing a slow mirror Two bugs, both mine, both caught by CI on this branch. 1. apt-get update takes /var/lib/apt/lists/lock and nothing else. The helper waited on /var/lib/dpkg/lock-frontend only, so after an attempt was killed mid-update the wait saw a free lock, retried immediately, and produced E: Could not get lock /var/lib/apt/lists/lock. It is held by process 2420 (apt-get) twice in under two seconds. Three attempts, one real one -- exactly the 'a retry that cannot succeed is worse than none' failure the header warns about, in the one case it was written for. It now waits on all four: dpkg's two, lists, and archives. 2. 150s per attempt was racing the mirror, not bounding it. The mirror was degraded rather than dead, so every attempt got killed at the same point and none finished. Two attempts of 360s instead of three of 150: the bound exists to stop an infinite hang, not to beat a slow server. studio-update-smoke's job budget goes 15 to 25 to fit the new worst case; the guard checks that arithmetic. * Ask the image's package lists first, and refresh only if they miss The mirror outage is still live. From this branch's own GGUF smoke: 06:55:33 Ign:4 http://azure.archive.ubuntu.com/ubuntu noble-backports InRelease 06:55:35 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease [126 kB] 07:01:11 (nothing, for 5m36s, then the attempt's cap) Azure is unreachable and the public archive it fails over to is not provisioned for this fleet. Acquire::http::Timeout does not save us there: it is an idle timeout, and a server dribbling a byte every few seconds never trips it, so the wall-clock bound is what ends the attempt. The bound is working as intended -- the step now fails in 14 minutes with the reason printed, where before it spent 30 and was reported as "cancelled" with nothing said. But a bounded failure is still a failure, and the operation that fails is one we mostly do not need. `apt-get update` refreshes every index for every suite. The runner image already ships populated lists, and these steps install a handful of ordinary packages. So try the install first, and refresh only when it misses: apt-get install -y X || { apt-get update && apt-get install -y X; } In the common case the slowest and most failure-prone apt operation is never performed at all. When the image's lists really are too stale -- the "Unable to locate package" case the runner-images maintainers warn about -- the update still runs and the install is retried, so nothing is traded away. Not applied to clean-machine-install-ci: a fresh WSL image genuinely has no lists, so install-first would always miss, and a bare machine is that leg's whole premise. The three outcomes were checked as shell rather than reasoned about: resolvable (update never reached, rc=0), not resolvable (update runs, rc=0), and both failing (rc=1, so the failure still propagates under the `bash -e` GitHub runs steps with).	2026-08-19 00:19:50 -07:00
Daniel Han	cabed07f95	Bound every apt step in CI, and fix the retry that never ran (#9256) ... * Give apt one bounded, retrying entry point, and fix the retry that never ran Two problems, one of them mine. The retry I added to the Playwright install last round was dead on arrival. GitHub runs `run:` blocks as `bash -e`, and I wrote the loop as timeout --signal=TERM --kill-after=30 480 python -m playwright "$@" rc=$? A bare failing command under -e aborts the step then and there, so the second attempt never ran. The step exited 124 having printed none of its own warnings -- indistinguishable from the unbounded stall it was meant to replace, which is exactly how it read on #9251's indicator and banner shards, both dying at 8m05s. The previous version used `if timeout ...; then` and was exempt from -e; changing it to read the exit code is what broke it. Reproduced both forms under `bash -e`. The second is broader. `Linux deps` is a bare `apt-get update && apt-get install` with no bound at all, and on #9251 it sat on the Azure mirror for 28 minutes. Of 24 apt-invoking steps across 13 workflows, 2 were bounded. An unbounded apt step is not slow, it is silent: it spends the job's whole budget, GitHub scores the result as "cancelled" rather than a failure, prints no reason, and skips every step after it. .github/scripts/retry-with-apt-lock.sh is now the single definition: per-attempt timeout, retry, dpkg-lock wait between attempts, and exit codes reported honestly (124/137 as a stall, anything else as the status it actually returned). It carries no `set -e` and uses `|| rc=$?`, so it works whatever the caller set. Six paths exercised locally, including the regression above: called from a `bash -e` step, both attempts still run. Sizing respects what this file already learned. The note on ui-smoke's timeout-minutes records that cutting the JOB budget to 20 turned this same stall from a slow pass into a red build, because one attempt had nowhere to go. The retry is what changes that, not a shorter wait: 180s is ~14x the 13 seconds `Linux deps` takes healthy, so only a mirror that is genuinely gone burns all three attempts. Every step's worst case fits inside its own bound and every step bound sits under the job's 30, which stays the final backstop and is deliberately not tightened. Listed in the workflow's paths filter, since both apt steps now execute it. The other 20 unbounded apt steps are left alone: every stall observed so far has been in this workflow, and converting release workflows unreviewed is a worse trade than leaving them until one of them actually stalls. * Bound every apt step in CI, not just the two that were noticed Three jobs were lost to an unbounded apt step in one day, each in a different workflow and each reported as "cancelled" with no reason: Chat UI Tests (chat) at 30m18s, Frontend build at 16m38s inside playwright install --with-deps, and Source lint at 5m02s on a job whose whole budget was 5 minutes. None of the three said anything about its actual subject, and the last was noticed only because someone happened to open a job that said "cancelled". Routes the remaining twelve on-runner apt steps through the shared retry helper, gives each an explicit step timeout, and adds a guard so the next one cannot be added without one. update and install now go as a single unit: retrying the install after a stalled update just re-reads the same broken package list. playwright install --with-deps counts as an apt step, because that is what it is. Leaving it out by name is what cost the Frontend build job. Job budgets were raised where the step's bounded worst case no longer fit: source-lint 5 to 20, api-smoke 12 to 20, frontend build 20 to 40. A job timeout is a backstop for the unforeseen; using it as the bound on a known-flaky step is what turns a diagnosable failure into a silent one. Not converted, with reasons recorded in the guard: clean-machine-install-ci runs apt inside bare containers and WSL with no checkout and deliberately no sudo, and the two steps where apt is the assertion rather than the setup. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 23:18:35 -07:00
Daniel Han	d9e5c059c8	Print what the server said when an inference smoke request 4xx's (#9202) ... * Print what the server said when an inference smoke request 4xx's broken for three main runs, and the only thing CI printed was urllib.error.HTTPError: HTTP Error 400: Bad Request The response body carries llama-server's own explanation and it went out unread, so the cause had to be reconstructed by hand from the workflow source. #9155 fixed the regression; this stops the next one costing the same dig. The HTTPError branch of every request helper in the three inference smoke workflows (nine sites) now prints the status, the reason and the body before re-raising. The read itself is guarded, because a truncated or already-consumed body must not replace the real status with a confusing one, and the original error still propagates: reporting is not tolerating. The tenth site, the tool-probe seed loop in studio-inference-smoke.yml, is a caller rather than a helper. post_sse has already printed the body by the time it re-raises there. tests/studio/test_inference_smoke_http_diagnostics.py parses the Python actually embedded in the workflows with ast rather than matching text, so a rewrite that keeps the behaviour keeps passing. It also asserts every embedded probe parses at all, which nothing else did: the heredocs are shell text inside YAML, invisible to every linter in the repo. Seven mutations checked red (revert the change, drop the re-raise, drop the print, drop the status code, unguard the read, remove the handler, break the syntax). Listed in workflow-trigger-lint because it reads workflow files, so the edit that breaks it is workflow-only and no paths filter would collect it. Confirmed by removing the line and watching test_workflow_guards_run_unfiltered name it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Collapse the split f-string the formatter left behind --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 21:02:07 -07:00
Daniel Han	566272b721	Run the Mac GGUF inference phases in the Mac UI job, not on a second runner (#9213) ... * Run the Mac GGUF inference phases in the Mac UI job, not on a second runner studio-mac-inference-smoke.yml did the same checkout, setup-node, setup-python, `install.sh --local --no-torch` and assert-llama-loads.sh as studio-mac-ui-smoke, and primed hf-cache from the same key with the same GGUF. A studio/** PR paid two macOS slots to install Unsloth twice on two Macs and then test two surfaces of it. Slots are the point, not seconds. Over the last 8 green main runs of each, the UI job executed 1154s behind a 17438s queue and the inference job 402s behind a 13252s queue. Concurrent macOS jobs are capped at 5 account-wide and that pool is shared with unslothai/unsloth-zoo, so a slot returned shortens the queue for everything else on it. Sequential execution costs about 400s of wall clock, which is under 4% of what the second queue cost. This is the fourth workflow folded into that job, after studio-mac-api-smoke and studio-mac-update-smoke. Placement is load-bearing in both directions. The absorbed phases go after the UI and API phases because `Phase N environment` writes GGUF_REPO, GGUF_VARIANT, GGUF_FILE and STUDIO_PORT to $GITHUB_ENV and phase 3 points them at gemma-4-E2B; $GITHUB_ENV outranks the job-level `env:` for every later step, so running them first would hand the UI phases a different model than the one they declare. They go before the update phase because that phase uninstalls and asserts the machine is clean, which is the teardown for the whole job. Four things the move would otherwise have broken silently: - Phase 1 carried no `if:` at all. Under the install that implicit success() meant "the install worked"; under the UI phases it means "and every Playwright test passed", so one flaky browser run would drop all of phase 1 as a skip. Phase 1's steps are now gated and chained the way phases 2 and 3 already were. - Phase 1 booted with boot-studio-api-only.sh's defaults, logs/studio.log and STUDIO_PID. logs/studio.log is the chat UI phase's server log and the artifact upload publishes it, so an absorbed phase 1 would erase the evidence of a chat UI failure that had already happened. It now uses logs/studio_chat_api.log and STUDIO_CHAT_PID. - UNSLOTH_ALLOW_HOST_OFFLOAD lived on the absorbed job's env and does not travel with the steps. Without it the load returns HTTP 400 several layers from the cause. It is back at job level, with the paravirtual-Metal explanation, and test_mac_host_offload_optin.py is what caught the omission. - Phase 3 bound 18899, which the UI job's WebKit indicator run also binds. The phases are sequential and each kills its server, so they would not overlap today, but "would not overlap" is a property of step order. Phase 3 is now on 18892. Phase 1's own HF restore/prime/save block is not carried over: it used the identical cache key for the identical GGUF this job already primes before the install, so it would restore over a cache it is sitting on and race its own save. The trigger gains .github/scripts/studio_smoke/multi_turn_chat.py, the only path the absorbed workflow watched that this one did not already cover. Timeout goes 65 to 100, against a measured 1154s + 483s worst case plus three cold downloads. tests/studio/test_mac_bundled_job_phases.py guards the four phase-isolation properties the bundle now depends on: no port and no server log shared by two phases, no absorbed step without an explicit `if:`, and the uninstall phase last. The log scan resolves boot-studio-api-only.sh's default from the script, because the collision that actually occurred was two phases both omitting `--log`, which a text scan cannot see. Six mutations checked red, including reintroducing each of the four defects above. The check name changes from "GGUF inference smoke (API, tools, vision)" on Mac to "Chat UI, API, Update and GGUF inference". No branch protection rule names it: required_status_checks.contexts is empty on this repo. scripts/build_mac_absorb.py in the workspace reproduces the splice from origin/main; it is not checked in. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop the permission-level reloads racing the composer mount studio-mac-ui-smoke went red at 35672fc9b and again at bfcaea465, both times on waiting for locator("button[aria-label=\"Permission level for tool calls\"]:visible") with green runs on either side. The permission block reloads the page four times with `wait_until="domcontentloaded"` and then asserts the pill visible on the default 5s expect timeout. DOMContentLoaded fires long before React has mounted the composer, and on a 3-core macOS runner that gap is regularly wider than 5s. The composer-mount step at the top of the script already settles the network before waiting, and its comment says why: "on macos-14 racing straight into wait_for() either burns the timeout or crashes the renderer mid-mount". The permission block never got the same treatment. It does now, through one helper used at all four sites. This asserts exactly what it asserted before. It just stops asking before the answer can exist. It rides with this PR because this PR is what makes that job the sole carrier of Mac GGUF coverage as well as the UI coverage, so a flake there now costs three more phases than it did. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 21:00:06 -07:00
Daniel Han	df8fb6a03b	Say both load fallbacks when both fire, and repair the two suites that guard it (#9189) ... * Repair the two chat auto-load suites that #9173 left red on main #9173 added a vision-projector fallback to the auto-load success toast. Both of the suites that read that code broke on it, and both were green on the commit immediately before (54b6ca4c3). 72 tests, in two different shapes: 1. tests/studio/test_chat_autoload_failure_gate.py, 71 failures. The harness slices autoLoadSmallestModel verbatim out of chat-adapter.ts and runs it, so anything the slice references must exist in PREAMBLE. mmprojFallbackMessage did not, which is a bare ReferenceError inside the retry loop; the loop catches it and scores it as a failed load, so every scenario fails as a wrong-model assertion. The file's own guard caught this and named the symbol, which is what it was written for after #7699 did the same thing. Stubbed as a function of the reason rather than a copy of the real three-message record. The value only reaches `options.description`, and these scenarios assert on which model loaded, never on toast copy, so copying user-facing strings in here would give them a second home to drift from. 2. tests/studio/test_model_picker_contracts.py, 1 failure. It asserted the literal `description: cpuFallbackReason`, and the mmproj branch went in front of it. The property held; only the spelling moved. Its own comment records this happening once already, when the CPU-fallback branch first appeared, so it now pins the property: the description varies on both fallback reasons and still has an undefined arm for the ordinary path. Scoped to the description EXPRESSION, not the whole helper. `cpuFallbackReason` is also a parameter name in the signature above, so a substring test over the block stays green with the CPU branch deleted outright -- the first cut of this check did exactly that, and mutation caught it. Three mutations verified red: the description no longer driven by any fallback reason, the CPU branch dropped, and the mmproj branch dropped. 4223 passed, 4 skipped. * Say both fallbacks when both fire, not just the projector one Both load paths wrote the toast description as mmprojFallbackReason ? mmprojMessage : cpuFallbackReason ? cpuMessage : undefined so whenever both reasons are set the CPU-fallback sentence is dropped. The user is told "loaded without vision" and never told the model is running on the CPU, which reads as a deliberate, explained degradation rather than an unaccelerated session. The combination is reachable. On a CPU-fallback replay llama_cpp.py preserves _cpu_fallback_reason (it clears it only when not _replaying_cpu_fallback) and resets _mmproj_fallback_reason so the projector can fail again inside that same launch. A low VRAM machine whose Vulkan backend crashed is exactly where the projector then falls back too. loadFallbackNotice() in mmproj-fallback.ts is now the single composition of the title suffix, the description and the degraded flag, and both call sites delegate to it. CPU_FALLBACK_MESSAGE moves there as well, so the two paths cannot describe the same condition differently again. Tests: four combined-case cases in mmproj-fallback.test.ts, verified red against the shipped nested ternary. test_model_picker_contracts.py now asserts the call sites delegate and pass both reasons rather than matching the old inline ternary, and test_chat_autoload_failure_gate.py's stub mirrors the composition so a call site dropping a reason stays detectable. * Repin the CPU-fallback toast test to behaviour, and unbreak the queued-capabilities test Two frontend suites were red. auto-load-cpu-fallback-toast.test.ts matched the warn-vs-success choice and the message text as substrings of showAutoLoadSuccess. Both moved into loadFallbackNotice, which is the single definition the explicit-load path now shares, so the match went stale. Matching the inline form again would go red on a refactor that changes nothing a user can see, and would stay green if only one of the two load paths kept the behaviour. It now calls loadFallbackNotice and asserts the verdict, and separately asserts the call site delegates to it. queued-model-capabilities.test.ts was red on main before this branch. #9173 added `import { isTextOnlyMmprojFallback } from "./mmproj-fallback"` to image-input-support.ts, which the test imports statically. Extensionless is the right form -- 2314 of the 2367 relative imports under src/ are written that way, and vite and tsconfig's "bundler" mode resolve them -- but the bare node loader does not, and a static import resolves before any registration can run. The test now registers the bundler resolver and imports dynamically, which is what mmproj-fallback.test.ts already does for the same module. Both files reformatted by biome; regex literals hoisted out of the test bodies for useTopLevelRegex. Full frontend suite: 3734 pass, 0 fail. typecheck clean. The 4 biome errors in chat-adapter.ts and use-chat-model-runtime.ts are byte-identical on origin/main. * Order the split-axis abort against the mmproj strip, not against its argument test_tensor_split_abort_raises_early_to_layer_fallback has been red on main since #9173, which renamed the text-only strip's argument from _last_spawn_cmd to _vision_gpu_cmd. That rename is right: the strip should read the vision GPU command rather than whatever was spawned last, and #9173 refreshes _last_spawn_cmd from the result immediately after. The test was pinned to the old argument name, so a rename with no behavioural content took it down. The failure also misreported itself. `assert raise_idx < src.find(needle)` reads as an ordering check but is two claims at once, and when the landmark is gone it fails with "assert 249423 < -1" -- which says the ordering broke, when what happened is that the landmark moved. Each landmark is now required to exist before it is ordered, and says so. The strip is matched on the call rather than on what is passed to it. What this test is about is that the abort raises BEFORE the projector is discarded (#6659); which command the strip reads from is that code's own business. Checked both ways: removing the strip call from load_model goes red with a message naming the missing landmark, and renaming the argument again stays green. * Name the endpoints when the heavy-thread harness catches a stray request The harness records every /api/ URL issued during a measured action, then keeps only the count, so the failure reads let 2 /api/ requests reach the network during the measured actions and stops there. It says an interaction paid for a round trip without saying which one, and the reader has to bisect the frontend to learn what the harness already knew and discarded. It now reports the endpoints. Deduplicated and capped at eight, because the case this instrument exists to catch is a request issued once per message, which would otherwise print hundreds of copies of one line. This is why it surfaced now: the step has not run on main since #9173, whose unit test break fails earlier in the same job and short-circuits it. It was last green at 54b6ca4c3. With the unit tests repaired on this branch the job reaches the step again, and the first thing it needed to say was the one thing it did not. * Skip the playwright harness tests on the module they need, not the package Five tests guard themselves with `pytest.importorskip("playwright")` and then import a harness that does `from playwright.sync_api import Page`. On the Repo tests (CPU) runner the top-level name resolves as a namespace directory with no sync_api inside it, so the guard passes and the import dies with ImportError: cannot import name 'Page' from 'playwright.sync_api' (unknown location) A skip condition reported as a failure, on every branch, for as long as that runner stays that way. It is red on #9202 and #9213 too, neither of which touches any of this. One of the two files already said what the guard was really for: "importing a harness pulls in playwright.sync_api". It now checks that. * Stop the fork-count store asking the server about threads it has never seen Two fixes, both found by the heavy-thread smoke once it could name what it caught. The smoke reported "let 2 /api/ requests reach the network during the measured actions" and, with the endpoints now printed, they were POST /api/chat/threads/__LOCALID_lsQbsDZ/forks A `__LOCALID_` thread has no server record, so that request can only 404, and getThreadForkCounts already maps 404 to the empty map the entry starts as. It is a round trip whose answer is known before it is sent. Not a rounding error. A new chat is in exactly that state, and this store refreshes on CHAT_HISTORY_UPDATED_EVENT, which fires once per streaming chunk, so the first reply in a new chat paid one useless request per debounce window for as long as it streamed. #8992 added the store to stop the chat getting slower as a thread fills; excluding threads the server has never seen is the same intent. thread-ids.ts already had the predicate. Two tests: a local thread must not fetch on subscribe or on a burst of history events, and a saved thread on screen beside it must still refresh -- the guard has to be per thread, not a global off switch. They import the real isAssistantLocalThreadId rather than restating the prefix, so the rule under test cannot drift from the app's. Both go red with the guard removed. Second fix, same job: the playwright skip guard. The previous commit moved it from "playwright" to "playwright.sync_api" and it still failed, because sync_api resolves as a namespace package on that runner too. Only the symbol the harnesses import distinguishes a usable install, so the guard now checks for Page the way the harness does. Verified both ways against a Page-less sync_api: it skips, and it still proceeds when Page is there. Frontend suite 3789 pass, typecheck clean, tests/studio 4237 pass. * Bound and retry the Playwright browser install so a stall is not a silent 30 minutes This step stalls. Three times in one day it sat in apt's download loop until the job's 30-minute timeout killed it, while the sibling shards finished the whole job in 4 to 9 minutes. Twice on #9202 and once on #9189, always the same step. The cost is out of proportion to the cause. GitHub scores a job timeout as "cancelled" rather than a failure, prints no reason, and skips every step after it, so the chat shard reported nothing about the chat surface for what both times turned out to be an infrastructure hiccup that cleared on a plain re-run of the same commit. A per-attempt `timeout` turns the stall into a failure instead of a silent wait, and the retry is what actually recovers. The healthy time is about 2 minutes, so 8 per attempt is 4x headroom and a merely slow mirror will not trip it. timeout-minutes bounds the pair in case `timeout` is outlived by an unkillable child. Same reasoning, and the same wording, as the bounded prime-hf step in studio-mac-ui-smoke.yml. Both install steps in this file, since ui-smoke and ui-indicator run the identical command. Left alone on mac and windows: neither passes --with-deps, so neither has the apt phase this is about, and neither has been observed to stall. * Make the Playwright install retry able to actually recover The bound added in the previous commit worked: the stall became an 8m37s step FAILURE with a complete log instead of a silent 30-minute cancellation, and the log named the cause on the first try. It also showed the retry could not work. playwright shells out to apt-get as root, so terminating the python parent leaves that child alive holding the lock, and attempt 2 died two seconds later with E: Could not get lock /var/lib/dpkg/lock-frontend. It is held by process 4578 (apt-get) A retry that cannot succeed is worse than no retry: it buries the real reason under a second, different failure. Attempt 2 now waits for the lock to clear, up to two minutes, and only then takes it -- the holder is our own orphan and the runner is a throwaway. Two smaller things the same log exposed. --kill-after was missing, so a process that ignores SIGTERM would have been waited on forever inside the step bound. And the warning said "did not finish within 8 minutes" about a two-second exit, which sends the next reader looking for a stall that never happened; it now separates timeout's 124/137 from playwright refusing outright, and prints the status. timeout-minutes 18 to 22 to cover two 8-minute attempts plus the lock wait, still inside the job's 30.	2026-08-18 20:58:30 -07:00
Daniel Han	06220da4d2	Run two short jobs as background lanes of Lint CI, not on their own runners (#9176) ... * Run two short jobs as background lanes of Lint CI, not on their own runners Two workflows each held a runner slot on every commit for a few seconds of read-only checking: Unsloth load-orchestrator CI :: test ~33s, its own slot Lockfile supply-chain audit :: audit ~6s, its own slot Both now run as background lanes inside Lint CI, which has no path filter and was already going to occupy a runner on every commit. Three slots become one. Backgrounded rather than appended as steps, so the lanes overlap the ~65s of lint instead of adding to it: ubuntu-latest has four cores and the lint steps are single-threaded. Measured locally on the real step bodies, a 5s lane alongside 5s of foreground work took 7s total, not 10s. Absorbing a narrower-triggered job into an unfiltered one can only reduce the slots a commit takes. The absorbed work now runs on commits that would not have triggered it, on a runner that was going to exist anyway. Three things about this were not obvious and are worth recording: * Each lane needs its own venv. The load-orchestrator lane installs fastapi and friends while the foreground steps use the interpreter Lint CI's own pip install populated, and two concurrent installs into one site-packages is a real race. * The launch has to detach from the step's stdout and stderr. A background child inherits those pipes and the step is not considered finished while a writer holds them: launching a 4s lane took 4.0s before the redirect and 0.0s after. Nothing would have gone red, the overlap would just have silently disappeared. * A missing exit-status file has to be a failure. A lane that never started is otherwise indistinguishable from one that passed. The payloads live in .github/scripts/lane-*.sh, called by both the lane and the standalone workflow, so there is one definition. studio-load-orchestrator-ci.yml keeps workflow_dispatch as an escape hatch; its push trigger had no paths filter at all, so it was running on every commit to main regardless of the four paths its PR trigger listed. lockfile-audit.yml keeps its nightly schedule, which is a different check: it re-reads the lockfiles against advisories published since the last commit, which no commit-triggered run can do. tests/studio/test_absorbed_lanes_still_run.py covers the silent failure modes. Eight mutations verified red, including an uncollected lane, a missing sentinel read as success, and a launch that stops detaching. * Run the lane guard on workflow-only PRs The guard covers work moved off its own runner into a background lane, and every way that regresses -- a lane launched but never collected, a missing exit status read as success, a launch that stops detaching -- is an edit to lint-ci.yml or to one of the two absorbed workflows. No job's paths filter matches a workflow-only change: workflow-trigger-lint enumerates specific test modules, and Backend CI matches tests/** and its own YAML but not the workflow files guarded here. So the suite would first be collected by Backend CI's unfiltered push on main, after the change had merged. That matters more than usual because every failure mode here is silent. Nothing goes red when a lane stops being collected; the absorbed suites just quietly stop being able to fail anything. * Run the workflow guards in one pytest, and run the ten that ran nowhere workflow-trigger-lint is the only job in the repo with no paths filter, which makes it the only job a workflow-only PR is guaranteed to start. Every other job filters on source paths: Backend CI matches studio/**, tests/**, scripts/** and its own YAML, but not arbitrary workflow files. So a guard that reads a workflow file and is not run by this job has a specific silent hole. The edit it exists to reject is by definition a workflow-only edit, and on such a PR it is never collected. It is collected later by Backend CI's unfiltered push on main, after the change has merged. The guard still works, it just stops being able to block anything. Ten modules were in that state at once, including the indicator-parallelism guard, the GGUF phase-independence guard and the Windows small-checks guard, each of which reads a workflow and can only be broken by editing one. Three separate review rounds reported one instance each before the pattern was visible. They now run in ONE pytest invocation rather than a step per module. Measured over these 18 modules: 53.9s as a single call against 300.8s as one each, because this repo's conftest is expensive to import and a step per module pays it every time. That also removes the reason the list stopped growing: adding a guard no longer means adding a step. tests/studio/test_workflow_guards_run_unfiltered.py keeps the list honest, with two exemptions carrying their reason (PIL and a local utils helper, neither of which that job installs). It caught itself on its first run, being a workflow-reading module that was not yet in the list. Three mutations verified red: a guard dropped from the list, the invocation split back into two steps, and the job gaining a paths filter. * Give the consolidated guard job the cores and the budget it now needs Absorbing 12 more guard modules into one step made that step the job, and the job still carried a 5 minute budget sized for the old set. Serial the 21 modules are 209s on a 192-core box, which would have failed on timeout on a slower runner rather than on anything real. So: pytest-xdist, `-n 4`, and 15 minutes. Pinned to 4 rather than `auto` on purpose. ubuntu-latest is a 4-core runner so on CI the two are identical, but `auto` scales to the host and every xdist worker re-imports this repo's expensive conftest. Measured here: `auto` spawned 192 workers and took 327s, worse than running serially, while `-n 4` is 42s. A fixed width behaves the same everywhere and cannot be made pathological by the machine it lands on. 304 tests, identical results serial and parallel.	2026-08-18 05:27:55 -07:00
Daniel Han	54b6ca4c3f	Stop spending the Actions cache budget on caches nothing reads (#9151) ... * Stop spending the Actions cache budget on caches nothing reads The repo holds 50.0 GB of Actions cache against a 10 GB limit, so LRU eviction runs continuously and the entries CI actually depends on are the ones being thrown away. Measured: 28.9 GB 60 entries setup-python pip 58% of the budget, only 18 distinct keys 7.2 GB 8 entries v0-rust 11.8 GB 5 entries the GGUF / HF model caches CI depends on 25.9 GB 214 entries on PR refs, restorable only by re-runs of that same PR Two doors let that happen, and both are silent: nothing goes red when a cache is evicted, CI just re-downloads a 4.6 GB model and everyone assumes that is the cost. setup-python derives one pip key per interpreter from dependency files across the whole repo, so dozens of unrelated jobs share it and race to save under it. The entries measure 666-715 MB. Jobs that only pip-install huggingface_hub, pytest or playwright were paying that for the 0-7s their restore step takes. Those 24 call sites give the cache back; the 9 that install torch/transformers keep it. The Playwright browser caches in studio-ui-smoke.yml used the read-write actions/cache, which saves from its post-step on every ref, at ~470 MB per browser set. They now restore always and save on main only, matching what every model cache in this repo already does. This is the same thrash loop the GGUF caches were fixed for, arriving through a different door: PR misses, downloads, saves its own copy, evicts main's, next PR misses. Windows benefits most. Its model caches are among the evicted, and a Windows cache miss costs about 3x a Linux one. tests/studio/test_cache_budget_discipline.py asserts both doors stay shut, that cold-install lanes are never warmed by a cache, and that every setup-python step still pins an interpreter -- the last because removing cache: 'pip' from an inline-flow mapping by deleting the line takes python-version with it, which this change did once before it was caught. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Correct the cache budget figure: 50 GiB, not GitHub's 10GB default The budget comments across eleven workflows quoted GitHub's 10GB default. This repo's quota is 50 GiB, so the earlier framing (and the 33.3GB / 3.3x-over measurement they carry) understated the headroom and overstated the severity. Re-measured with the correct quota: 49.63 GiB across 258 entries, 99.3% full. The conclusion does not change, only the reason. Eviction runs at the margin, and what fills the budget is redundancy rather than useful payload: 20.74 GiB the SAME key held on several refs (42% of the whole cache) 6.67 GiB 13 copies setup-python ... python-3.13.15-pip-85e247d7... 6.50 GiB 11 copies setup-python ... python-3.11.15-pip-85e247d7... 3.83 GiB 10 copies setup-python ... python-3.12.13-pip-85e247d7... 0.91 GiB 3 copies ms-playwright-Linux-1.62.0-cfw-v1 10.70 GiB 84 entries written and never read again, 10.60 GiB of it setup-python 24.01 GiB 198 entries on PR refs, restorable only by re-runs of that same PR 0.00 GiB entries unread for 7+ days: nothing is idle, the cache is churning Every duplicated key already has a copy on main, which every PR can restore from, so the PR-scoped copies buy no hit rate and evict the copy that does. Worth recording for whoever adds the next cache: the duplication is a SYMPTOM of sitting at quota, not an independent cause. A PR only writes its own copy after missing, and it only misses because main's copy was evicted. Getting back under the quota stops the loop on its own, which is why this change reclaims claimants rather than restructuring the nine jobs that legitimately cache torch. * Scope every pip cache key to what its job actually installs Five of the nine jobs that keep a pip cache had no cache-dependency-path, so setup-python hashed dependency files across the whole repo to build their key. That is the second multiplier behind the 27.05 GiB those caches occupy: 16 distinct keys appeared in a week, because one requirements edit anywhere invalidates every interpreter's entry at once and orphans the old ones at ~700MB each. The four already-scoped jobs (consolidated-tests, studio-backend) show the pattern; it just was not applied consistently. mlx-ci installs from studio/backend/requirements/studio.txt, so it now points there like its siblings. The other four pin their dependencies inline in the workflow (a torch CPU index URL, pinned transformers/trl/peft), so there is no requirements file to name and their key was describing files they never read. For those the workflow file IS the dependency spec, so the key hashes that: it moves when the install actually changes and not otherwise. notebooks-ci's step had to be expanded from the inline-flow form (with: { python-version: '3.12', cache: 'pip' }) to take the new key. The guard test now asserts every allowed pip cache scopes its key, since an unscoped one costs budget quietly rather than failing. * Save the Playwright browsers only when the download succeeded The save ran under a bare always(), so a browser install that failed part-way still stored whatever had landed on disk. The key is pinned to the resolved Playwright version and never rolls over, so every later run would restore that partial tree, report a cache hit, skip the download and run install-deps against engines that are not there. Every UI job fails until someone deletes the entry by hand, and nothing in the log points at the cache. always() stays, so browsers that did download are not thrown away because an unrelated earlier step failed; the install step now carries an id and the save checks its outcome. Both jobs in the workflow had the same condition. * Point the scoped cache keys at the nested checkout, and run the guard on workflow-only PRs Three jobs check the repo out under `unsloth/` because they need a second repo beside it: notebooks-ci api-introspect, and version-compat-ci zoo-imports-under-spoof and grpo-fake-run. The cache-dependency-path added for them was workspace-root-relative, so it matched no file. setup-python treats that as fatal ("No file in ... matched to ..."), not as a reason to skip the cache, so all three jobs would have stopped before installing anything. Prefixed with the checkout directory, and guarded: the new test resolves every cache-dependency-path against its own job's checkout paths. The discipline test is also wired into workflow-trigger-lint, which carries no paths filter. Repo tests (CPU) does collect the file, but studio-backend-ci.yml's paths do not match a workflow-only PR, so the job never starts for exactly the change this guard exists to reject. Collection is not coverage if nothing triggers the workflow that collects it. * Count setup-python's implicit save, and drop a cache path with no cache The save scan only looked for actions/cache steps, so it reported clean while nine jobs wrote a PR-scoped entry on every run. setup-python's cache: input registers a post-step (post: dist/cache-save/index.js in its own action.yml) that saves after the job on whatever ref it ran on, with no condition to gate it. Those nine are now named in PIP_CACHE_JOBS_PENDING_CONVERSION rather than skipped silently, so a tenth joining them fails the check, and the follow-up that converts them to an explicit restore plus main-only save empties the set. lint-ci.yml also kept a cache-dependency-path after its cache: 'pip' was removed. That is inert, since setup-python only reads the path when caching is on, but it reads as a scoped cache key and the next person believes the job is cached. Removed, with the comment rewritten to say why the job has no cache, and a check for the same shape elsewhere. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Read the save condition and the cache path, rather than searching them Two guards in this file passed on text that looked right rather than on the property they claim. The main-only check was a substring search for "refs/heads/main". Three conditions contain that literal while still saving on every PR: a `!=` comparison, an `||` that admits another event, and the check appearing in only one branch of one. It now splits the expression on its top-level `||` and requires a positive `github.ref == 'refs/heads/main'` in every alternative, which is conservative in the safe direction. The splitter is itself tested against nine expressions, including a `||` inside a string. The cache-dependency-path check compared prefixes and skipped root-checkout jobs entirely, so `unsloth/.github/workflows/typo.yml` read as correct and a misspelling in a root-checkout job was never examined. Each entry is now resolved against the checkout it belongs to and globbed against the tree. Paths under a checkout of a DIFFERENT repository are skipped rather than reported: notebooks-ci api-introspect checks out unslothai/notebooks beside this one, and a path under it cannot be resolved here. Both matter because setup-python does not skip a cache it cannot resolve, it fails the job with "No file in ... matched" before anything is installed. All four shapes verified red: the inverted comparison, the widened `||`, a correctly prefixed but misspelled nested path, and a misspelled root path. * Save the pip cache on main only, via a restore/save action pair (#9165) * Save the pip cache on main only, via a restore/save action pair setup-python's built-in cache: 'pip' is the read-write form. It restores in the step and saves from its own post-step on whatever ref the job ran on, and exposes no condition to stop that. An entry written on a pull_request ref can only be restored by re-runs of that same pull request, so it buys no hit rate while competing for the shared 50 GiB budget against the copy on main that every PR can read. Measured on this repo before the change: setup-python entries were 19.45 GiB across 40 entries, 15.49 GiB of it on PR refs, with four interpreter keys duplicated four to six times each for 9.60 GiB of pure waste. None of it shows up as a failure. Over quota, GitHub evicts least-recently-used, so main's copy goes, the next PR misses, downloads and writes its own, and CI simply gets slower. The repo had already diagnosed and fixed this same loop for the GGUF caches and the Playwright browsers; setup-python was left doing it because the built-in cache has no save-gating knob. Splitting restore from save is how you get one. The nine jobs that genuinely install a torch/transformers-class dependency set now use .github/actions/pip-cache-restore and .github/actions/pip-cache-save, with the save gated on refs/heads/main. Defined once as a composite pair rather than inlined nine times, following install-unsloth-local: the gate is the whole point of the change and nine copies of it would drift. The action resolves pip's cache directory with pip cache dir rather than hardcoding a path, so the macOS and Windows call sites work unchanged, and it fails loudly when key-files matches nothing, since an empty hash would silently collapse every job onto one key. The guards move with the architecture. The save scan now reads composite actions too, which is where the save now lives; a job using the built-in cache at all is now a failure rather than an allowlist question; and each of the nine is checked to restore and save as a wired pair. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Point the local action references at the nested checkout `uses: ./...` resolves from GITHUB_WORKSPACE, not from the workflow file, and three jobs check the repo out under `unsloth/` because they need a second repo beside it. The unprefixed path is a directory that does not exist there, so all six references would have failed with "Can't find 'action.yml', 'action.yaml' or 'Dockerfile'" before installing anything. Same root cause as the cache key paths, one level out: those were fixed for these jobs and the action paths were not. The guard now resolves every local action reference against its own job's checkout paths. * Discover pip cache users, rather than only iterating the allowlist Replacing setup-python's built-in `cache: 'pip'` removed the mechanism that found claimants. Every remaining check is parametrized over PIP_CACHE_JOBS, so a new job adding the restore/save pair was never visited: it would get a ~700MB entry with no scoping check, no wiring check and no justification, and this file would stay green. The allowlist stopped being enforced and became a list to iterate. Two checks now read the workflows instead. One rejects any job using either half of the pair without being listed. The other re-applies the heavy-install requirement to the discovered set, so a job that keeps its cache after its torch-class install moves elsewhere is reported rather than grandfathered. Verified red by giving workflow-trigger-lint, which installs nothing heavy, a pip-cache-restore step. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 04:47:22 -07:00
Daniel Han	aa32c1861c	Studio CI: make a failing browser smoke say why, and stop it skipping the build gates (#8983) ... * Fix two module resolution failures in the frontend test suite * Studio CI: make a failing browser smoke say why, and stop it skipping the build gates * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix the third Windows-only path failure in the frontend test suite * Drop the #8980 content this branch no longer needs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Snapshot the vite tail before printing it, and put the startup bundle gate ahead of the smokes The failure dump iterated the live deque the drain thread is still appending to, while writing each line to stdout, so a vite server that was still talking during the dump raised deque mutated during iteration and dropped the tail in exactly the noisy failure the dump was added for. list() of a deque is atomic; take it first. Startup bundle budget still ran after the browser smokes, and every step carries an implicit success(), so a red smoke skipped it. It only needs dist/, so it moves up with the other build gates. * Tighten comments in the smoke diagnostics changes * Upload the failing smoke's own report, not every report but that one The failure upload globbed logs/playwright-*, which four of the five browser smokes write. playwright_settings_tabs.py writes logs/settings_tabs_report.json and, for the blocked-chunk arm, logs/settings_tabs_blocked_report.json. So when either settings smoke failed the artifact contained the reports of the smokes that had passed and not the one that had just failed, which is the opposite of what this upload is for. Add both names to the upload path, and guard it: the new test reads every logs/ path the wired-up smokes actually write out of their own source and fails if one is not matched by an upload pattern. Shown red on the bare glob first, naming logs/settings_tabs_report.json. * Upload the non-blocking smoke's report on the runs where it is the point The stream-pacing smoke is continue-on-error, which rewrites its CONCLUSION to success while leaving its OUTCOME as failure. The artifact upload was gated on a bare failure(), so on the runs where that smoke was the only thing that failed -- exactly the runs where its report is the whole point -- the upload was skipped and logs/playwright-stream-pacing went nowhere. Give the step an id and OR its raw outcome into the upload condition. Guarded: the new test walks every continue-on-error browser smoke and fails if it has no id, or if its outcome is not named in the upload condition. Shown red two independent ways first, reverting the condition to bare failure() and separately deleting the step id, each naming the step. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>	2026-08-18 04:42:41 -07:00
Daniel Han	58403dd001	Studio: measure where a heavy thread stalls, across engines and thread size (#9016) ... * Studio: measure where a heavy thread stalls, as a curve over thread content Users report Studio and Desktop going sluggish after long generations with code cells and text. That is a statement about content volume, so the new harness varies characters of thread content rather than message count, and the fixture carries the mix the report names: prose, large code fences, tool calls with collapsible output, code-execution result panes, HTML and canvas artifacts, and inline images. The primary metrics are DOM-observable and wall-clock, because Unsloth Desktop is a Tauri webview and not Chromium. PerformanceObserver accepts type longtask on WebKit 26.5 and Firefox 153 without throwing and then never fires, so support is read from supportedEntryTypes; CDP counters are recorded alongside and labelled Chromium-only. * Settle the highlighter between repetitions and tolerate constant engine chatter Re-opening the thread throws away every highlighted fence, so repetitions 2 and 3 were measuring a thread that was still building itself: on Chromium at 300K the scroll gesture read 667ms on the first repetition and 1100ms on the two after it, and the difference was the re-highlighting. Firefox 153 emits exactly two scroll-anchoring notices per run at every size. A warning count that grows with the thread still fails; a constant one does not, or the harness could never report a Gecko number. * Measure time to settle from the start of the action, and stop crediting floored metrics Measured from the end of the gesture, time to settle reads ~50ms at every size on every engine, because the answer is then three frames, which is the minimum the loop can return. From the start of the action it is what a user waits. A count that goes 0 to 4 has answered the question and counts as discriminating. A floored timing that is zero or negative at the smallest size has not: it says the action resolves inside one frame there, which is a metric with no room to move. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Record a crashed cell instead of losing the whole matrix A WebKit page that ran out of memory at 300K on a loaded machine took eight good measurements down with it. The cell is now recorded as crashed, the run continues, and the verdict still fails on it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Say precisely what the Long Tasks observer does off Chromium * Say how to bound a wedged engine, having measured that nothing in-process can On a macos-14 runner Chromium finished all three sizes in 90 seconds and then Playwright's WebKit wedged at the smallest size and never came back, which cost the whole matrix. page.evaluate and browser.new_page have no timeout, and SIGALRM does not help: the sync API blocks the main thread inside a greenlet, so the exception lands in the driver and the caller never resumes. The process boundary is the only bound that works, so the docstring says to drive one engine per invocation under an external timeout. * macOS runners have no timeout(1), so give the portable bound instead * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Load the crypto polyfill on the heavy-thread smoke page Same defect as the thread-weight page: the smoke page this branch adds was missing <script src="/crypto-boot.js"></script>, so crypto-uuid-boot.test.ts fails with "smoke-heavy-thread.html must load /crypto-boot.js". It matters more here than on a normal smoke page. This harness is what the perf numbers are measured on, and a page that lacks the polyfill differs from production in exactly the kind of way that makes a measurement mean something other than it claims. Verified: the named assertion fails before the change, and all four tests in the file pass after it. * Measure what the labels say in the heavy-thread harness Every repetition deleted a message and nothing put it back, so repetitions 2 and 3 ran on a smaller thread than the census recorded before the loop. An instrumented run at 25K read 20, 19, 18 messages at the start of each repetition; the smoke page now exposes restore() and it reads 20, 20, 20. One cycle is 20 messages against 10 content kinds, so at 25K those deletions were taking a whole kind each time. The re-open window closed on three calm frames, which held 7 rAF samples for an action taking up to 1.4s, and the leftover highlighting was absorbed by the untimed gate at the top of the next repetition. It now settles on no long frame and no new highlighted token for a grace period, and reports the time of the last activity so the grace is not added to both ends of every ratio. The token probe is polled rather than read per frame because it is a document-wide query whose cost would otherwise grow with the signal. The recorder decided ownership from a shared running flag, so a callback scheduled by the previous action ran once more under the next one and both loops appended to the same array. It carries a generation token now. Both settle() calls in the menu script compare a MutationObserver flag before the observer microtask has run, so each waits out a full double rAF. The growth axis carries a count of those floors instead of a flag, and the menu total carries two. median() dropped None, so a repetition where the menu never opened was averaged away and the null checks downstream never saw it; it now returns None if any repetition did, and a key that was null throughout stays present. A scroll, jump or re-open that never settled is a harness failure rather than an axis reading not recorded. Expanding the tool panes after waiting for the highlighter mounted two fresh unhighlighted fences per cycle whose work landed in the keystroke window, the next thing timed. Seeding and repetitions share one build_fixture() now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the measurement integrity tests in CI The 22 tests added with the harness fixes were registered in no workflow, so they ran nowhere. A test that cannot fail is not a guard, and these are the ones that keep a timed-out repetition from being published as a median of three. * Stub the fork count with a real zero, and only that endpoint getForkCount returns data.count and the badge's guard is count <= 0. An empty object body makes that undefined, and undefined <= 0 is false, so the badge rendered on every assistant message with a title reading undefined forks from this message. Measured at 25000 chars: 10 badges and 4031 DOM nodes before, 0 badges and 3981 after. That is DOM in proportion to thread size, added to the axis this harness exists to measure. The stub also matched every url containing /api/, so any other request a measured interaction made was resolved locally before Playwright emitted it and measure_cell's listener never counted it. The harness could report zero stray API requests while fanning out, which is the thing it claims to catch. It matches the fork count endpoint alone now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Answer the requests the harness provokes from an allowlist Narrowing the fetch stub to the fork-count endpoint was right, and it broke the run: the delete action persists through syncExportedRepositoryToBackend whenever remoteId is truthy, which the synthetic __LOCALID_ id always is, so three requests reached the wire inside the delete measurement and the stray check failed every size. CI showed it as HARNESS-BROKEN, 5 requests, at both 25000 and 100000 chars. The stub is an explicit allowlist now rather than either a blanket match or a single endpoint. Each entry answers a request the harness itself provokes, with the body that endpoint really returns, so no round trip lands in a timed region, and anything unlisted still goes to the network and trips the counter. Narrowing it is also what made two requests visible that the blanket match had been hiding since the beginning: re-opening a thread asks for the project list and the knowledge bases. Those are app fan-out rather than fixture upkeep, so they are stubbed to keep the network out of the reopen window but counted and printed as stubbed api requests. Answering a request must not delete it from the record. The fork-count guard now pins the body to its own allowlist entry rather than scanning the whole file, since another entry legitimately answers with an empty object and a file-wide check would fail on it while saying nothing about fork counts. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add seedCompactTail and gapMetrics for the viewport gap measurement #9058 measures the empty band below the last mounted row and needs two things this harness did not expose. Landing them here rather than having that branch duplicate 767 lines of an unmerged harness. seedCompactTail(targetChars, tailMessages) builds the heavy fixture with the same buildThread call seed() makes, then appends N one-word messages, so the first mount commit lands entirely on compact rows. That is the worst case for a fixed size initial window. Census parity is the point: seedCompactTail (25000, 16) reports 36 messages against seed(25000)'s 20, a tail of exactly 16, with every other count unchanged. gapMetrics() reports the band below the last row, measured against the viewport's bottom edge rather than scrollHeight, so the viewport's own bottom spacer counts as the gap it always was and the caller subtracts spacerHeight to get the part the mount window owns. Computed any other way the numbers stop being comparable across sizes. Both degenerate returns are kept: no viewport gives ok 0 alone, and a viewport with no rows gives ok 0 with mountedRows and clientHeight. The contract test is separate from the rest because what it protects is on another branch: #9058's probe preflights for these exact names and keys, so a rename here breaks a probe nothing in this repo would otherwise exercise. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Gate the tool expand on a count that a closed thread fails The wait after expandTools() read collapsibleOutputs, which is the Radix collapsible CONTENT ELEMENT. Radix keeps that element in the tree for its collapse animation, so it is present whether the card is open or shut. Measured at 25000 chars before any expandTools() call: collapsibleOutputs 2 of the 2 expected while codeExecutionPanes was 0, and after expanding, collapsibleOutputs was still 2. The gate could not fail, so the ordering that build_fixture exists to guarantee, expand and then wait for the highlighter, was never enforced. It reads codeExecutionPanes now, the pane's own pre, which is 0 collapsed and 22 expanded at 300000 chars on all three engines. The comment on collapsibleOutputs claimed Radix mounts the content only while open; that is measurably false and it now says what the count really is, with a warning not to gate on it. Also records what the timed windows scan, since two review items asked. Each window scans a fixed number of times rather than once per frame, and the cost was measured rather than argued: the re-open makes two messageCount passes, 0.4ms of a 2292ms re-open at 300000 chars and 0.0ms of 363ms at 25000, and the menu scans four times for 2.7ms of 3208ms against 0.3ms of 375ms. The share is 0.017 and 0.08 percent, the same at both ends of the axis. * Run the gap contract in frontend CI I added test_heavy_thread_gap_contract.py and registered it in neither the path filter nor the pytest step, so the guard that keeps #9058's probe working ran nowhere. That is the third time this round something was added that could not fail, and this one was mine, two commits after fixing the same class twice. In both lists now. * Count the reopen paint floor instead of declaring it zero growth() subtracts one ~33ms double-rAF vsync floor per wait a metric is clocked across, and the count was a hand-declared integer in GROWTH_AXES. reopen ms declared 0. Reopening is driven by a React state update, so the count check immediately after openThread() always still sees the unmounted tree and the loop always pays at least one __nextPaint() before it can observe the rebuilt messages. That is the same floor already subtracted from jump painted ms and delete ms. Leaving it at 0 kept a full floor of constant baseline in both ends of the ratio, which drags the ratio towards 1 and can report a real reopen curve as flat, worst when the smallest fixture rebuilds near the vsync floor. Rather than change the constant and leave the next one to be declared by hand, REOPEN_JS now counts the waits it actually pays and returns them, and floor_declaration_problems compares the declaration against the count for every engine and size, from harness_failures, so a mismatch fails the run instead of being published. FLOOR_COUNTERS is keyed on the exact axis name, since a prefix would let a later reopen settle ms axis be checked against this one's declaration. A cell that reports no count is a failure rather than a skip; leaving it silent is how the check would quietly stop checking. The clean-cell fixture in the integrity tests gained the field, and it caught the change: with the axis back at 0 the existing clean-cell test goes red too. Eight assertions added, each made to fail on its own targeted broken tree before being kept: the axis back at 0, the checker short-circuited, a missing count skipped rather than reported, the crashed-cell guard removed, the checker unwired from harness_failures, the counter removed from the loop, and mismatches accepted. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Do not report an action that did not run as an unverified paint floor Self-review of the floor check. An action that never ran carries no paintWaits, so the new check reported its floor as unverified on top of the failure harness_failures already raises for the action itself, with the reason. Two failures for one cause, and the second buries the first. Skipped only when the action explicitly reports ran false. An action that DID run and still has no count is still a failure, since that is the case where the subtraction is genuinely unverified, and there is a test on that side too so the skip cannot be widened into a blanket exemption. Both proven red: removing the skip, and widening it to always. * Measure the wall floor per window, and stop filing exceptions as chatter Two things. The generated wall ms axes declared zero double-rAF waits for every action, while the explicit axes declared theirs by hand. MENU_JS opens the recorder before opening the menu and closes it after closing it, so it crosses the same two waits menu open+close ms correctly declares, and menu wall ms was subtracting none of them. That leaves roughly two vsync floors of constant baseline in both ends of the ratio, which compresses it towards 1 and can label a primary metric flat on a small or fast fixture. Rather than hand-declare a second set of numbers, the recorder now counts the waits each window is clocked across and reports them, and the wall axes read that count from the row. growth() accepts a callable floor for this. Waits taken outside a recorder window, ACTION_SETUPS among them, are excluded by construction rather than by remembering to exclude them, and begin() zeroes the counter so a window cannot inherit the waits of the ones before it. Separately, console.error and uncaught pageerror went into the same list as Firefox's two scroll-anchoring notices and were tolerated by the same > 4 allowance. Engine chatter is the engine describing itself; an application exception is not that, and a single one inside a measured interaction means the interaction did not do what the row says. Severity is preserved now, the allowance applies to warnings only, and any error in seeding or in the measured actions fails the run. The counts and the first message are printed either way. Ten assertions added and each proven red on its own broken tree. Two did not go red on the first attempt: the severity assertion pinned an exact one-line expression and stayed green when the predicate was moved to its own line, and the counter-reset break did not apply at all because its needle was mis-indented. Both are fixed and both now fail as they should. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Require a meaningful rise before a zero-based counter counts as an answer No ratio can be formed against zero, so DISCRIMINATION_RATIO never applied to the counter axes at all and large > small was the entire test. The CI workflow runs one repetition on Chromium, so there is no median to smooth a stray dropped frame, and harness_failures accepts any ONE discriminating axis: 0 missed frames at 25K and 1 at 100K was marked as discriminating and could carry the whole liveness verdict while every latency axis was flat or broken. ZERO_BASED_MIN_RISE is what such a counter has to reach. 5 because these axes are dropped frames and long tasks: at twelve times the content a real curve produces them in quantity, while one or two is what an unloaded machine produces on its own. Absolute rather than a ratio because there is nothing to divide by, and the reason string now distinguishes a counter that rose too little from one that never moved, so a reader can see which happened. Five assertions, each proven red on its own broken tree: back to a bare large > small, the threshold lowered to 1, and every zero-based counter rejected outright. The last of those covers the control, since a check that rejects every counter would leave the harness unable to report a live run at all. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Say which axes are counts instead of inferring it from the paint floor The zero branch keyed on floored, which only identifies a timing that had a paint floor subtracted. An unfloored timing does not have one: longest stall ms and worst frame ms read zero at the smallest size whenever the action resolves before the recorder produces a sample, and were then judged as dropped-frame counters, so a noisy 5ms at the largest size read as a rise of 5 and discriminated. harness_failures accepts any single discriminating axis, so that stray millisecond could carry a run in which every valid latency curve was flat. COUNTER_AXES states which axes are counts. Only frames over 33ms is one; everything else is milliseconds. A timing that reads zero at the smallest size is now reported as having no rise to measure rather than being given a counter's credit. Three assertions, each proven red on its own broken tree: timings judged as counters again, no axis classified as a counter at all, and a timing axis classified as a count. The middle one matters because emptying the set would silently turn every counter into a timing and remove the only zero-based axis the liveness verdict has. One correction: the first version of the set assertion required a counter's name not to end in ms, which is wrong, since the counter axis is called frames over 33ms and does. It names the set exactly now, which is the point of classifying it explicitly. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put the floor COUNT in the report, not the thing that computes it Making the wall floor a callable put the lambda itself into the growth report. main() attaches that report to results and json.dumps it, so a complete run raised Object of type function is not JSON serializable after every measurement had already been taken, including the new CI smoke. That is my regression from the previous commit and it broke every full run. resolve_floor returns an int, growth uses it, and the report stores the resolved count at each end of the ratio rather than a boolean. A boolean would serialise and say nothing; the counts let a reader check the subtraction instead of trusting it. The reason no test caught this is that none of them serialised the report, so three assertions now do, and one of them covers the callable-floor axis specifically since that is the case that broke. Both proven red: the callable put back into the report, and a boolean marker in place of the count. * Keep fractional paint floors, and hold counters to the noise floor at any baseline Two follow-ons from the same review. resolve_floor cast to int. summarise takes a median across repetitions, so a run whose repetitions paid 1 and 2 waits reports 1.5, and truncating that left half a vsync floor in the wall axis. The documented two-repetition configurations are precisely the ones that produce halves. The median is kept as a float, which serialises fine. The noise floor only applied when a counter started at exactly zero. A dropped-frame count going 1 to 2 is a ratio of 2.0, cleared DISCRIMINATION_RATIO, and since harness_failures accepts any single discriminating axis, one incidental frame could carry the CI smoke while every latency axis was flat. A ratio on a counter is only meaningful once there are enough events for it to be about the content rather than about one frame either way, so the floor now applies whatever the baseline, and the reason string says which of the two rules rejected the axis. The floor stays a count of events and is NOT applied to timings, which would silently reject real latency curves that happen to sit at low absolute values. That has its own test. Five assertions, each proven red on its own broken tree: the floor truncated again, the noise floor skipped for nonzero baselines, the noise floor applied to timings as well, and every counter treated as noise, which is what covers the control. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Subtract the whole window's floors from the axes that span it quiet() and quietUntilIdle() return the elapsed time since this.startedAt, not the time they themselves took, and gestureMs is computed from startedAt as well. All three therefore span the entire recorder window and contain every double-rAF wait in it, and all three declared zero. For the scroll that is twenty vsync floors left in both ends of the ratio, which compresses it hard enough to report a real size-dependent regression as flat. scroll gesture ms, scroll settle ms and jump settle ms now take the measured paint_waits. Counted at runtime rather than declared, because the twenty come from a loop: the literal nextPaint count in the source is one, so any number written in here would have been wrong the same way the zero was. Deliberately NOT applied to everything. jump painted ms starts at a mark taken after begin() and spans one wait while the jump's window holds two, and MENU_JS awaits no paint at all, so its window count is zero while its two floors are real, coming from settle() reading the pre-MutationObserver state on entry for open and again for close. Giving either the window count would subtract a floor the number never contained, or drop one that it did. The rule is that an axis measured from startedAt takes the measured count and an axis measured from a later mark keeps a declared one. Seven assertions, each proven red on its own broken tree: each of the three axes back to zero, jump painted given the whole-window floor, and menu given a window count of zero. That last break also turned two PRE-EXISTING menu tests red, which independently confirms the menu axis really does carry both of its floors. There is an end-to-end case too: with the floors left in, a 16x scroll curve reads as 1.86x. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com>	2026-08-18 04:33:11 -07:00
Daniel Han	3a7c5ddfde	Time the install from CI, without changing the installers (#9153) ... * Make the installer say where its time goes Install Unsloth (--local, --no-torch) is the largest step in every Windows job: 260s of Windows API CI's 374s, 291s of Windows UI CI's 715s, 281s of Windows Update CI's 794s. The same install on Linux is 88s. Across the ~11 Windows cells a commit triggers that is roughly 50 minutes of Windows runner time per commit spent installing the same thing. Which phase spends it could not be answered from a CI log. Neither setup.ps1 nor setup.sh emits a timestamp anywhere, and the one Stopwatch in setup.ps1 is inside the llama.cpp source-build branch that CI never takes. Guessing would have been misleading: unsloth studio update over an already-complete install costs 297s, more than the 281s full install it follows, which is the opposite of what a download-bound install does. That number is the reason this lands before any caching work rather than after it. UNSLOTH_INSTALL_TIMING=1 prefixes every step/substep line with seconds since the script started, in both the PowerShell and bash installers, so one run of any install turns into a phase breakdown. Off by default and output is then byte-identical, which the tests check by running the bash helper rather than reading it: PowerShell treats every non-empty string as true, so a bare [bool]:... would have made UNSLOTH_INSTALL_TIMING=0 mean on. Enabled on the five Windows install steps, so the breakdown is in the logs from now on rather than needing another PR the next time this is asked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the installer print helpers self-contained tests/python/test_windows_setup_output_encoding.py dot-sources Get-StudioAnsi, Write-StudioLine, Write-StudioStdoutMirror, step and substep on their own and runs them, so a call out of step to a helper defined elsewhere in setup.ps1 is a hard failure rather than a warning. Inline the elapsed-time prefix in both installers and drop the helper. The PowerShell side reads its state through Test-Path so an unset script variable is empty rather than fatal under a caller's Set-StrictMode, and the guard test now asserts the two print helpers call nothing the probe does not dot-source alongside them. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Skip the setup.sh execution tests where bash is the WSL launcher The cross-platform parity job runs this file on windows-latest, where `bash` resolves to the WSL stub: it ignores the script, prints a UTF-16 "no distributions installed" notice and exits 1. That is not a finding about setup.sh, which is not the installer Windows uses. Probed rather than keyed off sys.platform, so a Windows box with a working git-bash still runs them, plus a sentinel test that fails on any platform that does ship bash. Without it a probe that quietly started returning False would skip both tests everywhere and stay green. * Time the outer Windows installer too, and the update runs Two gaps the instrumentation left, both of which kept the numbers it was added to explain out of reach. install.ps1 is what the Windows jobs actually run, and it does the uv bootstrap and the whole Unsloth dependency install itself before handing off to studio/setup.ps1. Only the child was instrumented, so the larger half of the 260-291s stayed untimed and the child's clock restarted at the handoff. install.ps1 now carries the same opt-in prefix and publishes its start as UTC ticks; setup.ps1 continues from that instead of counting from its own zero, and falls back to a local start when the value is absent or unparseable. The prefix is inlined and Test-Path guarded in install.ps1 for the same reason as in setup.ps1: tests/python/test_windows_setup_output_encoding.py slices those helpers out of this file as well and runs them alone. UNSLOTH_INSTALL_TIMING was also scoped to the install step, while the two `unsloth studio update --local` steps declare their own env. Those runs are the sharpest anomaly on record, a 297s no-op update after a 281s full install, and they were producing no breakdown at all. Set at job scope so both are covered, along with anything added later. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Bounds-check the timing handoff, restore it, and time the POSIX installer too Three follow-ups on the phase timing. The tick handoff was parsed with TryParse alone, which accepts -1 and 9223372036854775807. Both are outside DateTime's range, so the constructor threw, and under $ErrorActionPreference = "Stop" that made inherited junk a fatal installer startup error rather than the documented fall back to a local clock. It also ran when timing was disabled. Now gated on the switch and bounds-checked against DateTime.MinValue.Ticks and MaxValue.Ticks; -1, near-long-max, non-numeric and empty all fall back. The origin was exported and never restored. The documented `irm ... | iex` entry point runs in the caller's process, so it outlived the install: the next run in that session kept the old origin and a later `unsloth studio update` inherited it, both then reporting time since the first install. Saved and restored in the existing finally, like every other handoff variable in this script, and removed when there was no previous value. install.sh had the same gap install.ps1 did. It bootstraps uv and installs the dependencies before launching studio/setup.sh, and its own step/substep were untimed, so a Linux or macOS run had the same unattributed first half. Both POSIX halves now carry the prefix and share one origin, off by default and identical in shape to the Windows pair. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Set the timing origin at the handoff, and validate it on POSIX Restoring the origin in a finally was not enough. Install-UnslothStudio has several early returns above that block, including the install-lock failure and the "another install is already running" path, and they exit before any try begins, so a run that hit one still left UNSLOTH_INSTALL_TIMING_T0 behind in the caller's process. That matters because the documented irm | iex entry point runs there. The variable is no longer written at the top of the function at all. The origin is computed there, and exported only beside the other handoff variables immediately before the child launch, then restored in the finally that already covers them. That is the one region an early return cannot skip, so nothing between entry and handoff can leak. On POSIX the inherited value landed inside an arithmetic expansion, and $(( )) evaluates a bare word as a variable name: with set -u, UNSLOTH_INSTALL_TIMING_T0=junk aborted the installer with "junk: unbound variable", and "1;rm" was an arithmetic syntax error, so an unrelated outer process could stop an install that merely asked for timing. Both halves now accept only a plain non-negative integer and fall back to the local clock otherwise, including when the arithmetic yields a negative elapsed. Same reasoning as the tick bounds check already on the PowerShell side, which this had fallen a platform behind. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Time the install from CI, and leave the installers alone The previous approach put the clock inside install.sh, install.ps1 and studio/setup.*. That is a lot of surface for a log detail: it needed a UNSLOTH_INSTALL_TIMING switch with a different off-by-default rule per shell (PowerShell treats every non-empty string as true, so "0" enabled it), plus a UNSLOTH_INSTALL_TIMING_T0 epoch handed from the outer installer to the inner one, which then had to be bounds-checked in both dialects and unset again on every early return. Two of those hazards were reported on this PR. None of it is needed. Every step that runs an installer already pipes its output, so CI can prefix elapsed seconds as a pipeline stage and the installers stay exactly as they were. * POSIX: one stage in .github/actions/install-unsloth-local, which is the single definition behind 40 jobs. * Windows: the five install.ps1 pipelines, plus the two `unsloth studio update --local` steps in the update workflow. That update is the number worth explaining: a no-op update over a complete install costs 297s, more than the 281s full install it follows. The filter sits downstream of the log write, so logs/install.log keeps byte-for-byte what the installer produced and the ~30 steps that read that artifact are unaffected. interrupted-install-ci.yml matches ^\[TAURI:STEP\] anchored at line start against one of them, which is the reader that a prefix in the file would break silently rather than loudly. tests/studio/test_install_phase_timing.py asserts both halves: that the four installer scripts carry no timing machinery, and that in every prefixing step the log write comes first. Nine mutations were checked red, including two that were green on the first cut of the tests: an ordering check that read Tee-Object out of the explanatory comment above the pipeline, and a $LASTEXITCODE check that stayed green when deleted because the same name appears inside the child command string. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 04:23:09 -07:00
Daniel Han	fc325f431a	Stop running every macOS workflow on every commit to main (#9174) ... * Stop running every macOS workflow on every commit to main GitHub caps macOS at five concurrent jobs account-wide, across every repository, which makes a macOS runner slot the scarcest thing in this repo's CI. Four workflows scoped their pull_request trigger carefully and then declared `push: branches: [main]` with no paths filter at all, so after merge they ran on everything. Commit 6371f46a changes README.md and nothing else. It started Mac Studio GGUF CI, Mac Studio UI + API + Update CI, Mac Studio Install Matrix CI and Unsloth Tauri CI: seven macOS legs, 40 percent over the entire account cap, for a documentation typo. Every one of them then queued behind the others. Replaying each workflow's own pull_request filter against the last 40 commits to main puts the cost at 280 macOS job-slots today against 94 with the filters mirrored, and drops the peak on a single commit from 7 to 3, under the cap. Most of that is one workflow: Mac Studio Install Matrix fired on 40 of 40 sampled commits and would fire on none of them, at 4 slots each, and it also carries the worst queue in the set at a 200 minute median for a 2m38s job. No coverage changes. Each push list is its own pull_request list copied verbatim, which is the same question asked twice, and the post-merge backstop still runs whenever a commit can affect what the workflow tests. clean-machine-install-ci.yml and mlx-ci.yml already do exactly this and say why; this brings the other four in line. tests/studio/test_macos_slots_per_commit.py asserts no macOS workflow has an unfiltered push trigger, that the two filters do not drift apart in either direction, and that a README-only commit starts nothing. Its first cut reported workflow-trigger-lint and studio-inference-smoke as macOS workflows, because it scanned whole jobs and both merely mention macOS in a step; it now reads runs-on and resolves matrix expressions. Three mutations verified red. * List the helpers these workflows execute, now that the trigger is scoped Scoping a trigger is only safe if its path list is complete, and these were not. Five helper scripts and one auditor are invoked by name from a run step and matched no pattern in either list: studio-mac-inference-smoke assert-llama-loads.sh, hf-download-with-retry.sh, studio_smoke/multi_turn_chat.py studio-mac-ui-smoke assert-llama-loads.sh, hf-download-with-retry.sh studio-tauri-smoke scripts/lockfile_supply_chain_audit.py The gap predates this PR, but it was invisible while the push trigger was unfiltered, because every commit ran everything after merge. Narrowing the trigger is what turns it into a real hole: editing assert-llama-loads.sh would stop running the workflows that assert with it. studio-tauri-smoke also gains studio/package.json and studio/package-lock.json. Its Linux job installs and runs the Tauri CLI out of that package root (npm install --prefix studio, npx --prefix studio), so those manifests decide what it builds with. They are listed by hand rather than found, because that dependency does not go through a file path in a run step. test_every_helper_a_workflow_executes_is_in_its_trigger asserts the rest, from the run bodies rather than a fixed list, for both triggers. Verified red by dropping assert-llama-loads.sh again. * Cover the install matrix's transitive inputs, and run the guard on workflow-only PRs studio-mac-install-matrix listed install.sh and studio/setup.sh but not what they reach: setup.sh:1444 runs install_python_stack.py, which imports install_manifest.py, and install_llama_prebuilt.py imports prebuilt_core.py at line 55. A commit touching only one of those changed exactly what this matrix asserts on and did not start it. Not studio/backend/requirements/** or pyproject.toml, which install_python_stack.py also consumes. Generic install breakage on macOS is clean-machine-install-ci's job, and it lists both on either trigger with seven macOS legs; duplicating them here would re-spend the slots this filter exists to save. That is a judgement rather than an oversight, so it is written in the filter next to the paths it excludes. test_a_listed_python_input_brings_its_sibling_imports asserts the mechanical half: a listed .py must bring the modules it imports from its own directory. The full transitive closure of an installer is most of the repo, and chasing it is how a filter stops saving anything, so anything deeper stays hand-listed with a reason. The guard found install_manifest.py on its first run, which is one hop further out than the report that prompted it. The guard itself is now wired into workflow-trigger-lint, the one job with no paths filter. Every way it can regress is a workflow-only edit, which no other job matches, so it would otherwise have been collected first by Backend CI's unfiltered push -- after the change had merged.	2026-08-18 04:22:15 -07:00
Daniel Han	9ccc64d16e	Unblock Mac Studio GGUF CI, red on every main run since #8883 (#9155) ... * Unblock Mac Studio GGUF CI, red on every main run since #8883 Mac Studio GGUF CI has failed on every main run from ee68d9e2a onward, the merge of #8883 (refuse a gguf that cannot fit in free vram plus available ram). Bisected over 90 completed main runs: green through 31c42e872, red from ee68d9e2a, no recovery since. The guard is not wrong. From the failing run's own server log: Metal device looks virtualised (Apple Paravirtual device) ... GGUF inference will run on CPU Forcing gpu_layers=0 for gemma-4-E2B-it-UD-Q4_K_XL.gguf Error loading model: About 3 GB of this model does not fit in GPU memory and would run from system RAM. Only about 4 GB is available and 2 GB of that is kept free for the rest of the system, leaving about 2 GB usable. GitHub's macOS runners expose a paravirtual Metal device, and Unsloth already refuses to offload to one because paravirtual Apple GPUs return corrupt output. So the launch is --gpu-layers 0 --device none and the WHOLE model is a host mapping, not the partial spill the guard was written to price. It then measures honestly: gemma-4-E2B UD-Q4_K_XL (~2.4 GB) plus mmproj-F16 (~780 MB) does not fit in 2 GB usable. The load returns HTTP 400 and the phase fails several layers away from the cause. The workflow is what was wrong. It had been getting away with a model the runner cannot hold because the prompts are tiny and the mapping is paged, which is exactly the gamble the guard stops taking on a user's machine. CI now takes it knowingly with UNSLOTH_ALLOW_HOST_OFFLOAD, the escape hatch the refusal names, rather than silently swapping in a smaller model and quietly changing what mac vision inference is tested against. Set at job level, not on the vision phase: every phase there is CPU-only for the same reason, so a runner image with slightly less free RAM would take out the tool-calling phase next. tests/studio/test_mac_host_offload_optin.py keeps the blast radius honest. The opt-out disables a real safety net, so it asserts the reason stays written down next to it, that the Linux and Windows GGUF workflows never acquire it -- there a silenced guard would let a genuine host-offload regression pass green -- and that the guard keeps its own unit tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the host-offload guard on workflow-only PRs, and check workflow-level env The guard rejects a PR that removes UNSLOTH_ALLOW_HOST_OFFLOAD from the macOS GGUF job or adds it to the Linux or Windows one. Such a PR edits a workflow and nothing else, which Backend CI's paths filter does not match, so the assertion was never collected for the one change it exists to reject. Wired into workflow-trigger-lint, which carries no paths filter and already hosts the other workflow invariants for this reason. The blast-radius check also read only job and step env. GitHub propagates a top-level env to every job, so one line at the top of the Linux or Windows workflow would have disabled the guard across all of them while this test stayed green. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 01:32:35 -07:00
Daniel Han	68fec34d73	Run the three loaded-models-indicator engines at once (#9158) ... * Run the three loaded-models-indicator engines at once Loaded-models indicator (cross-browser) is the longest Linux job in CI at about 1000s, and about 870s of that is one step running the same Playwright suite three times in a row, once per engine. The runs are disjoint: each boots its own server and drives its own browser, so the serialisation bought wall-clock and nothing else. Two things were shared, and each is split rather than serialised. Each engine gets its own port, so three servers coexist, and its own UNSLOTH_STUDIO_HOME. The second one is the reason this was not already done: run-studio-indicator-browser.sh wipes $studio_home/auth so the boot mints a fresh .bootstrap_password, then reads that file back, and on a shared home one engine's wipe lands between another's mint and its read. Running the current step body concurrently against one home has two of the three engines read a password another engine minted. A per-engine home is cheap because UNSLOTH_STUDIO_HOME selects a data root only: unsloth on PATH still resolves the installed venv, the frontend is served from a package-relative path rather than from studio_root(), and the suite is API-only with the status endpoints stubbed via page.route, so a fresh home needs no model, no GPU and no llama.cpp build. The step now waits on all three engines before failing, so one engine's breakage no longer hides another's, and each engine's output is echoed in its own log group. test_the_linux_job_still_drives_all_three_browser_engines matched the literal '...sh 18899 <engine>' call form, which the loop no longer produces. It now asserts the property instead, scoped to the steps that invoke the helper so that the browser-install step naming all three engines cannot stand in as coverage. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Point each isolated indicator home at the installed studio venv UNSLOTH_STUDIO_HOME selects the CLI's install root, not just a data root, so the per-engine directories were empty installs: unsloth_cli resolves $UNSLOTH_STUDIO_HOME/unsloth_studio/ bin/python and exits "Unsloth Studio not set up. Run install.sh first." before binding a port. All three engines failed identically, which reads like a broken suite rather than broken isolation. Each per-engine home now symlinks the one venv install.sh already built and owns only the mutable state beside it, so the auth wipe still cannot race while nothing is copied or rebuilt. The step also checks the installed venv is there first, so a missing install is one clear error instead of three misleading ones. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 00:53:30 -07:00
Daniel Han	2073e0fe4d	Studio: load the settings tab panels when they are shown, not at launch (#8966) ... * Studio: load the settings tab panels when they are shown, not at launch * Studio: keep a settings panel that fails to load from taking the app down A panel is fetched the first time it is shown, so it can now fail where it could not before: offline, or a page whose entry bundle predates an in-place rewrite of dist/ and still names chunks that have been replaced. The dialog is mounted at the app root and nothing above it catches, so the throw unmounted the whole of Studio rather than one panel. Blocking a panel's module in a browser reproduced it: the dialog, its nav and the rest of the page went. The panel area now sits in an error boundary that offers a reload, and the Suspense fallback is a delayed loading line rather than an empty pane, so a slow first open shows something and a prompt one still shows no flash. Reload rather than retry: React caches a lazy rejection for the life of the page and the browser's module map caches the failed import, so re-importing the same URL rethrows without a new request. index.html is served no-store, so a reload does pick up the current chunk names. tests/settings-tab-panel-loading.test.ts gains a case that walks the JSX and asserts every panel Suspense is inside a class that defines getDerivedStateFromError. tests/studio/playwright_settings_tabs.py drives the real dialog in a browser: all twelve tabs, deep-open, the search jump, and the blocked-module case. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: consume a failed settings panel prefetch The idle prefetch warms every panel once the dialog opens, so a chunk it cannot fetch reached the page as an unhandled rejection for a tab nobody had asked for. Reproduced by blocking one panel's module in a browser: the rejection landed on window even though the boundary handled the panel that was actually on screen. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: typecheck the settings smoke entry, and stop the harness settling on a placeholder tsconfig.app.json lists the three existing smoke entries explicitly, so the new one was outside the project and npm run typecheck skipped it. Added, and the harness handle it installs on window is now optional, since app code sits in the same project and must not be able to reach a handle only the harness page installs. tsc --listFiles now names the file. The Playwright harness settled on whatever held still for 600ms. The panel renders from a deferred value, so a switch keeps the outgoing content up until the incoming panel is ready, and on a loaded machine that hand-off outlives the window: one run read the placeholder as the final panel and called a correct recovery a failure. It now refuses to settle on something with almost no content. A run that dies on a cold dev server also writes its report instead of leaving none. * Tighten the settings lazy-panel comments * Name the encoding when the settings harness writes its report tests/test_source_read_encoding.py holds every checked-in file read and write in the test trees to an explicit utf-8, so it does not depend on the platform default and break on Windows the day the file gains a non-ASCII byte. The report write was the one that did not. * Let the select's keyboard scroll settle before the font-scale wheel check Pre-existing flake in this step, not something this branch introduced. The step reads scrollTop straight after keyboard.press, but Radix scrolls the highlighted item into view off the back of that keypress, so the value is a mid-scroll sample: instrumented on the ubuntu CI image the viewport went on to settle 24-35px further down in 20 runs out of 20, on this branch and on its merge base alike. Two things break as a result. The stale sample is not the floor the wheel has to beat, which is why the failure reads '20 -> 44' as though the viewport had moved the wrong way when 44 is simply where the keyboard scroll ended up. And a wheel dispatched into a scroll Chromium is still animating can be swallowed outright, which is the actual failure: at a maximum scrollTop of 243 a working -400 wheel lands on 0 every time. So wait for the scroll to stop before taking the floor, keep the pointer inside a viewport that is not always 40px tall, and re-send the wheel on a bounded retry. A viewport that genuinely refuses the wheel still never moves and still fails, just after more tries. * Clear an unconsumed archive deep-open when settings navigates away * Run the settings tab-panel browser smoke in frontend CI * Keep an archive deep-open when the navigation lands back on Data * Keep a settings scroll target when its own tab is reselected * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Load the crypto polyfill on the settings smoke page * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Trim comments in the settings lazy-loading changes * Hold only the Data panel's own module in the settings deep-open test The abandoned-deep-open step routed every request through a handler that sleeps 2.5s, and that sleep runs on the driver thread, so the whole page's module load queued behind it and arrived at the main thread in one go. On a two-core runner sharing the box with a live Studio that pushed the reopen past its 15s timeout, which reads as a settings dialog that would not open when nothing was wrong with it. Route the Data module alone. The assertion is unchanged and still goes red on the pre-change store: the next ordinary visit to Data reopens the archive listing. * Name the cause when the settings smoke page has navigated away Vite dev proxies /api to 127.0.0.1:8888. With a Studio listening there and no token those calls answer 401, the app's auth handling navigates, and the harness window goes with it, after which every step times out waiting for a dialog that cannot exist. It happens on main too, where the harness is gone before the first open, so it says nothing about the panels. Report it. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com>	2026-08-18 00:10:58 -07:00
Daniel Han	1f09a17868	Studio: keep the streaming render harness the perf PRs kept rebuilding (#8969) ... * Studio: keep the streaming render harness the perf PRs kept rebuilding * Studio: close the false-green paths in the stream pacing harness The long-task total is the metric the budgets turn on, and it reads 0 both when the render is free and when the observer never ran. observe({type: "longtask"}) is specified to abort silently on an engine that lacks the entry type rather than throw, so the try/catch around it never fired: under firefox or webkit the harness scored a perfect zero and exited 0. Detect support with PerformanceObserver.supportedEntryTypes, record it, and fail the run when no long tasks were seen or when throttling was disabled. Also: - add the entry to tsconfig.app.json, which lists the smoke entries one by one, so npm run typecheck actually covers the 270 lines it was reported against - write the JSON under logs/ like every sibling harness instead of dropping an untracked stream-pacing.json in the repo root, and create the directory - treat an exported-but-empty SMOKE_BASE_URL as unset, matching the siblings; it drove "" as the base URL and burned the full readiness timeout - register the harness in the two contract tests, which is what surfaced the SMOKE_BASE_URL bug, and pin the new guards there - record the second mutation: reverting #7892 moves the longest stall 4-5x while leaving the long-task total inside the clean range, the exact opposite of reverting #8750, so both budgets are load-bearing * Studio: budget only the long tasks the stream itself caused buffered: true replays whatever the performance timeline already held, so module evaluation and the first React render landed in the budgeted total: one entry, ~140ms, about 2.6% of a clean run here, and larger on a cold or loaded runner. Nothing filtered by startTime and run() reset nothing, so a slow page load read as a slow renderer. Open the measurement window in run() and drop entries that began before it. Pre-stream share goes 2.6% to 0.00% while the stream's own tasks are unchanged (60 and 52 entries over two clean runs), and reverting #8750 still fails the budget at 52,465ms. * Studio: tighten the stream pacing harness comments Comments and docstrings only, no code change. Every measured number, PR reference and causal reason is kept verbatim. * Studio: check the reply that settled, not the peak it once reached paintedChars is a high-water mark and only ever climbs, so a completion render that truncated the bubble would leave the peak behind and the 90% workload floor would still pass on a DOM that no longer held the reply. Record what is on screen at settlement and check that too. Measured equal to the peak today (24,033 both), so this is a guard rather than a live discrepancy, and it is pinned in the harness contract test. Also count slow frames only inside the measurement window and reset the counter in run(), the same rule long tasks now follow. Contamination measured at 0 of 286 here, but an external server or a slower box need not be 0 and the number is meant to be comparable across them. * Studio: record a stall that never ends, and keep the task that starts the stream Two holes left by the measurement window added in the previous commits. A long task carries the start time of its whole task, so appending the user message in the same task that assigned measureFrom stamped runtime startup and the first publish as earlier than the window and dropped them as page load. Hand the append to a later task so the work that begins the stream sorts inside it. longestStallMs was only ever written when a later paint closed the stall, so a freeze that ran to the end of the stream was never recorded: the tail can go missing inside the 90% floor and the quiet-frame loop then calls it settled. Measure the stall in progress while text is still arriving, which is what the number means, and not afterwards, where the settle window's own quiet frames would read as a freeze. Clean runs unchanged (stall 933 to 1,050ms, long tasks 4,749 to 5,159ms over three) and both mutations still caught: #7892 reverted fails the stall at 5,233ms, #8750 reverted fails long tasks at 52,263ms. * Studio: tighten the comments added since the first pass Comments only, no code change. Every measured number and every causal reason is kept. * Studio: record a freeze that spans the end of the stream The stall in progress was measured only while text was still arriving. A freeze that spans the moment the stream ends blocks the frame loop across it, so the first frame afterwards already observes a non-null streamEndedAtMs and the whole frozen interval was skipped. With the lost tail able to hide inside the 90% workload floor, thirty quiet frames then settled the reply and the run reported a short longest stall, which is the one shape this number exists to catch. Cap the interval at the absolute stream-end timestamp instead. A freeze across that moment is recorded in full, and the stall stops growing once there is no more text to wait for, so the settle check's own quiet frames are still not counted as a freeze. The rule moves into smoke-stream-pacing-stall.ts so it can be tested without importing the harness entry, which mounts React on import. The new tests cover the spanning freeze, the settle-window bound, idempotence and late tail paint; restoring the previous rule fails two of the five. Clean runs unchanged (stall 967 to 983ms, long tasks 5,442 to 5,842ms) and both mutations still caught: #7892 reverted fails the stall at 5,017ms, #8750 reverted fails long tasks at 63,687ms. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make the stream pacing smoke page load again Merging main brought in ASSISTANT_PART_COMPONENTS, which thread.tsx builds at module scope with Text: MarkdownText. Entering the markdown-text -> features/chat -> chat-page -> thread cycle from markdown-text runs that object literal while the MarkdownText binding is still in its temporal dead zone, so the page died with Cannot access MarkdownText before initialization and rendered nothing. Import the chat barrel first, as the app's entry does. The page is also a new HTML entry, so it has to load the crypto polyfill before its module entry like every other one. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>	2026-08-17 22:38:56 -07:00
Daniel Han	9eb614207a	Give the small Windows checks one box per image instead of one box per check (#9143) ... * Bundle Unsloth GGUF CI onto one runner, matching Windows and macOS The three ubuntu-latest jobs differed only in model, port and test body; checkout, apt, setup-node, setup-python and install.sh --local --no-torch were identical in all three. Windows and macOS already run these three phases in a single job. This is the Linux port of that layout, step bodies unchanged. studio-windows-inference-smoke.yml carried a note saying the trade was not worth making on ubuntu, because setup there is ~1.9 min against ~3.0 min of tests and the sequential wall time would cost more than the slots are worth. The arithmetic was right; the premise that the three jobs start together is not what the runners do. Run 32089506294 started two jobs at 02:40:11 and the third at 02:46:52, then finished at 02:52:57: a 12m46s wall for about 6 min of work. Run 32089558062 staggered all three. Across 18 recent multi-job runs on main the median gap between a run's first and last job start was 175s, and 13 of the 18 exceeded 60s. Bundled is about 7m35 every time. When all three do get slots at once that is ~1m30 slower than the 6m05 longest job; when they do not it is 2-5 min faster. It also returns 2 of the 60 concurrent slots, which is what makes the contended case rarer for every other workflow. Each phase keeps its own model cache directory, HF_HOME, port, server log, artifact and step timeouts, and gates on the shared preamble rather than on the phase before it, so a phase-1 failure still lets phases 2 and 3 report. Phase 3 moves to hf-cache-vision with a key bump because phase 1 keeps hf-cache, whose key is byte-shared with the macOS and Windows gemma phases. tests/studio/test_gguf_smoke_phases_stay_independent.py asserts all of that from the workflow rather than from a list, since every one of these regressions is silent rather than red. * Use github.workspace for the xet scratch dir; runner is not a job-env context Actions rejects the whole workflow before scheduling a job, so the first staging push produced a run with zero jobs and only 'This run likely failed because of a workflow file issue'. Job-level env can read github, needs, strategy, matrix, vars, inputs and secrets, not runner. * Record the measured staging timing for the bundled job Green end to end on a staging repo with an empty actions cache, so every model downloaded cold: 6m03s for all three phases, of which 105s is the now-shared setup. The phases themselves are 26s / 179s / 36s. That is at or under the 6m05 the longest of the three jobs took on its own, on a third of the runners, so the earlier estimate of ~7m35 was pessimistic. * Give the small Windows checks one box per image instead of one box per check pester, no-vs-gpu-resolve and the two cells each of vs-integration and vcredist-clean-box are six Windows job-runs that execute for 16-34s apiece. On this repo's Windows pool that is not what they cost. Measured over recent main runs, every Windows job waits 2600-3400s for a slot regardless of what it then does: exec queue job 16s 2606s real-VS detection (VS 2022) 22s 3315s real-VS detection (VS 2026) 24s 3170s VC++ round-trip (windows-latest) 26s 3358s VC++ round-trip (windows-2025-vs2026) 33s 3311s setup.ps1 unit tests 34s 3391s GPU prebuilt resolves without VS 1108s 3164s Chat UI Tests So six slots deliver 155s of work, and the queue those slots help create is what the 18-minute Chat UI job waits in. Merged by runner image they are three job-runs and nothing moves platform: windows-2022 keeps its own box for VS 2022 detection, windows-2025-vs2026 keeps its own for VS 2026, and windows-latest carries the three checks that only need a stock Windows box. Ordering inside a box is load-bearing. The VC++ phase uninstalls the runtime and restores the registry in a finally, so it runs last everywhere. The phases before it install a PowerShell module, pip packages and a simulated no-build-tools tree under the workspace; none writes the VC++ registry keys the round-trip reads as ground truth, so its clean-box precondition still holds. The two long jobs (inference-smoke 718s, no-vs-cpu 464s) stay on their own runners: their cost is execution, not slot occupancy. tests/studio/test_windows_small_checks_stay_on_their_image.py pins the phase-to-image contract as data rather than reading it back out of the workflow, because a check running on the wrong Windows image mostly still passes. Stacked on #9139, which edits a comment in this file. Not verifiable locally: needs a cross-platform staging run. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Find the Pester phase by content, not by job id test_pester_bootstrap_hardening.py keyed on jobs['pester'], so grouping the small Windows checks by runner image turned all 8 of its assertions red on a rename that changed nothing they assert. The staged run caught it: 'Guard the Pester bootstrap' failed while every phase after it passed. Looks up whichever job installs Pester instead. Worth noting that the gating held exactly as designed under a real failure: the guard failed and the no-VS resolve and VC++ round-trip phases on the same box still ran and passed. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 21:13:48 -07:00
Daniel Han	7298bb9568	Bundle Unsloth GGUF CI onto one runner, matching Windows and macOS (#9139) ... * Bundle Unsloth GGUF CI onto one runner, matching Windows and macOS The three ubuntu-latest jobs differed only in model, port and test body; checkout, apt, setup-node, setup-python and install.sh --local --no-torch were identical in all three. Windows and macOS already run these three phases in a single job. This is the Linux port of that layout, step bodies unchanged. studio-windows-inference-smoke.yml carried a note saying the trade was not worth making on ubuntu, because setup there is ~1.9 min against ~3.0 min of tests and the sequential wall time would cost more than the slots are worth. The arithmetic was right; the premise that the three jobs start together is not what the runners do. Run 32089506294 started two jobs at 02:40:11 and the third at 02:46:52, then finished at 02:52:57: a 12m46s wall for about 6 min of work. Run 32089558062 staggered all three. Across 18 recent multi-job runs on main the median gap between a run's first and last job start was 175s, and 13 of the 18 exceeded 60s. Bundled is about 7m35 every time. When all three do get slots at once that is ~1m30 slower than the 6m05 longest job; when they do not it is 2-5 min faster. It also returns 2 of the 60 concurrent slots, which is what makes the contended case rarer for every other workflow. Each phase keeps its own model cache directory, HF_HOME, port, server log, artifact and step timeouts, and gates on the shared preamble rather than on the phase before it, so a phase-1 failure still lets phases 2 and 3 report. Phase 3 moves to hf-cache-vision with a key bump because phase 1 keeps hf-cache, whose key is byte-shared with the macOS and Windows gemma phases. tests/studio/test_gguf_smoke_phases_stay_independent.py asserts all of that from the workflow rather than from a list, since every one of these regressions is silent rather than red. * Use github.workspace for the xet scratch dir; runner is not a job-env context Actions rejects the whole workflow before scheduling a job, so the first staging push produced a run with zero jobs and only 'This run likely failed because of a workflow file issue'. Job-level env can read github, needs, strategy, matrix, vars, inputs and secrets, not runner. * Record the measured staging timing for the bundled job Green end to end on a staging repo with an empty actions cache, so every model downloaded cold: 6m03s for all three phases, of which 105s is the now-shared setup. The phases themselves are 26s / 179s / 36s. That is at or under the 6m05 the longest of the three jobs took on its own, on a third of the runners, so the earlier estimate of ~7m35 was pessimistic. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 21:13:01 -07:00
Daniel Han	e463194583	Split Chat UI Tests into four shards on the Studio boundaries (#9132) ... * Split Chat UI Tests into four shards on the Studio boundaries Chat UI Tests was the largest job in the repo: 2,514 minutes over a week, 22.1 minutes on average, 11 Playwright scripts run one after another against four Studios booted one after another. Timed on a real run, 17.6 minutes of which the setup every shard has to repeat is about 2.6 (checkout, Linux deps, Install Unsloth, Playwright browsers). The rest is the scripts, and the two banner engines alone are 7.25 of it, which is why they are a shard of their own rather than sitting with their neighbours. The boundaries are the Studio instances, not an even division of scripts: a boot, its health wait, its bootstrap password and the scripts that drive it have to stay on one machine. chat takes 18892 plus the cross-browser permission pass, extra and banner each boot their own 18894, picker takes 18898 and 18896. In-machine parallelism is deliberately not used. These are separate runners, so each shard still boots one Studio at a time and nothing competes for RAM, which is the constraint that rules out simply running the scripts concurrently on one box. Runner minutes go up and wall-clock comes down, which is the trade being made on purpose. The risk worth guarding is not a broken shard, which is loud. It is a step whose if: names no shard, or names one not in the matrix, or a shard left with nothing to do: the step runs nowhere, all four shards are green, and a regression suite has quietly stopped existing. The new test asserts coverage from the workflow itself rather than from a list, checks that a script and the Studio it talks to land on the same shard, and pins the count of driving steps at 11. All three mutations fail it: renaming a shard in one if:, dropping a gate, and moving a script away from its Studio. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Cache the Playwright browser downloads Three engines, downloaded on every job, on every run, uncached. It was 0.75 minutes when one job needed them; with four shards it is four times that, and it sits on the critical path of each. Keyed on the RESOLVED Playwright version rather than on the '>=1.45' spec, because that is what decides which browser builds are wanted: a floating spec that resolves higher must not hit a cache holding the older engines. --with-deps stays unconditional on a miss and becomes install-deps on a hit. The apt half installs system libraries outside the cached directory, so restoring the cache without it would give browsers that cannot start, which is the failure this kind of change usually ships with. Only the download is skipped. * Give each shard its own artifact name, and run this guard on workflow edits Two from review, both real, and the first would have made the change worse than what it replaced. Artifacts are immutable within a workflow run, so four cells uploading studio-ui-smoke-artifacts means the first to finish creates it and the other three fail on the conflict. The upload step carries if: always() and no continue-on-error, so a UI run where every test passed reports red on three cells out of four. The name now carries the shard, and the new assertion is written for any matrix job in this workflow rather than for this one by name, since the next job to be sharded inherits the same trap. Removing the shard from the name fails it. The second is the same gap this repo keeps finding: the guard reads studio-ui-smoke.yml, so the edit it exists to catch is by definition a workflow-only edit. Backend CI's paths filter does not match one, and the UI smoke job it protects is precisely the thing that would silently stop running, so it cannot be the one to notice. It joins the four guards already running in workflow-trigger-lint.yml, which carries no paths filter and sees every pull request. * Keep the 30 minute budget per shard I cut this to 20 on the reasoning that a quarter of the job needs a quarter of the budget. Staging then cancelled three of the four shards at exactly 20m0s, all of them inside "Linux deps", an apt step that takes 13 SECONDS in the org repo. The work was not slow, the runner was. A timeout is there to catch a hang, not to police the mean. Sizing it to the expected duration converts an infrastructure stall into a red build on a shard where every test would have passed, which is a worse failure than the one the tighter number was meant to prevent, because it looks like the change broke something. * Capture the server logs on every shard, not just the last one The copy of ~/.unsloth/studio/logs lived inside the step that stops the third Studio, under a comment saying all three Studios share the directory. That was true when they shared a RUNNER. Each cell now has its own machine and its own logs directory, so gating it to picker uploaded three artifacts with no server-side traceback in them, which is the first thing anyone debugging a failed shard opens. The copy is now its own always() step on every cell; the IME process cleanup stays picker-specific, since that PID only exists there. Re-gating the copy to one shard fails the new assertion. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 18:49:10 -07:00
Daniel Han	2748b15eb3	One interpreter leg on a pull request, and a floor lint that reads more than syntax (#9100) ... * One interpreter leg on a pull request, and a floor lint that reads more than syntax A pull request ran 3.10 and 3.13. It now runs 3.13 only. Main still runs all four, so anything that needs a real run is caught at merge rather than never. The leg that goes is worth something, so this pays for it rather than dropping it. What a dropped floor leg actually stops catching is not syntax: it is reaching for a stdlib name that does not exist yet. core/research_runs.py already uses anext, which is 3.10, and that parses perfectly on every version and fails only when the line runs, so the existing ast.parse floor check would not have seen it. scripts/lint_backend_python_floor.py asks vermin instead, which reads syntax AND stdlib API availability, and takes its target from the workflow's own matrix rather than a number written in the script. Adding a call to itertools.batched, which is 3.12, fails it in seconds. It runs from workflow-trigger-lint.yml, which carries no paths filter, so it sees the pull requests that touch only backend source -- the ones that most need it now. The single leg has to be the NEWEST. Removals and deprecations land on the newest interpreter first and on the oldest never, so running only the oldest would be the wrong single choice; the guard asserts which end it is. What is genuinely given up, kept visible rather than deleted along with the old guard: the backend has version_info branches at 3.10, 3.11 and 3.12 boundaries, and a pull request no longer takes both sides of any of them. Nothing static covers that -- a parse reads both sides and runs neither. test_the_boundaries_the_subset_stops_executing_are_still_run_on_main lists them and fails if main ever stops running the full matrix, at which point this stops being a trade and becomes a straight loss. Mutation tested: making the single leg the floor fails one test, dropping the lint invocation fails another, removing vermin from the install fails it too, and taking Backend CI off push-to-main fails two. That vermin check needed a second pass: the first version looked for the string anywhere in the workflow and was satisfied by a comment mentioning it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Scan the backend tree, not a list of packages I remembered The floor lint named core, utils and routes, and silently missed 116 shipped files: all of hub, plugins, models, storage, auth, picker and state, plus _platform_compat.py, which main.py imports directly. It also named "loggers.py", which is a directory, so that entry matched nothing at all. With the 3.10 leg dropped this lint is the only thing looking at the floor before a merge, and a check that covers most of a tree reads exactly like one that covers all of it. It now scans studio/backend and excludes only tests and vendored code, which takes it from 307 files to 422. Verified by putting an itertools.batched call, which is 3.12, into each of hub, auth, picker, state, storage, models, plugins and _platform_compat.py in turn: every one is caught now, and none of them was before. Widening it immediately found something real, which is the point: locale.getencoding is 3.11 and the floor is 3.10. It turns out to be correctly guarded, in a try/except AttributeError whose fallback is locale.getpreferredencoding(False), commented "Python < 3.11". vermin reads names rather than control flow, so a guarded attribute lookup is indistinguishable from an unguarded one. That file is exempt with its reason printed on every run, and an exemption naming a path that no longer exists fails the lint, so it cannot outlive the guard it was written for. The guard test counts what the lint would hand to vermin against what is on disk, so narrowing the input back to a package list fails rather than quietly shrinking coverage. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Suppress the guarded call, not the file it lives in Excluding state_store.py wholesale left everything else in it permanently unchecked, which is the package-allowlist mistake from the previous commit one level down: a new unguarded 3.12 call anywhere in that module would have passed the floor lint on a pull request that runs only 3.13. The suppression moves to the site. vermin honours a `# novermin` annotation, so comment parsing is on now and the one guarded call carries the annotation with a note saying the except below IS the guard and that vermin reads names rather than control flow. The file is back in the scan, which is 422 files again rather than 421, and adding an unguarded itertools.batched call elsewhere in it now fails. The coverage test no longer permits any file-level exemption at all, rather than permitting a recorded one, so reintroducing the exclusion fails it. Separately: the assertion that the lint step exists was only collected by Backend CI, whose paths cover its own YAML and studio/**, not .github/workflows/**. A pull request editing only workflow-trigger-lint.yml could therefore delete the step without failing anything, which is the one change the assertion exists to reject. It runs from that unfiltered workflow now, alongside the three guards already there for the same reason. * Scan unsloth_cli on the floor as well, since the matrix runs it studio-backend-ci lists unsloth_cli/** in its own paths filter and runs pytest unsloth_cli/tests as a step on every leg, so the 3.10 leg this replaces was executing shipped CLI code on the floor interpreter, not only backend code. A lint aimed at studio/backend alone covers part of that while reading like it covers all of it, which is the same shape as the package allowlist the previous round removed, one level up. ROOTS is now both trees and targets() walks each of them, 441 files rather than 422, and the run is still clean at 3.10. The guard asserts on what the lint would actually hand to vermin rather than on its source, and dropping unsloth_cli back out fails it. * Lint the test code at the floor too, since the matrix executes it The first version dropped tests on the theory that they are not shipped. Shipping is not the question, execution is: studio-backend-ci runs pytest tests/ from studio/backend on every leg, so a 3.11 API in a test file is executed by the 3.10 leg exactly as one in a shipped module is. With the pull request down to a single 3.13 leg, that leg and this lint would both pass and the failure would arrive on the push to main, which is the gap this exists to close. Only vendored code comes out now, pinned to its own support range. 1093 files rather than 441, still clean at 3.10, so this costs nothing today and closes the hole. Putting tests back into EXCLUDE_PARTS fails the new guard. * Run one interpreter and defend the floor statically The 3.10, 3.11 and 3.12 legs are gone from Backend CI, on pull requests and on main alike. Measured on one runner over the same tree, the four legs collected the same 26,320 tests and differed by exactly one: the >= 3.12 gate on test_demonstrates_the_underlying_stdlib_regression. 3.10 and 3.11 reported 26193 passed / 127 skipped, 3.12 and 3.13 reported 26194 / 126. That is 97 runner-minutes per push to run one identical suite four times and learn the value of a single skip marker, into a queue that has been observed 195 deep, and queue depth is wall-clock for every other workflow in the repo. What the older legs were really defending is that nothing reaches for a symbol newer than the floor, which is static. scripts/lint_backend_python_floor.py now checks exactly that, on every pull request, in seconds, across 1093 shipped and executed files, reading stdlib API availability rather than syntax alone. The floor is DECLARED, as PYTHON_FLOOR in the workflow, next to where the legs used to be. Deriving it from the matrix was right while the matrix ran several interpreters and becomes self-defeating with one: a 3.13-only matrix would move the floor to 3.13 and leave the lint asserting that code written for 3.13 runs on 3.13. 3.10 rather than the 3.9 pyproject.toml declares, because 3.9 is not true today. unsloth/models/_utils.py already uses dataclasses.dataclass(kw_only) and tempfile.TemporaryDirectory(ignore_cleanup_errors), both 3.10, so a 3.9 target fails on the tree as it stands. Either the declaration or those two call sites has to give, and that is worth its own change; this lint is what made the mismatch visible rather than what hides it. The cost is stated rather than buried. A static check does not run anything, so the sys.version_info branches in sitecustomize.py, native_path_leases.py, third_party_source.py and worker.py are now covered by reading and by the lint's view of the names they use, not by execution. The guard that used to assert main still ran them asserts instead that every file carrying such a branch is inside the lint's scan, since that is the only check left on them. * Keep executing the pre-3.12 branches, and pin the ceiling by name Two from review. The first is the honest objection to a 3.13-only matrix on push as well as on pull requests: a break in a supported older runtime path that uses no newer stdlib name passes the lint and is then executed nowhere. So the branches were counted rather than argued about. Seven backend files carry a sys.version_info comparison, at 3.10, 3.12 and 3.14. The 3.10 ones were never straddled even by the old matrix, whose oldest leg WAS 3.10, so every leg took the same side of them and dropping legs loses nothing there. 3.14 is above every leg there has ever been. What is genuinely lost is the pre-3.12 side of three files, and that is small enough to keep running: a second matrix entry on 3.11, the newest version that still takes that side, running those three files and nothing else. 57 tests in under seven seconds, beside the full leg rather than in front of it, so the critical path is the full leg either way. It is not a second copy of the suite, and the four legs it replaces are still gone. The second is that asserting the sole leg is merely above the floor let 3.11 or 3.12 satisfy it, which would give up the removals-and-deprecations coverage that is the entire reason the single leg is the newest one. The ceiling is now written down and compared by name, so moving it is a decision somebody makes and defends in the same change. Both new assertions fail when mutated: pointing the full leg at 3.12 fails the ceiling test, and pointing the spot-check leg at 3.13 fails the pre-3.12 test because it would then re-test what the full leg already covers. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 18:48:26 -07:00
Daniel Han	96cf275d71	Run the Backend CI matrix in parallel, like its sibling job already does (#9095) ... * Run the Backend CI matrix in parallel, like its sibling job already does The matrix leg is the longest job in the repo at 23.3 minutes, and it was the only large pytest run still serial. repo-cpu-tests has run -n 4 since it measured 806s -> 220s. Measured over the same tree in the same environment, serial against -n 4: 1322.6s -> 343.0s, and the results are identical -- 51 failed / 26190 passed serial, 51 failed / 26188 passed parallel, with the two failure sets compared name for name and equal. So nothing in the backend suite depends on the order it runs in. (Those 51 are one local environment missing peft and a diffusers pin. The point is that the two modes agree.) This is CPU bound and not memory bound: the suite loads no model, unlike the inference smoke workflows where four workers on one runner would not fit. The existing isolation guard caught this change, correctly: it assumed exactly one parallel pytest run, and the two now run over different trees. The repo-root job runs tests/ from the checkout, the matrix job runs the backend's own suite from studio/backend, so tests/studio/load_freeze is not a path that exists for it and demanding those ignores would be nonsense. The guard now tells the two apart and applies the isolation rules to the repo-root run only, and a new test pins the matrix leg as parallel so losing the flag shows up as a failure rather than as CI slowly getting slower again. * Keep the relative-timing tests off the parallel workers Staging caught what the local comparison could not: the 3.10 leg reported 'early markup cost 1.354s against the reference's 0.854s' and 'incremental cost grew 7.0x vs the reference's 11.5x', while 3.13 passed the same commit in 9 minutes against the 23 it used to take. test_streaming_stripper times itself against a reference implementation measured in the same process. Under four workers on four vCPUs one side of that ratio gets descheduled and the other does not, so the comparison stops being between two implementations. It is the same reason repo-cpu-tests already keeps load_freeze out of its parallel run, and it does not reproduce on a machine with cores to spare, which is why the local run agreed serially and in parallel. So it is ignored from the parallel run and runs again in its own serial step, and the isolation guard now holds that pair together for the backend run the way it already does for the repo-root one: putting the file back in the parallel run fails one test, deleting the serial step fails another. * Find the tight elapsed-time bounds by scanning, not by remembering Two more files assert ABSOLUTE elapsed time, and tightly: 50ms for a short-circuit that should not run the probe at all, and 100ms for a regex backtracking guard. Bounds that small sit inside one scheduler quantum, so under four workers on four vCPUs they measure the scheduler as much as the code. Both passed on staging, which is the problem: they would have flaked later, on somebody else's change. Twenty-two backend files assert some elapsed bound, and serialising all of them would give back most of what -n 4 buys. So the line is drawn at 0.1s, where the measurement stops being about the code, and the three files at or below it are ignored from the parallel run and rerun serially, which costs 2.2s and 1.7s. The guard now finds them by scanning rather than by listing. It reads with ast, so the name has to be assigned from a difference of two clock readings: grepping for '< 0.05' would match a float tolerance, and grepping for 'elapsed' would match anything. A new test asserting a 20ms bound fails that guard instead of buying a flake, which is verified by adding one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the Backend CI matrix in parallel, like its sibling job already does The matrix leg is the longest job in the repo at 23.3 minutes, and it was the only large pytest run still serial. repo-cpu-tests has run -n 4 since it measured 806s -> 220s. Measured over the same tree in the same environment, serial against -n 4: 1322.6s -> 343.0s, and the results are identical -- 51 failed / 26190 passed serial, 51 failed / 26188 passed parallel, with the two failure sets compared name for name and equal. So nothing in the backend suite depends on the order it runs in. (Those 51 are one local environment missing peft and a diffusers pin. The point is that the two modes agree.) This is CPU bound and not memory bound: the suite loads no model, unlike the inference smoke workflows where four workers on one runner would not fit. The existing isolation guard caught this change, correctly: it assumed exactly one parallel pytest run, and the two now run over different trees. The repo-root job runs tests/ from the checkout, the matrix job runs the backend's own suite from studio/backend, so tests/studio/load_freeze is not a path that exists for it and demanding those ignores would be nonsense. The guard now tells the two apart and applies the isolation rules to the repo-root run only, and a new test pins the matrix leg as parallel so losing the flag shows up as a failure rather than as CI slowly getting slower again. * Keep the relative-timing tests off the parallel workers Staging caught what the local comparison could not: the 3.10 leg reported 'early markup cost 1.354s against the reference's 0.854s' and 'incremental cost grew 7.0x vs the reference's 11.5x', while 3.13 passed the same commit in 9 minutes against the 23 it used to take. test_streaming_stripper times itself against a reference implementation measured in the same process. Under four workers on four vCPUs one side of that ratio gets descheduled and the other does not, so the comparison stops being between two implementations. It is the same reason repo-cpu-tests already keeps load_freeze out of its parallel run, and it does not reproduce on a machine with cores to spare, which is why the local run agreed serially and in parallel. So it is ignored from the parallel run and runs again in its own serial step, and the isolation guard now holds that pair together for the backend run the way it already does for the repo-root one: putting the file back in the parallel run fails one test, deleting the serial step fails another. * Find the tight elapsed-time bounds by scanning, not by remembering Two more files assert ABSOLUTE elapsed time, and tightly: 50ms for a short-circuit that should not run the probe at all, and 100ms for a regex backtracking guard. Bounds that small sit inside one scheduler quantum, so under four workers on four vCPUs they measure the scheduler as much as the code. Both passed on staging, which is the problem: they would have flaked later, on somebody else's change. Twenty-two backend files assert some elapsed bound, and serialising all of them would give back most of what -n 4 buys. So the line is drawn at 0.1s, where the measurement stops being about the code, and the three files at or below it are ignored from the parallel run and rerun serially, which costs 2.2s and 1.7s. The guard now finds them by scanning rather than by listing. It reads with ast, so the name has to be assigned from a difference of two clock readings: grepping for '< 0.05' would match a float tolerance, and grepping for 'elapsed' would match anything. A new test asserting a 20ms bound fails that guard instead of buying a flake, which is verified by adding one. * Teach the timing scan the two shapes it was blind to The scan only recognised a comparison whose left operand was a name assigned from a clock difference. Two forms in this suite are written differently and were silently skipped: time.monotonic() - started < 0.2 the difference inline (test_stt_download_followups) _elapsed(big) < 8 * _elapsed(small) a helper returning one (test_diffusion_checkpoint_resume) The second is not a near miss. It compares two wall-clock measurements taken in sequence, so descheduling one side and not the other breaks it at ANY magnitude, with no threshold to be under, which is the same reason test_streaming_stripper came out of the parallel run. It was still running under -n 4. So the scan now asks whether an expression IS a duration, however it was spelled: a name assigned from a difference, a difference written inline, or a call to a function that returns one, found by walking for a return of a clock difference at any nesting depth -- the helper in question is defined inside its own test. And a relative comparison is fragile regardless of magnitude, while an absolute one still has to be at or below the threshold. test_diffusion_checkpoint_resume joins the serial step, costing 8.9s. Adding either shape to a file that is not isolated fails the guard, both verified. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow the clock value through a container, and record what is benign The scan tracked names assigned a clock DIFFERENCE. test_tool_output_streaming compares first_seen_at[0] - started against finished - started - 0.5, where every term is an instant, no single name ever holds a duration, and one of them is parked in a list by a callback. Nothing in it looked timed, so the file kept running under -n 4 while asserting that a callback fired at least 0.5s before the child exited -- which collapses if the worker is descheduled while the child sleeps. So instants count now, not just differences, including one appended to a container, and the check walks the expression rather than reading its top node. That widened net found four more files, and only two are real: test_web_fetch_extraction compares parse time at two input sizes, and test_tool_output_streaming is the above. The other three are not performance claims at all. A sandwich, "before <= recorded <= after", cannot be falsified by widening the gap; a poll deadline inside a wait-for-condition loop is the pattern that replaces a guessed sleep; and "stamp < 0.0" compares against a sentinel. Those are in BENIGN_TIMING with their reasons, keyed on the enclosing function so an edit above them does not move the exemption onto something else. Keeping the net wide means a new benign pattern lands here too, so the failure message now says which of the three ways out applies rather than assuming the test is wrong. Verified: a stored-instant comparison added to an unisolated file fails the guard, and the two new files cost 39.5s and 13.3s in the serial step. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow a duration back through the name a helper returned it in The helper check required the return expression itself to read a clock. test_tool_call_parser_strict has def best_ms(depth): best = float("inf") for _ in range(5): t0 = time.perf_counter() ... best = min(best, time.perf_counter() - t0) return best t200 = best_ms(200) t400 = best_ms(400) assert t400 < t200 * 3.0 where the return reads no clock, and neither does the assignment that holds the result. Two links were missing, not one: a function counts as a timing helper if it returns any of its OWN timed names, and a name counts as timed if it was assigned from a call to such a helper. Helpers are resolved first for that reason, and to a fixpoint, so a helper built on another helper is found on the next pass rather than missed. Worth noting as a check on the scan rather than on this test: test_streaming_stripper is now found by the scan on its own, having been in the isolation list by hand since the run that started this. The rule and the list agree where before only the list knew. test_tool_call_parser_strict joins the serial step at 2.0s. A helper returning a duration through a local name, added to a file that is not isolated, fails the guard. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Isolate the keepalive test staging caught, and say what the scan cannot see test_tunnel_safe_long_post::test_the_route_still_pads_the_same_slow_load failed on a staging 3.13 leg that had been green. It patches the keepalive threshold to 0.05s and makes the work sleep 0.2s, then asserts the response starts with padding. Whether it passes turns on which of two timers fired first, and four times the threshold was not enough margin under four workers on four vCPUs. The scan did not find it and structurally cannot: it looks for assertions COMPARING clock-derived values, and this one has no clock in it at all. The dependency is implicit, between a patched constant and a sleep, and the assertion is on the result. Ten backend files pair a sub-second sleep with a small threshold. Since 4x margin already proved insufficient, the ratio is not a usable rule, and flagging all ten would serialise a large part of the suite on a guess. So that class stays with staging, which is the only thing that has ever caught one, and the limit is written next to BACKEND_ISOLATED rather than left for the next person to discover the same way. The file joins the serial step at 3.9s. * Isolate the heartbeat-count test, and correct the note about how this class is found test_scan_loras_off_event_loop counts how many times a heartbeat coroutine ticked during a 0.3s sleep and requires at least three. Descheduling the worker costs ticks without the scan being wrong, so it fails for a reason that has nothing to do with the code under test. Same class as the keepalive test in the previous commit, from the other direction: there the assertion was on a result, here it is on a COUNT, and either way there is no duration in the expression for the scan to find. The previous commit said this class was left to staging, "the only thing that has ever detected one". That was true when written and is not now: this one came from review, on a file staging had not yet failed on. Found by reading, not by scanning, is the accurate statement, and the note says that instead. The file joins the serial step at 1.6s. * Isolate the anthropic keepalive counts from the xdist workers Codex found test_scan_loras_off_event_loop by reading, not by running: it counts heartbeats across a 0.3s sleep, and the assertion compares a count rather than anything clock-derived, so the AST scan in this file is structurally unable to see it. That is a class, not a one-off, so I read the rest of the suite for the same shape instead of waiting for staging to hit it. One more: test_anthropic_messages asserts len(keepalives) >= 3 and >= 2 across a _time.sleep(0.24) stall past several shortened keepalive windows. A descheduled worker loses keepalives exactly as the heartbeat test loses ticks. It costs 2.2s to run serially. One false positive worth naming, because the grep that finds these is crude: test_diffusion_backend asserts len(staged) > 1 near a 0.2s sleep, but staged is a list comprehension over cached filenames with no timing in it. It also costs 152s, so matching on the pattern alone would have been expensive as well as wrong. * Read every link of a chained comparison, not just the first A tight bound is often written as a sandwich, and a sandwich is one Compare node whose leftmost operand is the literal floor. Requiring that operand to be timed made the scan skip the upper link entirely, so a file could hold an assertion of the shape it exists to find and stay in the -n 4 run with the guard green. test_llama_cpp_wait_for_vram_settle already writes a bound that way. Verified by running the scan over a file with 0.3 <= elapsed < 0.05, which it now reports and previously did not. The walk also reads Gt and GtE by swapping the operands, since a bound written backwards bounds the same thing. That turned up one live case, an access token asserted to expire after the wall clock. The margin is 600 seconds, so reading both sides late by whole seconds still leaves it true, and it goes in BENIGN_TIMING rather than into the serial step. * Stop the test stubs shadowing httpx once the suite runs in small pieces The 3.10 leg failed collection on two of the ten files in the new serial step, on module 'httpx' has no attribute 'Response', and it is worth being precise about what that is: httpx is installed on that job. Thirteen backend modules build a fake one and install it with sys.modules.setdefault, which reads as deferring to the real library and does not. sys.modules holds what has been IMPORTED, not what is installed, so in a process where nothing has touched httpx yet the stub wins and shadows it for the rest of the session. The stubs have no Response, starlette.testclient reads httpx.Response at import, and everything collected afterwards that reaches fastapi.testclient or routes.inference dies. In a 26,000-test run something always imports httpx before any of them is collected, which is why this has been invisible for as long as the suite ran as one process. Splitting the timing tests out removed the accident rather than introduced the bug, and any future split would have found the same landmine. All thirteen now try the real import first, the form test_llama_cpp_placement.py already uses. Reproduced before the change by collecting wait_for_vram_settle and diffusion_checkpoint_resume together, which errored, and after, which collects 154 tests. The full suite still collects 26391. The guard is scoped to the isolated files. Around fifty other modules stub structlog the same way and are load-bearing in a run that also imports the real one, so rewriting those is a separate change with its own risk. What has to hold here is that anything moved OUT of the parallel run stands on its own. * Propagate helpers through assigned results, and isolate one more tick count Two from review, both real. The fixpoint over timing helpers called _timed_names without the helpers it had already found, so a wrapper that assigns value = base() and returns value never learned that value was timed. base was discovered, the wrapper was not, and any relative benchmark built on the wrapper stayed invisible. The pass that learns a helper is not the pass that reads its callers, which is the whole reason this runs to a fixpoint, so the set has to go in as well as come out. Verified on a base/wrapper pair the scan now reports and did not before. test_profile_stats counts event-loop ticks during a 0.5s blocking call and needs more than ten of the roughly fifty that fit. That is the same shape as the two tick counts already isolated: no clock appears in the assertion, so the scan cannot see it, and a descheduled worker loses ticks. 12.8s serially. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 08:23:36 -07:00
Daniel Han	5a1655ca1b	Studio: run the frontend unit tests on Windows in CI (#9099) ... * Run the frontend unit tests on Windows in CI, and fix the one test that cannot pass there Frontend CI is ubuntu-only, so a test that is correct on POSIX and wrong on Windows stays green forever. Thirteen such failures accumulated across three files before anyone ran the suite on a Windows runner. Add a windows-latest job that does checkout, node, npm ci and npm test, and nothing else: typecheck, build and the browser smokes cannot fail there for a reason they would not also fail on ubuntu, and repeating them would roughly triple the workflow. background-load-notice's stall-window test cannot pass on a Windows timer, and was measuring nothing on Linux either. Replace the millisecond budget with a comparison against the same loop with no healthy read. * Audit the lockfile before the Windows install, and run no install script Review: jobs run concurrently, so the ubuntu job rejecting a lockfile does not help a Windows runner that has already installed it, and npm 10 ignores the npm-11-only allowScripts policy. Run the same pre-install structural scan here, and install with --ignore-scripts so no package script runs on this runner at all.	2026-08-17 07:29:31 -07:00
Daniel Han	68815aa888	Do not let the signal handler's own logging stop it dying (#9083) ... * Do not let the signal handler's own logging stop it dying A cancelled launcher exited 0 instead of dying of its signal, intermittently, and only on loaded CI runners. The cause was in the handler's first line. A signal handler runs on the main thread wherever that thread happened to be. If it was inside a write to stdout, the interpreter refuses the second one: RuntimeError: reentrant call inside <_io.BufferedWriter name='<stdout>'> That is what the handler's opening _log call did. The exception escaped the handler before it could re-raise the signal, main()'s except BaseException caught it, and the process exited on whatever code main computed. A cancelled job then reads as a completed one. It needs the signal to land inside a print, which is why it never reproduced locally and why it took a contended runner to show it. The line that identified it is the diagnostic added in 9079, which printed the launcher's own log on failure. Two changes, either of which is sufficient, kept because they fail independently: logging from the handler is now best effort, falling back to os.write, which goes straight to the file descriptor and takes no lock the interrupted frame could hold; and the death moves into a finally, so nothing above it can prevent the process dying of its signal. The regression test makes the reentrancy deterministic by raising from the handler's log call rather than racing a print. Against the current main it fails with returncode 0, which is the symptom as observed. Worth recording: 9072 attributed this to a transient OSError from release(). That mechanism is real and its fix stands, but it was not what CI was hitting. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Drop the handler's diagnostic rather than blocking on it Raising is not the only way a log line can stop the handler. stdout in CI is a pipe, and if the collector stops draining it a write parks in the kernel instead of failing, so no except and no finally runs. The announcement is the FIRST thing the handler does, before release(), so the kernels would keep billing until something killed the launcher from outside: the one outcome this file exists to prevent. That backpressure is also what leaves the main thread interrupted mid-write, so it arrives with the reentrancy the previous commit fixed rather than instead of it. _log_from_signal now asks whether the descriptor is writable before touching it and drops the line if not. POSIX reports a pipe writable only when PIPE_BUF bytes fit, and these lines are far shorter, so a positive answer means the write completes. Any error from select answers no. The new test reproduces it rather than simulating it: the launcher writes until the pipe is full and the test never reads a byte, so the main thread is asleep inside a write, holding the buffer lock, when SIGTERM lands. Without the guard it hangs the full 120s budget and the kernel is never deleted. 536 kaggle tests pass. * Move the nonblocking check into _log, so release()'s own logging is covered The previous commit guarded only the lines the handler prints itself. release() logs through the ordinary path -- delete_kernel() reports a refused delete that way -- so on a full stdout pipe the handler still stalled, just one frame deeper, before its retry and before the finally that re-raises the signal. Same outcome: a kernel billing until something kills the launcher from outside. The check now sits in _log, gated on a flag the handler sets on entry, so every line printed from that point on drops instead of stalling however it was reached. Nothing changes on the ordinary path, which is every line this script prints before something kills it. The regression test missed this because its fake deletion succeeded silently. It now logs on the way through, which is what a real refused delete does. Remove the guard and the test hangs the full 120s budget with the kernel never deleted; the previous commit's fallback guard does not save it. 536 kaggle tests pass. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put every stdout line behind the handler's protections, not just the handler's own Two paths still bypassed them. release() warns about a kernel it could not delete with a raw print(), which is emitted on exactly the branch where a kernel is still billing, so a full pipe stalls the handler there before the finally can re-raise the signal. And _log dropped a line when the descriptor was unwritable but still raised when the buffer lock was held, and that reentrancy propagates out of delete_kernel() and out of release(), so a kernel Kaggle would have accepted on its third retry is never asked a third time. So the split is now between what a line IS and how it is written. _write_line carries the protections, _log adds the [launch] prefix on top, and the leaked-kernel warning goes through _write_line directly: through _log it would arrive as '[launch] ::warning ...', and GitHub matches an annotation from the start of the line, which would silently demote the one message that says a kernel is still billing. Three tests, each failing on the unfixed shape: a delete that is refused twice and accepted on the third still makes all three attempts while every write raises; a launcher whose deletes never succeed still dies of its signal with a full pipe; and the warning is read back out of a real main() run rather than from the source. The first version of that last one called the writer directly and passed on the mutation it was meant to catch. 536 kaggle tests pass. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 05:06:51 -07:00
Daniel Han	fa6be357dd	One multi-turn smoke script for all three operating systems (#9086) ... * One multi-turn smoke script for all three operating systems The check ran inline in the Linux, macOS and Windows smoke workflows as three copies of the same script, and being three copies is how one of them stopped checking. On 2026-05-22 an unrelated event-loop fix (#5669) turned the Linux copy's determinism assertion into a printed warning. macOS and Windows kept it, and are otherwise identical in logic: diffed, the only differences are that warning and some comments Windows had dropped. Nothing compared the three, so for three months the leg that runs on every pull request was the one not asserting, while the two that still asserted run rarely. The script moves to .github/scripts/studio_smoke/multi_turn_chat.py and all three call it. Nothing in it was platform specific; the only thing that differed per caller is the port, which it already read from BASE_URL. That removes 264 lines of YAML heredoc. It reads no environment and imports no SDK at module level, so the checking half is importable, and the new tests exercise it rather than grepping it: divergence between the two runs raises, trailing whitespace alone does not, an empty reply raises, and a transcript with no history grounding raises. Reverting the script to the warning form fails one of them; re-inlining a copy into a leg fails two. Worth stating plainly: this re-enables on Linux an assertion that has not run there since May. If greedy decoding is genuinely not reproducible on that leg, it will now fail rather than print. That is the right way round, and it will be one failure in one place rather than three copies to reconcile. The guard runs from workflow-trigger-lint.yml, which carries no paths filter, because a pull request that re-inlines one leg edits a workflow and nothing else. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Assert both runs are non-empty, not just the first The stripped comparison cannot tell the two apart: a second run returning "" against a first returning "\n" compares EQUAL, so a server that stopped answering halfway through printed OK. The Linux copy asserted both before this was consolidated; the macOS copy it was taken from asserted only the first, and I kept that half. The test now covers an empty SECOND run and the exact whitespace-against-nothing pair the comparison is blind to. Dropping the new assertion fails it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 04:59:17 -07:00
Daniel Han	66188b8d59	Let every main run finish instead of cancelling it before it starts (#9082) ... * Stop a merge burst cancelling main's CI before it starts cancel-in-progress: false does not keep a main run alive, and 27 workflows say it does. GitHub cancels any PENDING run in a concurrency group as soon as a newer one is queued; cancel-in-progress governs only runs already executing. The reference is explicit: 'any existing pending job or workflow in the same concurrency group will be canceled and the new queued job or workflow will take its place.' So on main, where pushes land in bursts, the runs in the middle of a burst are cancelled regardless. Both incidents this repo wrote down are that: studio-ui-smoke records a break sitting on main for 14 hours behind four cancelled runs, and the comment added to prevent it cannot. Today four merges in 37 minutes cancelled three consecutive main runs of Backend CI, which completed none, so main went untested across the whole batch. studio-backend-ci.yml and studio-ui-smoke.yml now key their group on github.sha when the ref is main, so each commit gets its own group and nothing supersedes it. Both are here because a regression already went unreported behind cancelled main runs of exactly these two. Pull requests are untouched: the sha term is empty off main, so superseded PR runs are still cancelled, which is the expensive direction to get wrong. The cost is real and worth naming: during a burst, main now runs these two per commit rather than once at the tip. That is what per-commit attribution costs, and it is what the incident comments were asking for. The remaining 26 keep supersession, since paying it repo-wide is not obviously right, but they stop claiming a protection they do not have. A guard asserts the two are grouped per commit on main, that pull requests still get latest-only, and that no workflow claims main protection while sharing a group across main commits. Each is mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Let every main run finish, not just two Widened from the two workflows with written-up incidents to all 28 that run on push to main. Cancelling a pending run is not a saving: the four cancelled Backend CI runs today recorded ZERO jobs each, so nothing was reclaimed, and main simply went unverified across five commits. The two Kaggle workflows are the exception and keep one shared group. They spend an external GPU quota rather than runner minutes, so a superseded run there genuinely should be dropped instead of replayed per commit. The guard is general now rather than a list of two: every workflow triggered on push to main must be grouped per commit, must still key on github.ref so pull requests keep latest-only, and must not claim a main protection while sharing a group. It also asserts the scan found something, since a broken glob would pass everything. Mutation-checked four ways: reverting one workflow to a shared group, applying github.sha unconditionally, pointing the exemption at a missing file, and adding the old claim to a workflow that still shares a group. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Evaluate the concurrency group instead of grepping it, and run the guard on workflow-only PRs The predicate was "github.sha" in group, which is not the invariant. Reverse the conditional to github.ref != 'refs/heads/main' && github.sha || '' and the substring is still there, every workflow still names refs/heads/main, and main commits share one group again: the guard passed on the exact regression it exists to catch. It now renders the group for two SHAs on main and requires them to differ, and for two SHAs on one pull request and requires them to match, so which branch supplies the SHA is what decides. The evaluator refuses to guess at syntax it does not model, and a separate test turns that refusal into a named failure rather than a silent pass. Also invoke this file from workflow-trigger-lint.yml. It reads every workflow's concurrency block, so the PR that breaks it edits some other workflow's YAML, and no workflow filters on .github/workflows/**. A revert of wheel-smoke.yml to a shared main group collected no test that reads it. The lint job carries no paths filter by design, which is the same reason the Playwright coverage guard sits there. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 03:59:13 -07:00
Daniel Han	1bf4be70e3	Studio: budget the JavaScript that runs before the first screen (#8964) ... * Studio: budget the JavaScript that runs before the first screen * Report the eager chunk count without budgeting it * Close the ways this gate could pass without measuring anything Three of them were reachable. Run through a symlinked checkout, the main-module guard compared process.argv[1] (the path as typed) against import.meta.url (the real path), they disagreed, and the script exited 0 having done nothing. That is anything under /tmp on macOS. Both sides now go through realpath. With build.modulePreload: false Vite emits the entry script and no preload links. The empty-set check did not fire, and a real build of that shape measured 424 KB of a 5,207 KB startup path and reported 4.8 MB to spare. The guard now needs two chunks from Vite, and it counts scripts and links together so the layout Vite uses when the entry module is nothing but imports (one script per chunk, no links) still passes. A chunk named in index.html but missing from the build threw out of readFileSync, exiting 1 with a stack, indistinguishable from a budget failure. It now reports the file and exits 2. Also: a parser-blocking classic script is charged to startup. index.html loads public/theme-boot.js that way, before the module graph, and it was outside the budget it belongs in. defer/async and cross-origin scripts are not counted. Matching is now case-insensitive and treats rel as a token list, so a partial match cannot quietly shrink the measured set. Total on this build goes from 5,207.2 KB raw / 1,496.2 KB gzip over 44 chunks to 5,208.3 KB / 1,496.8 KB over 45, still inside budget. * Judge a script tag on whether the browser runs it type="application/javascript" and the other JavaScript MIME types are classic scripts too. Matching only text/javascript would have let one sit outside the budget, which is the same silent under-measurement the rest of this is about. importmap and application/json still do not count: they are not code that runs. * Charge deferred scripts, and size non-asset files as they are served A deferred classic script runs after parsing but before DOMContentLoaded, in document order with the module entry, which is itself deferred. It is on exactly the timeline this budgets, so excluding it left a way to move startup JavaScript out of the budget without moving it off the startup path. Only async is excluded now, and async is the attribute tested because it wins when a tag carries both. The transfer column was gzip for everything, but the backend gzips the /assets mount only; anything else goes out through a plain FileResponse. theme-boot.js is the one such file today, and charging it gzip understated what actually crosses the wire. Non-asset files are now charged their raw size, and the column is called transfer rather than gzip, which is what it has always been measuring. 5,208.3 KB raw / 1,497.3 KB transfer over 45 chunks, still inside budget. * Anchor attribute matching on whitespace, not a word boundary A hyphen is a word boundary, so the \btype pattern matched inside data-type and read the decoy in preference to the real attribute. The same held for data-src, data-href and data-rel. Reproduced: a tag with data-type alongside type="module" dropped the entry entirely while its preload links kept the shape guard satisfied, which is a silent under-measurement of exactly the kind the rest of this exists to prevent. Every attribute in a tag is preceded by whitespace, the tag name included, so whitespace is the boundary HTML actually gives us. * Read index.html with a tag scanner instead of a regex Searching the whole tag text for an attribute name found it in other attributes' values, and treating the first > as the end of the tag ended it inside a quoted value. <script data-mode="load async later" src="/theme-boot.js"> <script data-note="a > b" type="module" src="/assets/entry.js"> The first has no async attribute and is parser-blocking, so it belongs in the budget; the second's entry is /assets/entry.js. The old code dropped both, and in each case the rest of the build still satisfied the shape guard, so the gate reported a comfortable pass over a startup path it had not measured. Attributes are now parsed off each start tag: > ends a tag only outside a quoted value, and a name is only read where a name can begin. Comments and inline script bodies are skipped, which a stateful scanner has to do to stay in sync, and which also stops a commented-out script being charged. * Count async scripts that are asked to block rendering The exclusion of async assumed it has no ordering relationship to the first screen. blocking="render" creates exactly that relationship, and it is the documented way to keep a boot script off the parser without letting the unthemed page paint, which is what theme-boot.js is for. Per the spec an element is potentially render-blocking if its blocking tokens set contains render, OR if it is implicitly potentially render-blocking; the async carve-out lives only in the implicit half, so the explicit attribute applies to an async script too. Measured rather than assumed. Holding /slow.js for two seconds moved first contentful paint from 28 ms to 2,020 ms in Chromium 151, which also reports the request renderBlockingStatus as blocking, and from 11 ms to 2,009 ms in WebKit 26.5. Firefox has not shipped it and treats the script as plain async. Left uncounted, such a script delays the first screen by its whole fetch and evaluation while the entry and preloads keep the shape guard satisfied. * Recognise every JavaScript MIME essence a browser still runs The set held the four spellings anyone writes today, but the rule it cites is the spec essence list, which has sixteen. The other twelve are not dead letters: measured in Chromium 151, application/x-javascript, text/jscript, text/javascript1.5, text/livescript, application/x-ecmascript and text/x-javascript all execute. So a startup script tagged with one of those was fetched and run by the browser and left out of the budget, while the entry and preloads kept the shape guard satisfied. The same probe confirms the two exclusions already relied on here: text/javascript; charset=utf-8 and application/json do not execute, because the attribute is matched against the whole essence string and a parameter makes it match nothing. The list is frozen upstream, so it does not grow. * Require a module entry before trusting the preload links The shape guard counted entry scripts and preload links together, so 48 links carried it on their own and a build whose entry was misread still measured and passed. The entry chunk is the largest single thing on the startup path, so that is the worst place for the total to paper over a gap. Preloads without an entry is not a shape Vite emits. A modulepreload link exists to announce the entry a static import closure hangs off, so links surviving while the entry does not means the entry was read wrong. The total still decides whether this is a code-split build, which keeps the inlined-entry layout passing: several module scripts and no links at all is a complete measurement. This is the residue of two mis-parses fixed earlier in this branch. Both did their damage the same way, by dropping the entry while the links kept the guard satisfied, so the invariant is worth stating outright rather than relying on the parser never being wrong again.	2026-08-17 03:35:18 -07:00
Daniel Han	a41f77853c	Run both ends of the interpreter matrix on a pull request, all four on main (#9080) ... * Run both ends of the interpreter matrix on a pull request, all four on main Measured on one runner over the same tree, the four legs collect the same 26,320 tests and differ by exactly ONE of them. 3.10 and 3.11 report 26193 passed and 127 skipped; 3.12 and 3.13 report 26194 and 126. The single difference is the sys.version_info >= (3, 12) gate on test_demonstrates_the_underlying_stdlib_regression, which documents a stdlib behaviour change. They cost 97 runner-minutes per push, 1663 + 1249 + 1419 + 1506 seconds, against a queue observed 195 deep. Pull requests now run 3.10 and 3.13, which keeps both ends of the range and keeps that one gate straddled. Pushes to main still run all four, which the workflow already triggers on, so an interior-version break is caught on merge rather than never. That is the trade: two interior legs, for half the cost on every PR. A guard asserts the subset keeps both ends, that it loses no version gate the full matrix can see, and that main still runs everything. The gate check compares the two matrices rather than demanding the subset straddle every gate outright: one test gates on >= 3.10 and the full matrix starts AT 3.10, so neither list can see that one, and holding the subset to a standard the full matrix does not meet would fail forever. Each of the three is mutation-checked. * Run one interpreter on a pull request, all four on main Measured on one runner over the same tree, the four legs collect the same 26,320 tests and differ by exactly ONE of them: the sys.version_info >= (3, 12) gate on test_demonstrates_the_underlying_stdlib_regression. 3.10 and 3.11 report 26193 passed and 127 skipped; 3.12 and 3.13 report 26194 and 126. Four legs cost 97 runner-minutes per push, 1663 + 1249 + 1419 + 1506 seconds, against a queue observed 195 deep. What the older legs were really guarding is syntax and evaluated annotations on older interpreters, and tests/test_python39_compatibility.py already does that statically, against the floor pyproject.toml declares (3.9) rather than the oldest leg here (3.10). It parses every packaged module with feature_version set to that floor and separately catches PEP 604 unions in evaluated positions, which is the failure that created it. One AST pass on one interpreter, covering a version this matrix never ran. Pull requests therefore run the ceiling alone: removals and deprecations land there first, and the one version-gated test executes there rather than skipping. Pushes to main still run all four. What that leaves uncovered until merge is a stdlib BEHAVIOUR difference on 3.11 or 3.12, which no static check can see. One test in 26,320 is gated on such a thing. The guard asserts the pull-request leg is the ceiling, that the static floor check still exists and still parses at the declared floor, that the declared floor is below the matrix floor, and that main still runs everything. All four are mutation-checked, including deleting the floor check outright. * Keep the floor leg: a static parse cannot run a version-conditional branch The single-leg version was wrong. The static floor check covers older SYNTAX, and I took that to cover older interpreters generally. It does not: it parses, it does not run, so a branch taken only on an older version is never executed by it. The backend has several. sitecustomize.py repoints pathlib's pre-3.11 _NormalAccessor, which 3.11 dropped. native_path_leases.py and third_party_source.py branch on >= 3.12. worker.py branches on a minimum below that. On 3.13 alone the older side of each is dead code for the whole pull request. Pull requests run 3.10 and 3.13 again, both ends of the matrix. Main still runs all four. The guard now scans the backend SOURCE as well as its tests for version_info comparisons, and requires the pull-request matrix to straddle every boundary the full matrix straddles. That is the check that would have caught this: it rejects the single-leg matrix outright. Compared against the full matrix rather than in absolute terms, since >= 3.10 and < 3.14 sit outside the matrix range and no list here can see them.	2026-08-17 03:08:23 -07:00
Daniel Han	4d1f9c519e	Stub unsloth in test_safetensors_reasoning_stream so it can run first (#9027) ... * Stub unsloth in test_safetensors_reasoning_stream, and close the guard hole Three tests in this file fail on main if the file runs first, or on its own, against the dependency set Backend CI installs: ImportError: Unsloth: Please install unsloth_zoo via `pip install unsloth_zoo` Which PR, and which side is wrong: #5620 (2026-07-06) added this file, with no stub, from the start #8342 (2026-08-10) added the stub guard, which does not look here Neither is wrong about its own subject, and neither is reverted. #5620's tests are correct; they just relied on another file having stubbed unsloth first. #8342's guard exempts imports inside a function on purpose, because those are lazy and cannot fail COLLECTION, which is the disaster it was written for. The hole is that pytest.importorskip USED to make a lazy import safe and no longer does. Since pytest 8.2 it skips on ModuleNotFoundError only, and unsloth/_gpu_init.py raises a plain ImportError when unsloth_zoo is missing, so the call fails instead of skipping. #8342's exemption was sound when written and stopped being sound under a pytest it did not choose. Fix: install the same stub the sibling files install, at module scope. Stubbing rather than skipif keeps the coverage: the module under test is still the real core.inference.inference, and only unsloth is faked. A skipif would have made three tests silently stop running on the backend job. Robustification: a second guard beside #8342's, for modules reached through pytest.importorskip at any scope, with its own reason stated rather than folded into the collection guard. Verified against main's version of this file: 1 test module(s) reach a backend module that needs unsloth through pytest.importorskip without installing the stub ... ['test_safetensors_reasoning_stream.py'] It is pinned in both directions (an unstubbed sample is an offence, a stubbed one and an importorskip of numpy are not) so it cannot pass by matching nothing. I also swept the other five files that importorskip a core.* or unsloth* module without stubbing. Under the no-unsloth_zoo condition all five pass, so this file was the only live case: test_active_generations 84 passed test_bypass_permissions 117 passed test_diffusion_dataset_clips 22 passed test_mlx_inference_backend 80 passed, 10 skipped test_offline_gguf_cache_fallback 202 passed test_safetensors_reasoning_stream 3 FAILED, 10 passed Verified by hiding unsloth_zoo's metadata to reproduce the CI shape: before: 3 failed, 10 passed after: 13 passed * Drop the stubs once the module is built, so they do not outlive it The first version of this fix installed the unsloth stub and left it in sys.modules for the rest of the process. Staging CI caught it on all four interpreters: tests/test_audio_type_inconclusive.py:159: in test_the_stubs_do_not_outlive_this_module assert name not in sys.modules, name E AssertionError: unsloth That test exists precisely to forbid what I did. Every other file installs its stubs, performs its heavy import, then pops them, and a stub left behind is a cross-file leak: every other _stub_if_missing returns early when the name is already present, so its own bookkeeping never runs and its cleanup has nothing to undo. Traded this file's order dependency for a worse one, in other words. Now it follows the same shape as test_audio_type_inconclusive.py: stub, build core.inference.inference while the stubs are live, drop the stubs. The three tests reach the module through pytest.importorskip and get it from sys.modules, so the stubs only need to exist for that one import. It did not reproduce locally, which is why it reached staging: run the file alone and the victim's own _STUBBED list is empty, so its loop asserts nothing and passes vacuously. It takes xdist interleaving the two files on one worker to put the leak in front of that assertion. * Close two holes in the importorskip guard Both reported on the guard added by this PR, both confirmed by construction against the guard's own helpers. 1. The bare-name form was invisible. "from pytest import importorskip" then a bare importorskip("core.inference.inference") is a call on an ast.Name, not an attribute, so matching only pytest.importorskip returned no targets: bare-name form detected as target: [] Both call shapes are matched now. 2. A module-scope call was judged by the wrong boundary. The check scanned the whole module for a stub, so a file that calls importorskip at module scope and installs its stub BELOW that line read as safe: late-stub form judged stubbed (scanning whole module): True while the call runs during collection and the import has already raised. The boundary is now the call's own line at module scope, and the end of the module only inside a def, where the call really does run after the body. The pinning test covers all four shapes and both boundaries, so neither hole can reopen silently. The guard still names the file this PR fixes when run against main's version of it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Judge import-time calls and dropped stubs correctly in the guard Two more gaps on the guard this PR adds, both verified against its own helpers before changing anything. 1. Definition-time calls were given the wrong boundary. _runtime_nodes stops at every def and class, so a call in a class body, a decorator, an annotation or a default expression was treated as lazy and got the end-of-module boundary: class body, stub below calls=[('core.inference.inference', 5)] judged_safe=True default arg, stub below calls=[('core.inference.inference', 5)] judged_safe=True All of those run while the module is being imported. Only a function BODY is deferred, so the scope walk now keeps class bodies and definition-time expressions and excludes just the body. No file in this tree does this today; all five class-level importorskip calls are inside methods, which really are lazy. Fixed anyway, because a guard that is wrong about when Python runs things is not much of a guard. 2. A lazy call was judged only on whether a stub was ever installed, not on whether it was still there. A module that installs the stubs, drops them again, and then reaches the heavy module lazily passes that check and still raises at test time. This PR's own file is safe only because it imports the target eagerly while the stubs are live, and nothing made that a requirement, so a copy that omitted the eager import would have read as safe. Now: a lazy call is an offence when the module drops its stubs and does not import the target itself. A module that keeps its stubs installed is fine lazily, which is why the removal is part of the condition rather than the eager import alone. The decision is one function used by both the guard and the test that pins it. My first version of this test reimplemented the logic instead, and a mutation that deleted the real rule left it green. scope walk narrowed back to _runtime_nodes -> 1 failed eager-import rule deleted -> 1 failed stub-removal condition dropped -> 1 failed * Judge a dropped stub at import time too, not only for lazy calls Reported on this PR and correct. The guard asked "were the stubs dropped again, and was the target never imported eagerly" only in the lazy branch, so a module that installs the stub, pops it, and then calls importorskip at module scope satisfied the install check and read as safe, while collection still dies: the stub is gone by the time the call runs and nothing put the target in sys.modules. Both questions now take the same boundary as the install check. _drops_stubs and _eagerly_imports stop at that line, so an import-time call is judged on the pops and imports ABOVE it and a lazy call on the whole module body, which is what the old lazy-only branch computed. Three cases pinned: stub-pop-call is an offence, stub-call-pop is not, and stub-import-pop-call is not. Mutating the branch back to the lazy-only form fails the new pin. 6 passed. * Four precision fixes to the importorskip guard All four reported on this PR, all four real, each pinned and each verified by reverting the fix and watching the pin fail. 1. An eager import only counts if the stubs were live for it. A file that probes the target in try/except ImportError before installing the stubs, then stubs, pops, and calls importorskip lazily, was read as safe. On the dependency-light matrix that probe fails and Python removes the half-initialised module, so the later call still reaches the real dependency. The check now asks _stubs_before for the statement's own line, so there is one answer to "are the stubs live" in this file rather than two that can drift. 2. A drop has to be OUR stubs being dropped. Any x.modules.pop counted, so an unrelated sys.modules.pop("routes.foo", None) in a properly stubbed file got that file reported as an offender, and a pop on any object with a .modules counted as well. The receiver must now be sys.modules and the statement must name the required stub the same way an install does, directly or through a module-level list it reads, which is how the real files spell it. 3. importorskip(modname = "...") is the documented signature (modname, minversion=None, reason=None, *, exc_type=None), and node.args is empty for it, so a file written that way walked straight past the guard. The keyword is read now. 4. exc_type=ImportError is pytest's own opt-in to skipping on a plain ImportError, which is the exact failure this guard is about, so such a call is safe unstubbed and flagging it was a false report. Those calls are excluded. exc_type=ModuleNotFoundError is the default and stays flagged. Confirmed against the pytest reference: exc_type arrived in 8.2, "must be ImportError or a subclass", and defaults to ModuleNotFoundError. 6 passed, and reverting each of the four in turn: 1 failed each time test_safetensors_reasoning_stream.py + the guard together: 19 passed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Install peft on the backend job, and stop misreading unrelated cleanup as a stub drop Two items from this round, both reported here. 1. The eager import in test_safetensors_reasoning_stream.py was failing on the backend matrix and the handler was swallowing it, so the three importorskip tests SKIPPED rather than ran: 10 passed and 3 skipped, green either way. core/inference/inference.py imports peft at module scope, and peft is listed only in extras-no-deps.txt and no-torch-runtime.txt, neither of which this job installs. Confirmed both ways: the install log for a real matrix leg lists torchao and transformers and no peft, and hiding peft behind a meta_path finder locally reproduces exactly 10 passed and 3 skipped. Fixed by installing it rather than stubbing it. A stub does not work here: transformers probes importlib.util.find_spec("peft") during its own import, which raises ValueError on a stub whose __spec__ is None, so stubbing turns a silent skip into a collection error. Installing it also means these tests exercise the real import chain, which is the coverage the file is for. The swallow now records the error and a new test reads it back, so a module-scope dependency added to inference.py that this job does not install fails by name instead of quietly removing three tests. 2. _drops_stubs accumulated every module-level assignment target, so an unrelated cleanup list popped from sys.modules read as the unsloth stubs being dropped and got properly stubbed files reported as offenders. It now follows the link: a module-scope call naming the required stub identifies the helper, and whatever module-level name that helper appends to is the record. Two cases pinned, and the pin for the real shape now spells out the helper body, because that is what the link is followed through. test_safetensors_reasoning_stream.py + the guard: 20 passed with peft hidden: the new test FAILS by name instead of 3 silent skips restoring the old accumulation: 1 failed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Install peft after the CPU torch wheel, not before it Reported on this PR and correct, and this was my mistake in the previous commit. peft declares `torch >=1.13.0` with no index constraint (checked against its wheel metadata), so placed above the CPU-index line it resolves torch from PyPI, which on Linux is the CUDA build. That version satisfies the >=2.4,<2.11 the next line asks for, so pip leaves it, and a nominally CPU-only job ends up carrying the CUDA distribution and its nvidia-* dependencies on all four legs: minutes of download against a limit this job was already cancelling at, and runner disk. Moved below the torch and transformers lines, where the pinned CPU torch already satisfies the dependency and nothing re-resolves. * Judge eager imports and helper calls by what actually runs at import Three items from this round, all reported here, all real. 1. An eager import under `if TYPE_CHECKING:` or `if False:` never runs, so it is not what left the target in sys.modules, and a file that stubs, "imports" there, then drops the stubs still raises. The walk pruned nothing and read it as safe. It now skips a branch whose test is a constant the interpreter will not take, or TYPE_CHECKING, and nothing beyond that is guessed at. 2. The opposite error in the same walk: a class body, a decorator, a default and an annotation all execute while the module is being imported, so an eager import written in one of them DOES cache the target. _runtime_nodes stopped at every def and class, so such a file was reported as an offender when it is safe. That is what _import_time_nodes was already built for, so the eager check uses it now, with the reachability pruning above on top. 3. A def is only deferred while nothing runs it. Where the MODULE BODY calls a helper, that helper executes during collection, so an importorskip inside it runs then too, and the end-of-module boundary let a stub installed BELOW the call site read as being in place. Those calls now take the line they are invoked from. Followed one level, which is the shape that occurs; a helper reached only through another helper keeps the deferred boundary rather than being guessed at. Four cases pinned: TYPE_CHECKING import is an offence, a class-body import is not, a helper called at import time takes the call site's line, and the same helper never called keeps the end-of-module boundary. Reverting each of the three fixes fails a pin. 6 passed, and 20 passed with test_safetensors_reasoning_stream.py * Resolve importorskip aliases, and prune the else of a constant-true test Both reported on this PR, both real, both pinned and mutation-checked. 1. `from pytest import importorskip as ios` then `ios(...)` is a valid call, and matching the callee against the literal string missed it, so an unstubbed module written that way walked past the guard. Bare names bound to it are collected now, through the import and through a plain rebinding of the attribute form. `from mymod import importorskip` is not one of them and is pinned as such. 2. The other half of the unreachable-branch hole I closed last round: pruning the body of `if False:` while still descending into the `else:` of `if True:` left an import that never runs counting as the one that cached the target. The traversal now takes only the branch the interpreter takes, for either constant. Both directions pinned. 6 passed, and reverting each fix fails its pin * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow import-time helper calls all the way down Reported on this PR and correct, and my "conservative" one-level stop was conservative in the wrong direction. Stopping hands the inner helper the end-of-module boundary, which is the LENIENT answer: a stub installed anywhere in the file then reads as in time, while collection has already run the inner import and failed. Reachability now propagates through the module-level call graph to a fixed point, and a helper reached only through another helper inherits the outer call site's line, which is when it actually runs. Two cases pinned: the module calls the outer helper and the inner one holds the importorskip, which is an offence at the outer call's line; and the same chain with nothing calling it at import time, which keeps the deferred boundary. Removing the closure fails the first. 6 passed * Propagate only through the calls the helper actually makes Reported on this PR and correct, and this one fails a SAFE file rather than passing an unsafe one. ast.walk visits a call under `if False:` and a call inside a nested def that nothing invokes, and handing those the outer helper's import-time boundary judged an importorskip as running before a later stub when Python never executes that path during collection. It now walks the helper's body under the same reachability rule the module body uses, so the two cannot answer differently. Two cases pinned, both safe: an inner call under a constant-false test, and one inside a nested def that is returned rather than called. Reverting to ast.walk fails them. 6 passed * Do not report a call the interpreter never reaches Reported on this PR and correct, and it is the same false-positive direction as the last one: an importorskip under `if False:` or `if TYPE_CHECKING:` was collected as a module-scope call, so the guard failed a file that only type-checks the import or deliberately disables it. The helper seeding had the same gap for a helper invoked only inside such a branch. Both traversals are reachability-aware now. The call walk prunes constant branches wherever they appear, inside a function body as well as at module scope, since a call under `if False:` never runs there either. Three cases pinned, all safe: a TYPE_CHECKING call, an `if False:` call, and a helper called only from a disabled branch. Reverting either traversal fails them. 6 passed, and 20 passed with test_safetensors_reasoning_stream.py * Attribute a call to the innermost function that holds it Reported on this PR and correct, and the same false-positive direction as the last two. ast.walk descends into a nested def, so an importorskip written there was attributed to the enclosing module-level function and handed that function's import-time boundary. A nested body cannot run until something calls it, by which time a stub installed below the outer call is in place, so the guard rejected a file that works. Nested bodies are deferred now, and a call inside one falls through to the end-of-module boundary, which is what deferred means. Pinned: an importorskip inside a nested def that the outer helper returns rather than calls, with the stub after, is safe. Restoring ast.walk fails it. 6 passed, and 20 passed with test_safetensors_reasoning_stream.py * Stub the heavy imports in the peft-gated TTS test Reported on this PR, real, and a consequence of installing peft in the previous commit. test_audio_tts_cancellation.py gates on pytest.importorskip("peft") and then imports core.inference.inference in the test body without stubbing anything. That gate used to skip, so the import never ran. With peft installed it opens, and the import works only because collecting test_safetensors_reasoning_stream.py has already cached that module: module-level code runs during COLLECTION, before any test body, so the whole suite hides it. Running the file on its own does not. That is the same order dependence this PR exists to remove, reintroduced by the fix, which makes it worth more than the P2 it came in as. The file now stubs unsloth and trl, imports core.inference.inference while they are live, and drops them again, exactly as its siblings do. The peft gate stays, so a machine without peft still skips rather than failing. Verified: with peft hidden the file skips cleanly on its own (17 passed, 1 skipped), and 59 passed across it plus test_audio_type_inconclusive.py, test_safetensors_reasoning_stream.py and the guard. Locally the eager import is blocked only by a torch/torchao ScalingType mismatch inside peft, which is present on main too and has nothing to do with unsloth; with the stubs installed that is the sole remaining blocker, checked directly. Worth flagging separately: seven more files are in the same class, importing a heavy backend module lazily while never stubbing -- test_chat_eos_template_refresh.py, test_chat_template_continuation.py, test_control_markup_neutralize_7066.py, test_generation_timing.py, test_nudge_tool_calls_wiring.py, test_online_tokenization_wiring.py and test_training_preflight.py. All are masked by the same collection-time caching. They are latent rather than failing, and fixing eight files belongs in its own change rather than inflating this one. * Widen the stub guard to module-scope with/if blocks and helper-held stubs _first_heavy_import_line only read the direct children of the module body, so a heavy import inside a module-scope with or if was invisible to the guard: a file written as 'with something(): from core.training.trainer import X' without stubs would take collection down unseen. It now uses the import-time traversal, with try/except ImportError exempt since that is a deliberate guard. That widening needs the matching read on the other side. Two files hold their stubs with 'with _stubbed():' and import the heavy module inside that block, so the installing code is one call away from module scope and the statement spells neither the module name nor sys.modules. _stubs_before now reads through a call to a module-level helper, the same way the stub-record check already does, and keeps asking what the helper DOES rather than merely what it names. Pins added for both halves plus the helper that names the module without installing anything; each is mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Take the backend CI peft version from the requirements pin An unconstrained 'pip install peft' takes the newest release (0.20.0 on the last staging run), while extras-no-deps.txt pins peft==0.18.1 deliberately, because 0.19.0 breaks Unsloth's export subprocess. That left this job exercising the newly enabled inference tests against a peft that production never ships and no other job installs. The version is read out of extras-no-deps.txt rather than written here again, so a change to the pin cannot leave this workflow behind. peft 0.18.1 declares torch>=1.13.0, transformers unbounded and python>=3.10, so it holds across the 3.10 to 3.13 matrix and does not disturb the CPU torch installed above it. * Make both sides of the stub guard reachability and order aware Two follow-ons to widening the search into module-scope compound statements. The widened search used the plain import-time traversal, which descends into both branches of every conditional, so an import under if TYPE_CHECKING: or if False: read as an import-time dependency. A file with a legitimate type-only import would have failed this guard until someone added a stub it does not need. It uses the reachability-aware traversal now. And widening one side without narrowing the other read the whole enclosing statement as preceding the import, so if True: import core.inference.inference _stub_if_missing("unsloth", ()) counted as stubbed while Python attempts that import first and collection dies. Only the part of a statement that runs before the import line is read now. The stub side gets the same reachability treatment, from the other direction: a stub installed under if False: installs nothing, so counting it would report a file safe that still raises. Three pins added, each mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow the import's own branch, and match except clauses by what they catch Line order alone merges branches that exclude each other: if enabled: _stub_if_missing("unsloth", ()) else: import core.inference.inference puts the stub above the import while the two can never both run, and the guard called that file stubbed. What is walked now is the import's own chain: at each level only the block CONTAINING the import is descended into, and only the statements above it there. Those did run, because the import running means its branch was taken. A stub that merely might have run does not count either. That last rule needs one exemption, and the tree already had the case: test_training_progress_callback.py installs its stubs under 'if not _TRAINER_PRE_IMPORTED', where the flag is '"core.training.trainer" in sys.modules'. Skipping the stubs on the other branch is not an omission, because the import resolves out of sys.modules there and never reaches the real dependency. A test that asks sys.modules whether the target is already imported keeps its branch, read through a module-level flag as well as directly, since that is how it is written. Separately, the ImportError exemption matched substrings, so 'except MyImportError:' and 'except ExceptionGroup:' both exempted a try that catches neither. Handler types are compared as whole names now, including tuple members and the last component of a dotted name. ModuleNotFoundError is deliberately not in the set: it is a subclass, so it catches strictly less. Ten pins added across the four behaviours, each mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Take the loaded-models indicator suite off the UI job's critical path (#9060) * Take the indicator suite off the UI job's critical path Unsloth UI CI / Chat UI Tests has been timing out at its 30 minute limit on almost every open PR: 5 of the 6 currently open, at 30m09s to 30m24s. Nothing was failing. The job was simply longer than the limit, and a limit is a guillotine, so the steps after the cut never ran at all. Where it goes: "Cross-browser loaded-models indicator" is 853s of the 30 minutes, three browser engines at about 4.7 minutes each, each booting its own Unsloth. Everything downstream of it -- the other-engines update banner regression, both image staged-download regressions, the model-picker per-model-config suite and the IME / multilingual paste regression -- was reported "skipped" and has been running on no PR at all. It is now its own job rather than a step, so it runs beside the rest instead of inside their budget. That leaves ui-smoke around 16 minutes and the new job around 15, both with real margin, and the five skipped suites run again. The new job carries no HF cache and no GGUF: the suite stubs the four /status reads with page.route, so it needs no model, no GPU and no llama.cpp build. Moving the work rather than the limit is deliberate. The limit here already went 25 -> 30 for this exact symptom, with the comment on it still describing that round, so raising it again buys one more release of silence. Also tests/studio/test_playwright_suites_run_in_ci.py, because these drivers are standalone scripts that nothing collects: a suite runs only because a workflow step or a .github/scripts helper names it, and deleting that line leaves every job green. It fails if any tests/studio/playwright_*.py is named by no workflow and no CI script. Deleting either the new step or the driver call inside run-studio-indicator-browser.sh fails it. It also found two suites already in that state, exempted with a reason each rather than quietly ignored: playwright_tauri_python_tool_images.py needs the Tauri desktop shell no runner here builds, and playwright_train_pickers.py needs a dataset and model resolved through huggingface_hub while the UI workflows boot API-only. Both are worth wiring up, separately from this. test_playwright_suites_run_in_ci.py: 3 passed, and 2 failed under each mutation test_stt_model_search_locator_contract.py + test_composer_rtl_bidi_attribute.py: 34 passed * Count only helper scripts a workflow can actually reach Reported on this PR and correct: reading every file under .github/scripts counts an orphaned helper as coverage, so a driver named only by a script nothing calls reads as covered while it runs nowhere, which is the regression this test exists to catch. The workflows now seed the text and a helper joins only once something already in it names the helper, repeated to a fixed point because one helper can call another. .github/actions is included the same way, since a workflow reaches a composite action by uses: and the action then calls the script. Verified by making the reference unreachable in all three UI workflows while leaving run-studio-indicator-browser.sh in the tree. The new version reports the driver as running nowhere; the scan-everything version reports it covered. That one helper is named by the Linux, Windows and Mac UI workflows, which is why removing it from one is correctly still covered. 3 passed * Count only the workflow fields that run something Reported on this PR and correct. Reading a workflow whole counts a driver named in on.pull_request.paths as an invocation, and studio-frontend-ci.yml names playwright_strip_ansi_smoke.py in both its trigger list and its step, so deleting the step alone left this guard green while the suite ran nowhere. Trigger paths say when CI runs, not what it runs. The scan now parses each workflow and reads only the executable fields: a job's uses, and each step's run, uses and with. Helper reachability is followed from that text as before. Verified by deleting only the run line in studio-frontend-ci.yml and leaving the trigger path: the guard now reports playwright_strip_ansi_smoke.py as running nowhere, where before it stayed green. 3 passed * Follow a composite action by the directory workflows reference it with Reported on this PR and correct. A composite action is used as `./.github/actions/<name>`, never as the action.yml inside it, so the reachability walk never opened one and a driver launched from inside an action read as an orphan. An action.yml now also matches on its containing directory. Pinned with a line from inside install-unsloth-local's own body rather than something a workflow could also contain, so the pin fails if the walk matches the uses: reference without opening the action. Dropping the directory match fails it. 3 passed * Run the coverage guard from the workflow that has no paths filter Reported on this PR and correct: the guard could not catch the regression it exists for. It ran only in studio-backend-ci.yml's Repo tests job, whose paths do not include .github/workflows/**, .github/scripts/** or .github/actions/**, so a PR that only deletes a Playwright invocation never collected it and stayed green until the post-merge run on main. It now runs in workflow-trigger-lint.yml instead, which carries no paths filter at all and says so in its own header comment, deliberately, for the same reason: a gate that only runs for some PRs does not gate workflow changes. Cost is the same shape as the lint beside it, filesystem reads in seconds, and that job already installs PyYAML, which is all this needs besides pytest. 3 passed, run bare from the repo root as that job runs it the workflow-trigger lint itself still passes: 42 workflow files scanned * Pin the three browser engines to the job that runs them Reported on this PR and correct: the repo-wide check cannot see this job disappear. run-studio-indicator-browser.sh is named by the Mac and Windows UI workflows too, so deleting all three calls from the Linux workflow leaves every guard green while the Chromium/Firefox/WebKit coverage this job exists for is gone. Asserted against the ui-indicator job itself rather than the file, so a step moved back into ui-smoke fails it too: that is the 30-minute limit this change moved the work out of. 4 passed, and deleting the three calls fails the new test by naming all three * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Count a driver as covered only where CI actually runs it Reported on this PR and correct, with a real instance: substring presence is not coverage. .github/scripts/kaggle_studio_ci/report.py names playwright_chat_ui.py inside a result description, and the workflows name drivers in comments and in trigger paths, so deleting every real invocation could leave this guard green on prose alone. Every driver and helper in this repo is run the same way, as an argument to an interpreter, so that is what is matched now: the name at the end of a path token being handed to python, python3, node, bash or sh. Composite actions keep the directory match, since a workflow reaches one through uses: rather than a command. Verified by removing every real invocation of playwright_chat_ui.py while leaving report.py's description in place. The substring is still present and the guard now reports the driver as running nowhere; before, it stayed green. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop a commented-out invocation counting as coverage Reported on this PR and correct: commenting a command out is how one gets disabled, and the scan reads workflow run: bodies and helper scripts verbatim, so `# python tests/studio/x.py` matched the invocation shape through the whitespace before python. Comments are stripped before matching. Shell, YAML and Python all take # to end of line, and a # inside a string only ever appears in prose here, which is not an invocation either way. Verified by commenting out every real invocation of playwright_chat_ui.py: the guard now reports it as running nowhere, where before it stayed green. 4 passed * Tighten the comments in the coverage guard Comments and docstrings only. The code is byte-identical once docstrings are stripped, checked by comparing the parsed trees, and the tests still pass. * Read the browser-engine guard through the comment stripper The cross-engine check tested raw step bodies for the substring, so prefixing all three invocations with # left it green while the job ran no browser suite, which is the regression it exists to prevent. It uses the same _uncommented() helper the repo-wide scan does, and a pin asserts the disabled form of one of those lines does not read as coverage. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Keep the killed-by-signal exit status when release() fails (#9072) * Keep the killed-by-signal exit status when release() fails A cancelled launcher reported success. The signal handler deletes the kernels and then re-raises the signal so the status still reads killed, but the delete was outside any try: a raise there propagated out of the handler into whatever the main thread was doing, main()'s except BaseException caught it, and finish() called release() a second time. When that second call worked, which is what a transient OSError out of a subprocess spawn on a loaded runner looks like, main RETURNED 0 and the job read as completed. Caught on a contended runner rather than by inspection, and reproduced by failing the first delete only: returncode 0 instead of -SIGTERM. The delete is now best effort, retried once, and the death by signal happens either way. If both attempts fail the slugs stay in the registry and the next launcher's orphan sweep reclaims them, which is the same path a kill -9 already takes. The subprocess waits in the suite move from 30s to a named budget of 120s. They guard against a launcher that never dies, not against latency, and the repo suite now runs four of these at once on a four-core runner. * Make the stall outlast the death budget, and assert the signal did the killing Raising the wait to 120s put it past the 60s stall the launcher sits in, which defangs the tests it was meant to make robust: a handler that swallows its signal leaves the process asleep and then resuming, so it wakes, runs finish(), deletes the kernels through the ordinary path and exits INSIDE the wait. The deletion assertions are satisfied by that, so those tests would have passed on a launcher that ignored the signal entirely. The stall is now a named 900s against the named 120s budget, and a test asserts the relationship rather than leaving it to whoever next tunes one of the two numbers. The deletion test also asserts the exit status now. Without it, the deletion is satisfied by finish() doing its usual work, so nothing there said the signal was what caused it. Verified by making the handler return instead of re-raising: with the stall at 900s that fails all four signal tests, and with it at 60s the relationship test fails as well. * Tighten the comments added here * Read a stub helper only as far as it has run Reading the whole helper body accepted a sys.modules write below the yield of a context manager. That code runs on the way OUT of the with block, after the import inside it has already been attempted, so it stubs nothing for that import and the file still dies at collection while the guard stays green. The scan now stops at the helper's first yield, and drops unreachable and merely optional branches the way the module-scope side already does. The yield has to be the helper's own: one belonging to a generator defined inside it says nothing about when the helper suspends, and taking it cut the scan off above the install the real files do perform. Three pins, each mutation-checked. * Count only the finally of a try as certain A try body whose handler swallows the exception is exactly a block that may stop part way: an optional import fails, the handler catches it, the stub call below it never runs, and the heavy import after the try does. Counting the whole body reported that file stubbed. Only the finally counts now, which runs on both paths. The else and the handlers are conditional by construction and were already excluded. No test module in the tree installs its stubs inside a try body, so nothing legitimate changes. Two pins, both mutation-checked. * Read the polarity of the already-imported guard, and reach nested helpers Three holes, all about when code runs rather than what it says. The already-imported exemption accepted any branch whose test mentioned sys.modules, either way up. Written as 'if _PRE:' the stubs are installed only when they are not needed and skipped when they are, and the import then fails at collection with the guard green. The polarity is read now, through a negation and through a module-level flag, and only the branch that runs when the module is ABSENT counts, since that is the one whose stubs the import needs. A while-else was treated as certain. It is skipped when the loop leaves through break, so it is not. And nested defs were left out of the function map entirely. Deferring a nested body is right while nothing calls it, but an outer helper the module body calls, which defines and calls an inner one, runs that inner body during collection. The call inside it was handed the end-of-module boundary, the lenient answer, so a stub installed after the outer call read as being in time while the inner import had already run. Nested defs are in the map now and inherit the boundary of the call that reaches them; one nothing calls at import is still deferred. Seven pins across the three, each mutation-checked. * Judge handlers, helper exits and stub calls by what they do Three more exemptions that were granted on shape rather than effect. A handler naming ImportError does not necessarily absorb it: 'except ImportError: raise', and one raising a replacement, both take collection down while the try body read as guarded, which hid the import from the guard entirely. Any raise the handler can reach disqualifies it now. A module-level pytest.skip is a call rather than a raise, so the skip idiom these files use stays exempt. A helper that can return before installing has not installed anything, so the scan stops at an exit rather than reading on to the call below it. Two exits are benign and both are already in the tree: a return under 'name in sys.modules', and one under the importlib probe inside an absorbing try. On each of those paths the module is AVAILABLE, so skipping the stub is the point of the branch. Without that distinction five correct files became offenders, which is how the shape of the exemption was found. And a call was treated as installing a stub because its name contained 'stub', which counted _remove_stub and _validate_stub. Where the module defines the callee, what it does decides. The name stays as the fallback for a helper imported from elsewhere, since a name defined nowhere would raise NameError at import and never reach collection. Nine pins, four mutation-checked behaviours. * Honour handler dispatch order, and stop at deferred bodies and dead code Python dispatches an exception to the FIRST handler that matches it, so 'except Exception: raise' followed by 'except ImportError: pass' still propagates. Asking whether ANY handler absorbs found the second one and exempted a try that does not guard the import. Only the first handler that would catch it is asked now. The re-raise check also descended into a def the handler merely DEFINES. That body does not run while the exception is being handled, so a properly guarded file was reported as an offender. The traversal stops at deferred bodies now. And the call-graph closure walked past an unconditional exit, handing a helper named below one the outer call's import-time boundary although it never ran, which rejected a stub installed below that call. It stops at such an exit now, checked AFTER the statement since 'return _inner()' runs its own expression, and only for an unconditional one: the path that skips a conditional exit still reaches the calls below it. Five pins, three mutation-checked behaviours. * Keep every definition of a helper name, and stop reachability at dead code Two enclosing functions can each define a helper of the same name. Keeping only the first dropped the second from the call graph, so its importorskip took the end-of-module boundary and a stub installed after the enclosing call read as being in time. All definitions of a name are kept now. Which one a given call names cannot be told apart here, so all of them take the boundary, which is the strict answer. And the general reachability walk did not stop at an unconditional exit, so an importorskip written after a bare return or raise was reported as an unstubbed offence. Python cannot execute it, so that is a file failing CI over dead code. The walk now ends a statement list at a return, raise, break or continue, which the import-time traversal beside it already did. Four pins, both mutation-checked. * Judge every heavy import, not just the first Separate branches can each import a heavy module, and only one of them needs to be stubbed for the first to look fine. A file that stubs before the import in the if and forgets the one in the else dies at collection whenever the else runs, while reducing the module to a single line reported it safe. Every import-time heavy import is collected now and each is judged against the stubs that precede it on its own path. Any single unstubbed one is an offence, whatever the others do. Two pins, mutation-checked. * Require the availability guard to name the module, and drop async installers The already-imported exemption fired on any sys.modules membership test, so 'if "pytest" not in sys.modules:' decided whether the stubs under it counted. An unrelated cached package says nothing about unsloth, and the branch is skipped exactly when that package is present. The key now has to be the required stub or a heavy backend module. A key this file cannot read gets that latitude only inside a stub helper deciding about its own argument, which is the idiom _stub_if_missing opens with, and never at module scope. And calling an async def builds a coroutine and runs none of its body, so reading through such a call credited the module with stubs it never installed. Only synchronous helpers are read through now. Six pins, three mutation-checked behaviours. * Drop the review bookkeeping from the comments Thirty-three comments ended in 'Reported on this PR', which says nothing to anyone reading the file afterwards: once merged there is no this PR. The reasoning each comment records is what matters and is kept. * Name the file rather than the pull request --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 02:44:55 -07:00
Daniel Han	5307992416	Keep the killed-by-signal exit status when release() fails (#9072) ... * Keep the killed-by-signal exit status when release() fails A cancelled launcher reported success. The signal handler deletes the kernels and then re-raises the signal so the status still reads killed, but the delete was outside any try: a raise there propagated out of the handler into whatever the main thread was doing, main()'s except BaseException caught it, and finish() called release() a second time. When that second call worked, which is what a transient OSError out of a subprocess spawn on a loaded runner looks like, main RETURNED 0 and the job read as completed. Caught on a contended runner rather than by inspection, and reproduced by failing the first delete only: returncode 0 instead of -SIGTERM. The delete is now best effort, retried once, and the death by signal happens either way. If both attempts fail the slugs stay in the registry and the next launcher's orphan sweep reclaims them, which is the same path a kill -9 already takes. The subprocess waits in the suite move from 30s to a named budget of 120s. They guard against a launcher that never dies, not against latency, and the repo suite now runs four of these at once on a four-core runner. * Make the stall outlast the death budget, and assert the signal did the killing Raising the wait to 120s put it past the 60s stall the launcher sits in, which defangs the tests it was meant to make robust: a handler that swallows its signal leaves the process asleep and then resuming, so it wakes, runs finish(), deletes the kernels through the ordinary path and exits INSIDE the wait. The deletion assertions are satisfied by that, so those tests would have passed on a launcher that ignored the signal entirely. The stall is now a named 900s against the named 120s budget, and a test asserts the relationship rather than leaving it to whoever next tunes one of the two numbers. The deletion test also asserts the exit status now. Without it, the deletion is satisfied by finish() doing its usual work, so nothing there said the signal was what caused it. Verified by making the handler return instead of re-raising: with the stall at 900s that fails all four signal tests, and with it at 60s the relationship test fails as well. * Tighten the comments added here	2026-08-17 01:00:58 -07:00
Daniel Han	d0e828feff	Take the loaded-models indicator suite off the UI job's critical path (#9060) ... * Take the indicator suite off the UI job's critical path Unsloth UI CI / Chat UI Tests has been timing out at its 30 minute limit on almost every open PR: 5 of the 6 currently open, at 30m09s to 30m24s. Nothing was failing. The job was simply longer than the limit, and a limit is a guillotine, so the steps after the cut never ran at all. Where it goes: "Cross-browser loaded-models indicator" is 853s of the 30 minutes, three browser engines at about 4.7 minutes each, each booting its own Unsloth. Everything downstream of it -- the other-engines update banner regression, both image staged-download regressions, the model-picker per-model-config suite and the IME / multilingual paste regression -- was reported "skipped" and has been running on no PR at all. It is now its own job rather than a step, so it runs beside the rest instead of inside their budget. That leaves ui-smoke around 16 minutes and the new job around 15, both with real margin, and the five skipped suites run again. The new job carries no HF cache and no GGUF: the suite stubs the four /status reads with page.route, so it needs no model, no GPU and no llama.cpp build. Moving the work rather than the limit is deliberate. The limit here already went 25 -> 30 for this exact symptom, with the comment on it still describing that round, so raising it again buys one more release of silence. Also tests/studio/test_playwright_suites_run_in_ci.py, because these drivers are standalone scripts that nothing collects: a suite runs only because a workflow step or a .github/scripts helper names it, and deleting that line leaves every job green. It fails if any tests/studio/playwright_*.py is named by no workflow and no CI script. Deleting either the new step or the driver call inside run-studio-indicator-browser.sh fails it. It also found two suites already in that state, exempted with a reason each rather than quietly ignored: playwright_tauri_python_tool_images.py needs the Tauri desktop shell no runner here builds, and playwright_train_pickers.py needs a dataset and model resolved through huggingface_hub while the UI workflows boot API-only. Both are worth wiring up, separately from this. test_playwright_suites_run_in_ci.py: 3 passed, and 2 failed under each mutation test_stt_model_search_locator_contract.py + test_composer_rtl_bidi_attribute.py: 34 passed * Count only helper scripts a workflow can actually reach Reported on this PR and correct: reading every file under .github/scripts counts an orphaned helper as coverage, so a driver named only by a script nothing calls reads as covered while it runs nowhere, which is the regression this test exists to catch. The workflows now seed the text and a helper joins only once something already in it names the helper, repeated to a fixed point because one helper can call another. .github/actions is included the same way, since a workflow reaches a composite action by uses: and the action then calls the script. Verified by making the reference unreachable in all three UI workflows while leaving run-studio-indicator-browser.sh in the tree. The new version reports the driver as running nowhere; the scan-everything version reports it covered. That one helper is named by the Linux, Windows and Mac UI workflows, which is why removing it from one is correctly still covered. 3 passed * Count only the workflow fields that run something Reported on this PR and correct. Reading a workflow whole counts a driver named in on.pull_request.paths as an invocation, and studio-frontend-ci.yml names playwright_strip_ansi_smoke.py in both its trigger list and its step, so deleting the step alone left this guard green while the suite ran nowhere. Trigger paths say when CI runs, not what it runs. The scan now parses each workflow and reads only the executable fields: a job's uses, and each step's run, uses and with. Helper reachability is followed from that text as before. Verified by deleting only the run line in studio-frontend-ci.yml and leaving the trigger path: the guard now reports playwright_strip_ansi_smoke.py as running nowhere, where before it stayed green. 3 passed * Follow a composite action by the directory workflows reference it with Reported on this PR and correct. A composite action is used as `./.github/actions/<name>`, never as the action.yml inside it, so the reachability walk never opened one and a driver launched from inside an action read as an orphan. An action.yml now also matches on its containing directory. Pinned with a line from inside install-unsloth-local's own body rather than something a workflow could also contain, so the pin fails if the walk matches the uses: reference without opening the action. Dropping the directory match fails it. 3 passed * Run the coverage guard from the workflow that has no paths filter Reported on this PR and correct: the guard could not catch the regression it exists for. It ran only in studio-backend-ci.yml's Repo tests job, whose paths do not include .github/workflows/**, .github/scripts/** or .github/actions/**, so a PR that only deletes a Playwright invocation never collected it and stayed green until the post-merge run on main. It now runs in workflow-trigger-lint.yml instead, which carries no paths filter at all and says so in its own header comment, deliberately, for the same reason: a gate that only runs for some PRs does not gate workflow changes. Cost is the same shape as the lint beside it, filesystem reads in seconds, and that job already installs PyYAML, which is all this needs besides pytest. 3 passed, run bare from the repo root as that job runs it the workflow-trigger lint itself still passes: 42 workflow files scanned * Pin the three browser engines to the job that runs them Reported on this PR and correct: the repo-wide check cannot see this job disappear. run-studio-indicator-browser.sh is named by the Mac and Windows UI workflows too, so deleting all three calls from the Linux workflow leaves every guard green while the Chromium/Firefox/WebKit coverage this job exists for is gone. Asserted against the ui-indicator job itself rather than the file, so a step moved back into ui-smoke fails it too: that is the 30-minute limit this change moved the work out of. 4 passed, and deleting the three calls fails the new test by naming all three * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Count a driver as covered only where CI actually runs it Reported on this PR and correct, with a real instance: substring presence is not coverage. .github/scripts/kaggle_studio_ci/report.py names playwright_chat_ui.py inside a result description, and the workflows name drivers in comments and in trigger paths, so deleting every real invocation could leave this guard green on prose alone. Every driver and helper in this repo is run the same way, as an argument to an interpreter, so that is what is matched now: the name at the end of a path token being handed to python, python3, node, bash or sh. Composite actions keep the directory match, since a workflow reaches one through uses: rather than a command. Verified by removing every real invocation of playwright_chat_ui.py while leaving report.py's description in place. The substring is still present and the guard now reports the driver as running nowhere; before, it stayed green. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop a commented-out invocation counting as coverage Reported on this PR and correct: commenting a command out is how one gets disabled, and the scan reads workflow run: bodies and helper scripts verbatim, so `# python tests/studio/x.py` matched the invocation shape through the whitespace before python. Comments are stripped before matching. Shell, YAML and Python all take # to end of line, and a # inside a string only ever appears in prose here, which is not an invocation either way. Verified by commenting out every real invocation of playwright_chat_ui.py: the guard now reports it as running nowhere, where before it stayed green. 4 passed * Tighten the comments in the coverage guard Comments and docstrings only. The code is byte-identical once docstrings are stripped, checked by comparing the parsed trees, and the tests still pass. * Read the browser-engine guard through the comment stripper The cross-engine check tested raw step bodies for the substring, so prefixing all three invocations with # left it green while the job ran no browser suite, which is the regression it exists to prevent. It uses the same _uncommented() helper the repo-wide scan does, and a pin asserts the disabled form of one of those lines does not read as coverage. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 00:50:28 -07:00
Daniel Han	6c124b6dd8	Run the repo CPU test suite on all four runner cores (#9019) ... * Fix a 500 on dictation Unload before any backend is resident stt_unload passes expected_model positionally: _, unload_stt = _stt_lifecycle() failed = await asyncio.to_thread(unload_stt, engines, model) _stt_lifecycle() returns the orchestrator's unload_stt_model when a backend is resident and stt_registry.unload when one is not. Only the first takes expected_model positionally; on the registry it sits behind a `*`: def unload(engines = None, *, wait = True, expected_model = None) def unload_stt_model(self, engines = None, expected_model = None) So with nothing loaded yet, which is what a fresh process is, Unload raises TypeError and the route answers 500. Reproduced outside pytest: peek_inference_backend() fresh process: None stt_unload -> TypeError: unload() takes from 0 to 1 positional arguments but 2 were given Pass it by keyword, which both callables accept. Found while profiling CI, not by the tests, and the tests are why: this file's two unload tests passed only because an earlier test in the full suite had left a backend resident. Standalone they failed, and in their own file they failed; they went green only inside the full serial run. That order dependence hid a live bug. The new test drives the no-backend path directly, so neither the signature nor the call site can drift back. before: 2 failed, 53 passed (file alone) after: 56 passed * Run the repo CPU test suite on all four runner cores Backend CI is the most expensive workflow in the repo at 126.7 runner-minutes per push. Install is cached at ~1.2 minutes, so almost all of it is pytest. ubuntu-latest has 4 vCPUs and this suite is CPU-bound and GPU-free, so it was running on one core of four. On a real runner the step goes 17.75 minutes to 7.67, and the failure set is unchanged: the same single pre-existing failure, serial and parallel, on the same tree in the same staging repo. Locally, on the dependency shape the workflow installs, 806.1s to 219.7s with an identical 2 failed / 8066 passed / 178 skipped / 40 subtests result. The three small pytest steps in this job stay serial: the hardware-spoof step exists because those files mutate hardware.py module globals, and all three already measure under 0.1 minutes. The four-interpreter matrix is deliberately left alone. It is the bigger prize at ~79 runner-minutes, but measured under -n 4 on a real runner it fails tests serial does not, in more than one way and not the same way twice, so it needs its order dependencies found and fixed first rather than a flag. * Pin the two unload signatures against the route's one call site The bug fixed in the previous commit exists because _stt_lifecycle returns two different callables and the route has a single call site: def unload(engines = None, *, wait = True, expected_model = None) # stt_registry def unload_stt_model(self, engines = None, expected_model = None) # orchestrator A call that suits one is a TypeError on the other, and which one runs depends on whether a backend happens to be resident, so the broken half only appears on a fresh process. That will recur the next time either signature is edited. This binds BOTH real signatures against the arguments the route actually passes. It does not demand they be identical, only that one call site can serve both. Checked against two mutations: registry grows a keyword-only param -> passes (benign, and it should) registry drops expected_model -> fails, "got an unexpected keyword argument 'expected_model'" Paired with the test beside it: that one covers the call site (route -> registry on a cold process), this one covers the two callees staying compatible. 57 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Give each node harness run its own script file Reported on this PR, and reproduced before changing anything. Every _run in test_chat_preset_builtin_invariants.py wrote the same TEMP/run.mts and then executed "node run.mts" from TEMP. Under pytest-xdist's default load distribution two of this file's nine tests can land on different workers, so one worker executes the script the other just wrote over the top of it. Pinned to four cores, running the file at -n 4: before: 5 failed / 6 failed / 5 failed / 5 failed / 6 failed (of 9) after: 9 passed, six runs out of six Serial is unchanged at 9 passed either way, which is why the full-suite runs that back this PR did not show it: at ~8000 tests across four workers these nine rarely collide, so it is a latent flake rather than a reliable failure. That is worse, not better, and enabling -n 4 is what would have started rolling the dice. A unique NAME rather than a per-call directory, which is what tests/studio/_node_harness.py::run_harness does for every other node harness here. Those scripts reach the frontend sources by a relative path counted from TEMP, so an extra directory level breaks every import with ERR_MODULE_NOT_FOUND (tried it: 9 failed). register.mjs and loader.mjs stay shared because their contents are fixed, so a concurrent rewrite writes identical bytes. Swept the other five fixed TEMP roots under tests/studio: all of them already allocate per call through tempfile.mkdtemp, directly or via run_harness. This file was the only one rolling its own. * Shorten the harness comment * Stop two more shared-state races the parallel repo job would expose Both reported on this PR, both real, both confirmed by reading the code. 1. tests/studio/test_chat_preset_builtin_invariants.py still rewrote the shared register.mjs and loader.mjs on every _run. Only run.mts became unique in the previous commit. write_text truncates before it writes, so a worker rewriting one of those while another worker's node process is importing it can hand that process an empty or partial module. The contents are constant, which is why I wrongly called a concurrent rewrite harmless: identical bytes still arrive after a truncation. They are now written through a temp file and os.replace, so every reader sees one whole version. 2. tests/python/test_no_torch_filtering.py::TestRealRequirementsFiltering had an autouse fixture that snapshotted the requirements directory and, at teardown, deleted every filtered file that had appeared since. Those files land in the REAL requirements directory, not tmp_path, so under xdist one test's teardown removes a file another worker is still reading. It now records the paths it creates and removes only those. test_chat_preset_builtin_invariants.py: 9 passed serial, 9 passed on four -n 4 runs test_no_torch_filtering.py: 65 passed serial, 65 passed on four -n 4 runs no filtered files left behind * Run the event-loop latency tests outside the parallel invocation Reported on this PR. tests/studio/load_freeze asserts upper bounds on real elapsed time -- a /health burst under 250 ms while a 600 ms blocking probe runs, and a 100-request burst under 350 ms -- and those bounds ARE the contract, so they cannot be loosened without the tests ceasing to test anything. A pytest worker descheduled by the other three inflates them. Measured before changing anything, under 3x CPU oversubscription (8 spinners pinned to the same 4 cores): 8 ms against the 250 ms bound, 38 ms against the 350 ms one, 3 runs, no failures. So the margin is wide on this box. It is not wide enough to leave alone: a runner core is several times slower, which puts the 350 ms bound within reach of an unlucky schedule. The directory is now ignored from the -n 4 run and runs in its own serial step, 20s against the ~10 minutes -n 4 saves on this job. The ignore and the step are two edits held together by nothing, and losing the step is silent -- the job stays green while 23 tests stop running. New guard tests/studio/test_backend_ci_parallel_isolation.py fails if an isolated path is missing from the parallel run's ignores, or ignored with no serial step running it, for load_freeze and for the three hardware-spoof files that were already in this shape. Deleting either half of the load_freeze change fails it; a third test pins the command scan so it cannot pass by matching nothing. load_freeze: 23 passed in 20.14s serial new guard: 9 passed, and 1 failed under each of the two mutations test_ci_shell_suite_coverage.py + test_xpu_spoof_pipeline.py: 58 passed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Raise the matrix hang limit past the suite it was cancelling Not a review item; found while triaging why every PR in this stack carries a red check. The Backend CI matrix limit was 30 minutes, set when the step measured 14.1 to 15.0. It now measures 22.5 to 29.2, and it is cancelling: across the six open PRs on this stack, five had exactly one leg cancelled at 30m9s while its siblings passed at 25 to 29 minutes. Which leg loses is luck, so the red says nothing about the PR, and a cancelled leg burns the full 30 minutes for no signal. 45, which is what the Windows UI job already uses. This is a hang guard, not a budget, and it costs nothing until a job needs it. The real fix is running this matrix on all four cores, worth ~79 runner-minutes a push, and it stays blocked on the order dependencies this stack is working through one at a time. * Give each PowerShell probe its own script path Reported on this PR, real, and reproduced directly. _run_capturing_bytes wrote to tests/python/_{stem}_probe_{shape}.ps1, a name several tests share, so under xdist one case unlinks the script another has written before that one's pwsh child opens it. pwsh is installed on ubuntu-latest, so these do not skip there. Reverting to the shared name and running -n 4 four times: 1 failed each time. With a uuid in the name: 35 passed, 8 skipped serial 35 passed, 8 skipped on each of four -n 4 runs no probe scripts left behind * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop the pin scanner reading a generated filtered file as a second pin source Reported on this PR and reproduced. test_no_torch_filtering.py exercises install_python_stack._filter_requirements, which writes `.{stem}-filtered-XXXX.txt` BESIDE the source on purpose, so relative -r/-c includes still resolve. test_diffusers_pin.py::test_only_the_pin_file_names_diffusers scans REQ_ROOT.rglob("*.txt"), so under xdist it can see one worker's copy of diffusers-pin.txt and report it as a second source of the pin. Calling _filter_requirements on diffusers-pin.txt and then running that test fails exactly as described. It is not only an xdist problem: the function passes delete=False, so the file also survives a real install on any machine. The scan now skips dotfiles. That matches what it means -- no CHECKED-IN requirements file other than the pin may name diffusers -- rather than narrowing it: a real second source still fails it (verified with a probe file), and the production write path is untouched, since writing beside the source is the behaviour that makes includes resolve. 5 passed with a filtered file present, 1 failed with a real second source * Exempt only the generated filter files, not every dotfile Reported on this PR and right: a checked-in hidden requirements file such as .constraints.txt is a real place the pin could be overridden from, and the blanket dotfile skip took it out of the scan. Matched by the shape _filter_requirements actually writes now: a dot, the source stem, "-filtered-", and NamedTemporaryFile's 8-character suffix. generated filtered file present: 5 passed checked-in .constraints.txt naming diffusers: 1 failed --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-16 21:46:01 -07:00
Daniel Han	bcd2dfcf2c	Core: sweep every transformers model_type across a process pool (#8995) ... * Core: sweep every transformers model_type across a process pool test_compile_every_transformers_model_type walks every model_type the matrix's transformers ships and compiles each one. It was a serial loop, it was the second most expensive step in Core at 415.7s, and it runs once per matrix leg, so it cost about 21 minutes of runner time per push. The work is independent per model and it is compile bound, not import bound. Measured over the 359 model_types that have a modeling module: importing 2.6s total 0.007s mean compiling 81.8s total 0.228s mean So there is nothing to shave inside the loop; the only lever is running it on more than one core. Uses spawn rather than fork, since the workers start after torch is already loaded in the parent. Each worker re-imports the shim and gets its own hermetic cache directory. Nothing in this test asserts on the cache path; the per-model file assertions are in test_compile_real_modeling_module, which is unchanged and still runs in process. Ordered imap keeps the known and new-failure report in the same model order as before. Verified identical, not just equally green: every one of the 383 model_types gets the same verdict serially and pooled, 0 differences. serial 85.0s ok=359 pooled 25.7s ok=359 4 workers diffs 0 Whole step locally: 84.93s to 26.13s, 5 passed and 1 skipped either way. * Fail the pooled sweep fast when a worker dies A worker that dies mid-task (segfault, OOM killer) never sets its result, and Pool.imap then blocks forever: python/cpython#66587, open since 2014. _maintain_pool replaces the worker but the in-flight job's cache entry is never set, so the parent waits on a condition nothing will signal. The serial loop could not lose a worker -- a segfault killed pytest outright and turned the leg red in seconds. Pooled, the same crash would sit until the job's 35 minute timeout, which costs far more than running the sweep pooled ever saves. Drive the iterator with next(timeout=600) and turn a stall into a named AssertionError. chunksize drops from 4 to 1, and that is required rather than incidental: Pool.imap only returns an IMapIterator, the thing that has a timeout, when chunksize == 1. For chunksize > 1 it returns a bare generator expression over the chunks (Lib/multiprocessing/pool.py:396-420), which has no next(timeout=...) at all. Per-task IPC is noise next to a ~1s compile: chunksize=4 26.13s chunksize=1 28.39s both ok=359 skipped=24 known-broken=0 new-failures=0, matching serial. Guard verified against a worker killed mid-task: fires after the budget with the results collected so far, instead of hanging.	2026-08-16 07:39:59 -07:00
Daniel Han	c298c39658	CI: stop three workflows paying for work they throw away (#8976) ... * CI: stop three workflows paying for work they throw away Three independent bits of waste in the workflow layer, none of which changes what any job verifies. Core installed peft, accelerate and datasets before pinning CPU torch. All three declare a torch dependency, so pip resolved it from PyPI first: the default CUDA build plus fourteen nvidia_* wheels and cuda-toolkit, roughly 2.3 GB, which the pinned CPU wheel then replaced seconds later. Installing the CPU wheel first leaves a byte-identical environment and skips the download. Timestamped at 73s of a 148s step, across three matrix legs. Notebooks CI installed its Colab-shaped venv with a shell loop, one pip install per pin, each a full resolve against two indexes. With roughly 683 pins that step consumed its entire 25 minute cap on every leg of every run and has never once reached the Verify imports under spoof step that follows it: eight legs timed out on 15 Aug, six failed and two timed out on 16 Aug. It now does one bulk install and falls back to the per-pin loop only if that fails, so the best-effort contract that tolerates an unresolvable pin is preserved. Every pip cache in the repo was keyed on the wrong file. setup-python's default glob resolves to the only requirements.txt present, unsloth/kernels/moe/requirements.txt, five lines that have nothing to do with what most jobs install. Core's slot held about 8 MB put there by another workflow and the install then downloaded 3,249.7 MB regardless. Core now keys on pyproject.toml plus studio/backend/requirements/*.txt. Sixteen other workflows have the same defect and are left alone here. Security audit ran on every pull request with no path filter. Every job in it is a function of the dependency manifests, the lockfiles or the scanner sources: the three pip shards build their input entirely from pyproject.toml and studio/backend/requirements/**, the npm jobs read studio/frontend/package*.json, advisory-audit adds studio/src-tauri/Cargo.lock. Over a 198 pull request sample only 3.5% touched any of those, so the rest re-scanned an unchanged dependency tree for about 40 minutes each. The filter deliberately covers pull_request only. The daily cron and every push to main stay unfiltered, which is what catches the case a filter cannot see: a floating pin resolving to a newly published malicious version with no diff to trigger on. The worst case a filter introduces is that such a release is caught by the nightly run or at merge rather than on a pull request that did not touch dependencies. scripts/lint_workflow_triggers.py passes, and all three files parse. * CI: stop a linter squatting the pip cache slot the heavy jobs need Follow-up to the cache-key fix in this branch, with the live cache listing as evidence rather than inference. Every setup-python pip cache on Linux currently carries the same key hash, 3ca2b1fea..., because the default glob resolves to the repo's only requirements.txt, unsloth/kernels/moe/requirements.txt. Since the key does not distinguish jobs, the first job to finish for a given Python version owns the slot for all of them. The result, from the repo's own cache list: 554 MB python-3.13.15-pip-3ca2b1fea... 445 MB python-3.10.20-pip-3ca2b1fea... 444 MB python-3.11.15-pip-3ca2b1fea... 9 MB python-3.12.13-pip-3ca2b1fea... Core and the backend suite both run on 3.12. They restore the 9 MB slot, which Lint CI filled with ruff, pyyaml and codespell, and then download the full tree. Lint CI now keys on its own workflow file, where its pins are declared inline, and studio-backend-ci keys on pyproject.toml plus studio/backend/requirements/*.txt, matching what it installs. Core was already fixed earlier in this branch. Fourteen other workflows share the defect and are left alone deliberately: this change is bounded to the three jobs where the collision is measurably costing downloads, so the extra cache slots stay small. The repo is at 20.1 GB of its 50 GB allowance with 160 entries, and the largest entries are GGUF model caches of 4.7 GB and 3.3 GB that are expensive to rebuild, so adding slots is not free.	2026-08-16 03:41:44 -07:00
Daniel Han	926f76542f	Make the startup profile a gate, with budgets from its own measurements (#8965) ... * Make the startup profile a gate, with budgets from its own measurements * Size the startup budgets for a slow runner, and stop overstating what they catch 33 completed runs of this workflow, 99 profiles, 297 launches, no failed launch. Median time to a healthy port: ubuntu 3.24s, macos 3.02s, windows 5.11s. But a hosted runner can be slow for a whole run, and one already was: macos-15 in run 31932608086 measured 5.03/5.52/5.66s, 1.8x its own median, on a PR that touched nothing here. That run would have come within 0.48s of failing the 6.0s macos budget, and the same 1.8x on ubuntu or windows clears 5.0s and 9.0s outright. So budgets are now 2x the observed median rounded up to the next 0.5s, floored at 1.4x the slowest median seen: ubuntu 6.5, macos 8.0, windows 10.5. That size of gate catches a torch-sized regression, about 5s, and not a 1-2s one. The header claimed it caught the 2.2s pandas edge; on windows 9.0 it never did (5.11 + 2.2 = 7.3), and no budget that would is outside runner noise. A regression of that size is read off the import table in the job summary, which is how it was found. The header now says so. Also corrects the pipefail note. A step with "shell: bash" on a hosted runner already runs "bash --noprofile --norc -eo pipefail {0}", confirmed in the runner logs of run 31933219076, so the explicit set is belt and braces against that shell key changing, not the thing that makes the gate work. * Warn against making the path-filtered startup profile a required check A workflow skipped by path filtering never reports a status, so a required check on it sits at "Waiting for status to be reported" and blocks every PR that does not touch the paths list. GitHub documents this under troubleshooting required status checks, and its worked example is this exact shape. Now that the job can go red, the note belongs next to the filter. main has no required status checks and no ruleset today, and no workflow in the repo uses merge_group, so this is a guard against a later setting, not a live problem. * Drop the medians duplicated between the header and the matrix comment	2026-08-16 02:37:32 -07:00
Maheswar Kumar	5bb0bc6f73	studio: keep each chat's composer pills and settings with the chat (#8686) ... * studio: keep each chat's composer pills and settings with the chat The composer pills, the permission level and the retrieval controls were installation-wide, so switching chats carried one conversation's modes into the next, and reopening an old chat showed whatever the defaults happened to be. Sixteen of those settings now travel with the thread. chat_threads gains a settings_json column, ChatThreadSettings pins the contract PATCH /api/chat/threads/{id} accepts, and thread-scoped-settings.ts re-validates every value against the same literals and ranges before it is sent. Editing one of them with a chat open writes the snapshot onto that thread and leaves the installation defaults alone. Editing with no chat open still moves those defaults, which every chat without a snapshot follows, so a fresh install behaves exactly as before. A chat that stored nothing is pinned on first open, so a later change to the defaults cannot rewrite its modes. Full access stays session-only: the sanitizer drops it, and a write made while it is active carries through the level the chat already held. resolveToolsEnabledOnLoad, setBypassPermissions, setDeepResearchEnabled and both external-provider effects in chat-page.tsx read the open chat's value before the installation one, so a model load or a model switch no longer re-applies the defaults over the pills a chat is running with. A thread that stores no value for a setting falls back to the defaults rather than to the outgoing chat's. upsert_chat_thread COALESCEs the column, so the writers that rebuild a thread record cannot clear it; fork_chat_thread copies it; and list_chat_threads leaves it out, since the sidebar lists every thread and only an opened one reads it. normalizeStoredPermissionMode moves the legacy confirm-toggle migration out of the store so it can be pinned directly. Once the level is mirrored to /api/chat/settings the first hydration seeds it there, so the mapping can no longer be driven through the UI more than once per installation. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Hold a chat's edits until its snapshot lands, and keep deep research on a switch Two fixes on top of the thread-scoped settings work. A pill clicked between opening a saved chat and its GET /api/chat/threads/{id} returning was written to the installation defaults, because captureThreadScopedEdit needs activeThreadId to equal threadScopedSettingsThreadId and the pairing is only set when that read resolves. The click moved every snapshot-less chat's default and was then overwritten by the arriving snapshot, so it leaked and appeared to do nothing. Those edits are now held for the duration of the read: if the chat turns out to own a row they win over what came back and are stored on it, and if it does not (a new chat's runtime id, a legacy row, a failed read) they are replayed to the defaults exactly as before. On loopback the read wins the race, so this shows up on remote and tunnelled sessions. buildThreadScopedSnapshot carried the stored permissionMode forward when apply() held it back under Full access, but not deepResearchEnabled, which apply() holds back the same way for external models and incognito. Toggling any other pill in such a chat erased the true it had stored. Carried forward the same way. * Read a stored snapshot leniently, and keep the beacon for terminal events only Two problems found while simulating upgrades, downgrades and the three browser engines against this branch. settings_json is the first strictly validated nested model Studio builds out of the database rather than off the wire, and a stored snapshot outlives the build that wrote it. A newer Studio adding a setting, widening an enum or raising a bound writes a blob this one rejects, and refusing it 500s the chat on open, on fork and on patch, and takes GET /api/chat/export down for every chat, not just the affected one. _json_loads already shrugs off JSON that will not parse, so JSON that parses but postdates this build now gets the same treatment: rows go through thread_from_row, which drops what it cannot read. Only the read is forgiving. Both wire models stay extra = forbid, so a client still cannot invent a setting, and the row is left untouched, so upgrading again restores it. The unload flush also ran on visibilitychange, which is not terminal: it fires on every tab switch and the page carries on afterwards. That path used the keepalive beacon, which PATCHes the row directly and answers 404 for a thread whose row has not been created yet, while having already consumed the pending write. Terminal events keep the beacon; visibilitychange takes the normal path, which creates the row first and which the page is still alive to await. tests/studio/sim_thread_settings_portability.py covers the migration against a populated pre-existing database, idempotency, the COALESCE preservation, unicode and emoji and Windows-shaped paths through the column, WAL, and the SQLite floor. It is stdlib only, so it runs on Windows and macOS as well as Linux. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address the Codex review: pairing window, write ordering, and two over-broad guards Six items from the review, all reproduced against the code at head first. The composer is live while a chat's settings are being read, and until they land the store still holds the OUTGOING chat's values. snapshotQueuedChatRunSettings captures permissionMode at send time and sends it as permission_mode for the whole run, so a send in that window really does run under the previous chat's level, and a rejected read left those values up indefinitely. The store now drops to the installation defaults for the duration of the read, which is the only honest thing to show for a chat whose own settings are not known yet. Edits made during that window were still held, but the switch-away path replayed them into the installation defaults, which is the leak the holding exists to prevent. They are now committed to the thread they were made in; the values are still in the store at that point because the incoming chat's read has not resolved, and a chat with no row writes nothing, so an unsaved chat stays on the defaults. activeThreadScopedSettings was only refreshed when the debounce built its payload, so for 400ms after an edit neither it nor localStorage carried the new value. A model status poll landing there reverted the pill and the pending write persisted the revert. threadScopedOverride now prefers the live store while a captured write is pending; a pending pin keeps its own snapshot. Snapshot writes are chained per thread. The backend replaces settings_json rather than merging it, so two unordered writes do not merge, they pick a winner. Two guards were too broad. A chat first opened under Full access was pinned with no permission level at all and then followed the installation one forever; it now records the level underneath. And the deep research guard refused every external checkpoint, while externalCheckpointRefusesDeepResearch already exists and treats openai_codex as supported, which is the rule the composer follows. * Address the second Codex review: read/write ordering, the pre-hydration window, and two restore constraints - await this chat's in-flight snapshot PATCH before reading it back, so a chat edited, left and re-entered does not get its pre-edit snapshot applied over the values the user just set - keep the chat paired after a failed thread read, with a bounded retry, so later edits in it stay thread-scoped instead of moving the installation defaults - give each per-thread write a ticket so a queued write cannot land after the unload beacon and restore the older snapshot - start holding a chat's edits as soon as its id is known, not once /api/chat/settings has hydrated; the composer is interactive in between - keep Search and Thinking mutually exclusive on Kimi when restoring a snapshot - keep a stored Search/Code preference that a tool-less model has clamped off * Address the third Codex review: held-edit fidelity and the remaining write races - send only the edited fields, merged onto the row's own snapshot, when a chat's read never landed; the full replace was erasing settings the user never touched - release a held edit as soon as the first read says the row does not exist, instead of waiting for an unrelated history event - keep global hydration off a field whose edit is still held for its chat - keep a held edit out of the captured installation defaults - preserve a clamped Images and Fetch preference too, not just Search and Code - abort a settings PATCH that is already out when the unload beacon supersedes it * Address the fourth Codex review: settings merge semantics and a send that waits for them - PATCH gains settingsPatch, which applies only the fields it names. The unload path and the commit-on-leave path use it: both know what changed but not what else the row holds, and a replacement built from the defaults on screen was erasing the rest of the chat's snapshot - a replacement now keeps whatever the writing client could not read, so an older Studio opening a newer database no longer deletes the settings it had to drop - flush an edit held during pairing on a terminal event, keepalive, instead of relying on effect cleanup that unload does not guarantee - a legacy Dexie fallback means the backend read FAILED, so retry and keep the chat paired rather than releasing its edits to the installation defaults - park a send while the chat's own settings are still on their way, reusing the existing wait-and-send path, so a chat stored as ask cannot run on a global off * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address the fifth Codex review: order snapshot writes on the server, and stop parking sends forever - give up openly when the thread read is out of retries: staying paired held every send behind a wait with nothing left to resolve it, so the chat now falls back to the installation defaults and says so - record the installation defaults when pairing begins. Deleting a held field from the capture left it with no fallback, and on the session's first pairing the edited value then stayed live into the next chat, which is the same leak - add settings_seq and refuse a snapshot write older than the one already stored: aborting a fetch does not stop a handler the server has already started - do the read, merge and write of a snapshot in one transaction, so two tabs cannot both build a replacement from the same stale row - re-send with keepalive on a terminal event anything an earlier visibilitychange flushed normally but has not landed - settle the source snapshot before forking, or the fork copies the modes the chat had before the pill the user just clicked * Pin the pairing window invariants that keep getting broken Eleven assertions over the gap between opening a saved chat and its snapshot arriving: hold rather than release, capture the defaults up front, merge rather than replace, order every write, end the pairing when the read gives up. Each one has been broken at least once during review, and each break either leaks one chat's settings into another or loses an edit the user watched happen. Source assertions rather than a driven store, matching the sibling store tests: a .tsx barrel sits in the store's import graph, so it cannot be loaded in a bare node test. Checked against the earlier revisions, 10 of the 11 fail on the first of them and 4 on the most recent. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address the sixth Codex review: order writes per writer, not by wall clock - settings_seq is now compared only against the same writer's own earlier writes, identified by a per-tab id. Comparing one machine's clock with another's meant the browser that happened to be behind had every edit refused while still being told it had saved, which is worse than the race it was added to fix - carry the snapshot write in the same transaction as the guarded metadata write, or a rejected expectedTitle returns 409 with the settings already committed - remember a tab-close write that could not be confirmed and replay it next session: a chat whose row is still being created answers 404, and the creation that follows knows nothing about the edit - keep a chat's stored reasoningEnabled=false when the model forces thinking on - drop the thread-scoped state in compare mode. Two threads share one composer, so no single snapshot applies; leaving the module pointed at the last single chat let a model load read that chat's pills back into the pair Four more invariants pinned, all five failing on the previous revision. * Address the seventh Codex review: the queue path, retried pairings, and pending pins - gate the prompt queue on this chat's settings as well. handleSubmit reaches the queue branch before the send guard, so a prompt could be queued snapshotted from the installation defaults still on screen - sample the pre-window defaults once per chat rather than once per read attempt: a retry runs with the held edit already in the store, so re-sampling took that edit for the installation default and leaked it into the next chat - let a pending pin snapshot answer threadScopedOverride. A chat being pinned has no stored snapshot, so the override read null and a model load put the global pills back over the edit the queued write was about to persist Three more invariants pinned, all three failing on the previous revision. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address the eighth Codex review: four ways a run or a write could still go wrong - sample the pre-window defaults from the installation defaults, not the store. On a switch from one saved chat to another the store still holds the outgoing chat's pills, so its values were becoming the default every snapshot-less chat follows - keep a write watermark per writer. One writer column meant another tab's write replaced it, and the delayed request the ordering exists to refuse was then compared against a watermark that was no longer its own - bound the thread read. It gates sending, and the underlying GET has no timeout, so one that never answered parked every send in the chat with nothing to release it - wait for the chat's settings in the adapter, which every run reaches. Reload, Continue and send-from-edit never touch the composer's guard and could start a run on the installation defaults - keep a replay entry until its PATCH answers ok; authFetch resolves for the 404 this exists to recover from - track a debounce-fired write as unsettled too, so a terminal event can resend it - settle a held edit before forking, not just the debounce and the write chain Seven more invariants pinned, all seven failing on the previous revision. * Address the ninth Codex review: bind the run's wait to its chat, order the replay - await the pairing for the RUN's own chat. One promise for all of them meant a run started for A was released by B's pairing ending, and then read B's settings for A - keep last session's replay ahead of this session's writes. It carries the previous writer id, so nothing on the server orders it, and on a slow link a full snapshot from last time could land after an edit made now and revert it - track unsettled writes by identity. Two ordinary edits both carry a null snapshot, so the first request's settle was deleting the second one's tracking - do not resend an older unsettled snapshot for a chat the terminal flush has just beaconed; every beacon takes a higher seq, so the stale one would win - evict writer watermarks by last use, never by counter: every session starts at 1, so comparing across writers threw out the newest tab and kept long-dead ones - stop clearing the defaults sample when committing held edits, which put back the resample-on-retry bug fixed a round earlier - let an explicit clear beat a capability preservation, so enabling Search does not quietly keep Deep Research stored as on - bound the gating read at the fetch, so a stalled GET is aborted and not just raced Nine more invariants pinned, all nine failing on the previous revision. * Make the per-chat settings Playwright test unload any resident model first The Search and Code pills are disabled while a model that cannot run tools is loaded, and this test drives both. The CI job it was added to leaves a small GGUF resident from an earlier step, so every pill click timed out on a disabled button: the test failed for a reason that has nothing to do with what it checks. Unload first, which is also the state the test is about, since with nothing loaded the pills stay pre-selectable. Found by running the job on a staging repo while the org queue was backed up; this step has never actually completed on this PR. * Address the tenth Codex review: release pairing gates one chat at a time - release a chat's pairing gate only for that chat. The previous round held them per chat but resolved every one on any settle, so a run started for A was still freed by B's pairing ending. Leaving a chat mid-read now leaves its gate shut, since its snapshot never arrived, and the wait is bounded so a run cannot hang on it - clear a thread's replay entry once a write of this session's lands. The entry carries the previous session's writer id, which the server will not order, so a retry could revert settings changed since - bound each replay request. Every settings write waits on them, so one socket that never settled left the session unable to persist anything - keep a failed write tracked, so a terminal event still beacons it. Dropping it left nothing to resend and the edit came back reverted - let a failed snapshot write reach the fork, which now stops rather than making a fork that carries the pre-edit modes Six more invariants pinned, all six failing on the previous revision. * Do not open a pairing window for a chat that cannot have a row An unsaved chat carries an assistant-ui runtime id (__LOCALID_...), which no row exists for, so its settings read can only 404. Holding edits behind that certain failure meant a pill or permission level chosen on a fresh /chat did not reach the installation defaults until the round trip came back, and playwright_chat_ui asserts it is there immediately: 'Run automatically persisted ask, expected off'. That test is untouched by this PR and passed at an earlier revision of it, so this is a regression the branch introduced. Found by running the suite on a staging repo while the org queue was backed up. * Keep a chat's own settings when its pairing or its write does not land Four fixes from review: - A run whose pairing wait ran out no longer proceeds. The wait only expires for a chat left mid-read, whose gate is held shut on purpose, so the store describes a different chat by then; the run is refused instead. The wait is also raised past the read's own retry budget so an ordinary slow read never reaches it. - A fork now fails when the flushed write for the chat it is copying failed. The replacement path reports failure by resolving false, which the settle helper ignored, so the copy took the pre-edit snapshot silently. - A replay clears only the body it sent. A terminal event in this session can store a newer body for the same thread while an older replay is out. - A provider constraint no longer rewrites what a chat stored. Kimi's builtin search cannot run with thinking, so the composer moves the other pill with persist: false, deliberately bypassing the capture path; the next full snapshot then saved the provider's value over the user's. * Pin the installation permission level the per-chat test compares against The UI workflow boots this server on the Studio home the chat-ui and cross-browser permission tests have already driven, so the install is shared rather than fresh and arrives carrying whatever level they left in the mirrored settings. Every assertion in the file names a literal level, so the run failed on staging with the composer showing Run automatically where Approve for me was expected, before a single per-chat assertion had been made. Set the default from the unsaved chat the test already starts on, where no chat is open and the edit is the installation's, and print it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Bound the settings reads and recover the defaults hydration had to skip Four fixes from review: - Each pairing read now carries its own deadline and controller. bounded: true is the 30 second WRITE timeout, so the losing side of the 8 second race stayed open while the retry opened the next one, up to three per chat during an outage, and leaving the chat cancelled none of them. - The ensure step in front of a settings write is bounded and takes the caller's signal. It runs before the write, so neither the signal nor the write timeout reached it, and a stall there left the per-thread chain pending for the life of the page with reopening and forking that chat waiting on it. - A default hydration skipped because its field was held is kept and used when the pairing window closes. The restore was falling back to the pre-hydration copy, so snapshot-less chats followed this browser's stale value rather than the server's until a reload. - Tab-close snapshots are replayed even when /api/chat/settings fails to hydrate. They are rows' own settings and have nothing to do with that endpoint; the replay is guarded so it still runs once. Also documents why a refused settings write does not roll the metadata in the same PATCH back: a refusal means this writer already landed something newer, not that the write failed. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>	2026-08-16 00:13:04 -07:00
Wasim Yousef Said	83e163f13c	Revert "Desktop: ship a complete Linux AppImage (#8695)" (#8823) ... This reverts commit 93a77d4104.	2026-08-14 06:55:54 -07:00
alkinun	7bf8ade2db	Studio: enforce the research browser regression harnesses (#8736) ... * Studio: make research browser harnesses enforce their claims * Have the browser harnesses own their dev server for PR #8736 The new CI step backgrounds `npm run dev`, so $! is the npm wrapper and the EXIT trap leaves the vite node child alive holding the port and the step's stdout pipe. Reproduced: after the trap fires the port still answers 200. The ANSI smoke already solved this, so its start_vite / stop_process / drain_process_output move into _playwright_robust.py and all three harnesses use them. The CI step is two plain python3 calls now, and each file runs standalone the way its own docstring says. Readiness checks content, not status. Vite's SPA fallback answers 200 with index.html for anything missing, so a status check passes on a deleted smoke page. Confirmed locally: /smoke-DOESNOTEXIST.html returns 200. The report phase keeps a real, hit-tested click. A synthetic element.click() lands even with `body { pointer-events: none }` stranded, which is the freeze under test, so clicks_registered had stopped covering it. Measured both ways against a stranded layer: synthetic +1, real +0 and not actionable. The stall probe stays alongside it as main_thread_stall_ms, since nothing here reads an input timestamp, and its budget goes to 1000ms: 500 left only 1.2x against 342-416ms measured on a loaded host, and 1000 still fails ten times the report size (1518ms). Also: chat wall time and rAF count come from one page evaluation so they bracket the same interval; smoke-ansi-main.tsx joins the typechecked entries; the ANSI default port moves off the contended 8000; the job timeout goes to 20 minutes now that two browser smokes sit inside it; and the contract test pins the new verdicts plus the self-hosting rule. Verified: all three harnesses exit 0 standalone with no leftover vite and every port closed; typecheck 0; 2389 frontend tests; contract tests pass and fail when the new guards are removed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden the shared dev-server lifecycle for PR #8736 Simulated the failure modes of the self-hosting change in an isolated venv. Five were real; all five are fixed here, each with a test that fails when the fix is reverted. 1. A busy port was measured instead of refused. Under --strictPort our vite exits, and the readiness poll then talks to whoever else holds the port. Worst case is a squatter serving a page that happens to contain the entry string, which content matching alone accepts. start_vite now refuses an occupied port up front. 2. A dead server cost the full 120s timeout, three times per CI run. Readiness now takes the process and gives up the moment it exits, surfacing vite's own last lines. Measured: 120s to 0.0s. 3. SIGTERM orphaned the server. `finally` covers exceptions and SIGINT but not SIGTERM, which is what a CI cancel sends, so the exact leak this work is about survived a cancelled job. SIGTERM and SIGHUP now tear down registered servers, chaining to any previous handler, and atexit covers the rest. 4. Teardown could raise over the failure that called it. stop_process runs from a `finally`, and a child outliving SIGKILL turned a clean harness failure into a TimeoutExpired traceback with the real error lost. 5. An exported-but-empty SMOKE_BASE_URL counted as external, so no server started and the harness drove "" as its base URL. Empty now means unset. tests/studio/test_playwright_server_lifecycle.py drives both platform branches by injecting os.name, so the Windows path (CREATE_NEW_PROCESS_GROUP, taskkill /T then /T /F) is checked on every run rather than on someone's machine. It needs no browser and no npm, so CI runs it before the Chromium install. Verified: all three harnesses pass self-hosting and in the pre-existing SMOKE_BASE_URL mode, where an external server is correctly left running; ANSI smoke passes on chromium, firefox and webkit; no leftover processes and every port closed; 22 simulations and 23 in-repo tests pass; all four mutants of the fixes above are caught. * Run the lifecycle tests after the playwright install They import the harnesses, which import playwright, so the browserless step I put before the install could never have worked on a clean runner. Caught by staging CI, reproduced locally in a pytest-only venv (same 5 failures), and confirmed fixed in that same venv once playwright is present: 23 passed. pytest moves into the existing install line rather than getting its own pip call. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments added for PR #8736 Comment-only pass: collapses the duplicated SMOKE_BASE_URL notes, the teardown and readiness asides, and the click rationale in the contract test. Intent unchanged, 12 fewer lines. Pre-existing comments from #8633 are left alone. AST-verified comments only (comment_tools.py check: 4/4 OK), 56 tests pass. * Skip the harness-import tests when playwright is absent Cross-platform staging CI on macos-14 failed 5 of 23: the two tests that reload a harness module pull in playwright.sync_api, and that runner does not ship it. Several workflows run tests/studio without installing playwright, so the file broke collection there rather than only in Frontend CI. pytest.importorskip is the convention already used in this directory (test_cached_model_path_selection.py, test_pdf_qa_recipe_contract.py). The other 18 touch stdlib-only helpers and are unaffected. Verified in a pytest-only venv: 18 passed, 5 skipped, where it was 5 failed. With playwright present all 17 still run rather than skipping. * Honour SMOKE_BASE_URL in the ANSI smoke It started its own server unconditionally, so pointing it at an external one still spawned a second vite, and with the new occupied-port check it now raises before the external page is ever tested. Reproduced against a server on the default 5203: RuntimeError: 127.0.0.1:5203 is already serving. The other two harnesses already derive OWNS_SERVER from SMOKE_BASE_URL; this brings the third into line. Self-hosting is unchanged, since OWNS_SERVER is true there. Verified both ways: external mode exits 0 and leaves that server running; self-hosting still starts and stops its own. * Let the POSIX teardown tests run on Windows Cross-platform staging on windows-latest failed 3 of 23: AttributeError: <module 'os' (frozen)> has no attribute 'killpg' os.killpg is POSIX-only, so monkeypatch.setattr had no attribute to replace and raised during setup. The tests that exist to prove the Windows branch is covered were themselves the ones Windows could not run. raising = False lets them install the stub on either platform; the assertions are unchanged, and os.name is already injected so the POSIX branch is what they exercise. Verified with os.killpg deleted from the interpreter to mimic the runner: 17 passed, same as on POSIX. * Make the Windows paths actually work in the harness and its tests Two Windows-only breaks, both confirmed against the Python docs before fixing. signal.SIGKILL is Unix-only, so the tests that force os.name to posix to exercise the POSIX teardown could not name it on a Windows interpreter, and raised after the fake process timed out. raising=False on os.killpg was not enough. A posix_branch fixture now stubs both, and the assertion compares against the same portable constant. start_vite ran a bare npm. On Windows npm is the batch file npm.cmd, and CreateProcess cannot execute a .cmd with shell=False, so the self-hosting default this PR documents would have failed with FileNotFoundError before vite started. shutil.which honours PATHEXT and resolves npm.cmd there, and returns /usr/bin/npm here. Verified with os.killpg and signal.SIGKILL both deleted from the interpreter: 17 passed, same as POSIX. The ANSI smoke still runs end to end on Linux through shutil.which. --------- Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-14 06:44:58 -07:00