mirror of
https://github.com/unslothai/unsloth.git
synced 2026-08-23 07:44:06 +00:00
361 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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. |
||
|
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> |
||
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> |
||
|
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. |
||
|
Daniel Han
|
32ba55b578
|
Run the zoo suite in parallel, minus the two files that cannot share a worker (#9285)
Some checks are pending
Windows Unsloth GGUF CI / real-VS detection (VS 2022) (push) Waiting to run
Windows Unsloth GGUF CI / real-VS detection + VC++ round-trip (VS 2026) (push) Waiting to run
Unsloth export capability / capability (ubuntu-latest) (push) Waiting to run
Unsloth export capability / capability (windows-latest) (push) Waiting to run
Frontend CI / Frontend build + bundle sanity (push) Waiting to run
Frontend CI / Frontend unit tests (Windows) (push) Waiting to run
Unsloth GGUF CI / GGUF inference smoke (API, tools, vision) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-15) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-26) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-15-intel) (push) Waiting to run
Mac Studio Install Matrix CI / Install + load (macos-26-intel) (push) Waiting to run
Mac Studio UI + API + Update + Inference CI / Chat UI, API, Update and GGUF inference (push) Waiting to run
Unsloth Tauri CI / Tauri Linux debug build (no codesign) (push) Waiting to run
Unsloth Tauri CI / Rust unit tests (windows) (push) Waiting to run
Unsloth Tauri CI / Rust unit tests (macos) (push) Waiting to run
Unsloth UI CI / Chat UI Tests (extra) (push) Waiting to run
Unsloth UI CI / Chat UI Tests (picker) (push) Waiting to run
Unsloth UI CI / Chat UI Tests (banner) (push) Waiting to run
Unsloth UI CI / Chat UI Tests (chat) (push) Waiting to run
Unsloth UI CI / Loaded-models indicator (cross-browser) (push) Waiting to run
Unsloth Update CI / Unsloth Updating Tests (push) Waiting to run
Windows Unsloth API CI / Unsloth API & Auth Tests (push) Waiting to run
Windows Unsloth GGUF CI / GGUF inference smoke (API, tools, vision) (push) Waiting to run
Windows Unsloth GGUF CI / Unsloth install + inference without Visual Studio (push) Waiting to run
Windows Unsloth GGUF CI / setup.ps1 units, no-VS resolve, VC++ round-trip (windows-latest) (push) Waiting to run
Windows Unsloth UI CI / Chat UI Tests (push) Waiting to run
Windows Unsloth Update CI / Unsloth Updating Tests (push) Waiting to run
Wheel CI / Wheel build + content sanity + import smoke (push) Waiting to run
Windows Application Control CI / installer survives a denied unsloth.exe (push) Waiting to run
Workflow trigger lint / workflow-trigger lint (pull_request_target / cache-poisoning) (push) Waiting to run
* 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>
|
||
oobabooga
|
de102032ee
|
Desktop: ship a complete Linux AppImage (#9113)
--------- Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com> |
||
|
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> |
||
|
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. |
||
|
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. |
||
|
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>
|
||
|
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>
|
||
|
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. |
||
|
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>
|
||
|
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). |
||
|
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>
|
||
|
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>
|
||
|
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 |
||
|
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 ( |
||
|
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.
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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
|
||
|
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 |
||
|
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> |
||
|
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> |
||
|
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>
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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. |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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. |
||
|
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. |
||
|
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> |
||
|
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 |
||
|
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> |
||
|
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>
|
||
|
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.
|
||
|
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.
|
||
|
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
|
||
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>
|
||
Wasim Yousef Said
|
83e163f13c
|
Revert "Desktop: ship a complete Linux AppImage (#8695)" (#8823)
This reverts commit
|
||
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> |
||
|
Daniel Han
|
482abfabfa
|
Give Unsloth Studio its first CUDA coverage, on a Kaggle T4 (#8489)
* Run deterministic notebook smoke tests on real Kaggle T4s
The notebooks are written for a T4: sm_75, fp16 with a gradient scaler,
xformers rather than flash-attention, 16GB. Nothing in CI runs on that
card, so a regression that only appears there reaches users first.
Adds a gated job that launches one Kaggle GPU session and runs the smoke
payload once per T4. The account has a 60h weekly budget shared with
other consumers, so the job is a deliberate minority consumer of it:
paths filters (only 9.9% of a week's 444 main commits touch unsloth/),
a ~10% sampling draw keyed on the run id so a re-run cannot reroll, a
quota floor read live from Kaggle's own API, and a stand-down when the
account already has a kernel in flight. Expected cost is 5-7 GPU-hours
a week.
Only a payload that ran on a T4 and disagreed with its assertions turns
the check red. Throttling, the 2-kernel concurrency cap, a dead session
or a download that would not complete are all warnings, because a free
external service blocking merges would be ignored within a week.
The payload asserts three things, worth different amounts. Two fresh
processes must agree bitwise on per-step loss and grad_norm; that one is
exact, and it is fresh processes because two in-process cycles disagreed
from the first step while two processes agreed on all ten. Greedy
decoding must emit the canary string exactly, which catches trained
weights never reaching the generate call. Metrics must sit inside a band
around a committed reference, which is a tolerance and never an equality,
since fp16 reduction order moves with the driver and the libraries.
Ten steps rather than three: under fp16 the scaler skips the first two
overflowing steps, so a three-step run lands one real update and the
canary never forms. The dataset is supplied as prompt/completion columns
so the loss falls only on the answer.
* Make the payload cells the T4 rejected actually run there
Two launches on real Kaggle T4s died in the scaffolding before a training
step ran, both of them in generated code that nothing had ever parsed.
The dependency probe imported unsloth_zoo before unsloth. Zoo's __init__
ends with a find_spec("unsloth") guard and raised "Please install Unsloth
via pip install unsloth" on a session where unsloth was installed and
imported cleanly one entry later, so the payload aborted over a dependency
that was not missing. Probe unsloth first, and invalidate the import caches
after the pip installs that this interpreter did not see at startup.
The reference argument was generated as a shell fragment and spliced into
the middle of a Python list literal, with doubled braces that left ROOT
unsubstituted. The cell was a SyntaxError, and the workflow passes
--reference on every path, so no payload could ever have reported.
Verified locally by executing the regenerated run cell end to end: two
fresh processes, bitwise identical metrics, canary exact.
* Bound the in-flight kernel survey by session length, not kernel count
The concurrency check status-checked the twelve most recently run kernels,
which is a sample rather than a search: a kernel that started three hours
ago and is still running is missed the moment twelve newer ones have since
run, and the push then fails at the account capacity cap and is reported as
infra.
Kaggle kills a notebook session at twelve hours, and the listing is sorted
by last run time, which for an unfinished kernel is when it started
(measured: a kernel pushed at 10:05:19Z lists as 10:05:19.297). So walking
until the first entry older than that ceiling covers every kernel that can
still be in flight, and stopping there is exhaustive rather than arbitrary.
An hour of slack is added for clock skew.
Two silent ways to read an unanswerable question as a green light are
closed with it. Hitting the page cap now marks the survey incomplete, and
a survey where no status could be read at all is unknown rather than idle.
Both stand down, which is the cheap direction: the next commit draws again
in minutes.
On the live account this now costs three status calls instead of twelve.
* Stop the reference band check from passing NaN unconditionally
The band comparison did its NaN handling by accident, and it got it wrong.
Under fp16 the gradient scaler logs a NaN grad_norm on every step it skips,
so a committed T4 reference genuinely contains NaN. Left to the arithmetic,
abs(x - NaN) is NaN and NaN > tolerance is False, so those steps passed
whatever they held, including the case the check exists for: a step that
used to overflow and no longer does. Compare NaN to NaN as equal and NaN
against a number as a deviation, and treat a field logged on one side only
as a deviation too, since no tolerance covers a change in shape.
The mapping from an out-of-band verdict to the failure list moves into
reference_failures() so the path that turns the job red can be tested
without a GPU.
The references README now says plainly that no reference is committed, that
one must not be borrowed from other hardware, and how to lift one out of a
green run's evidence rather than spending a session on it. It also records
a measurement: the absolute floor never engages on this trajectory. The
smallest value on the ten-step curve is a loss of 0.1428 against a floor of
0.05, so the floor changes no verdict today and is kept for a configuration
that does go lower.
* Name the cause when a kernel finishes and reports nothing
A kernel that ends COMPLETE with no payload report is the hardest outcome
to read: the summary has no metrics to show and the reason sits in an
artifact nobody downloads. It is also the outcome both real T4 failures
took.
Kaggle returns the kernel log as a JSON array of stream records rather than
as text, so the interesting line arrives split across dozens of them and
reading the file directly shows a wall of JSON. Flatten it, filter to the
driver and payload sentinels plus tracebacks, and fold the tail into the
job summary. Checked against the evidence of a real failed run: the
SyntaxError that killed it is now visible without leaving the summary.
* Cover the four T4 CI fixes with CPU-only tests
The generated cells are now compiled on both the with-reference and
without-reference paths, which is what would have caught a SyntaxError that
instead cost a Kaggle session, and the dependency probe's module order is
asserted.
The in-flight survey gets a fake Kaggle client and the case that motivated
it: one kernel still running behind forty newer finished ones, which a
count-bounded scan misses and a time-bounded scan cannot. Also the window
boundary, timezone-aware and missing timestamps, the page cap, and the
difference between some statuses unreadable and all of them.
The band check is now proved to fail rather than assumed to work: a
perturbation half a band-width past the limit, a moved scaler skip pattern
in both directions, a field that stopped being logged, and a length
mismatch, each asserted through to the failure list. Three tests perturb
the committed reference itself and skip, saying so, until one exists.
41 passed, 3 skipped.
* Parse every generated cell on every build path before spending a session
Two of the three Kaggle sessions spent so far died on generated code that
nothing had ever parsed. The compile check now builds all three paths the
generator has, using the workflow's own argument list verbatim, including
the one where a committed reference exists and is carried inline as a
fourth file: that path becomes live the moment a green run supplies the
file, and would otherwise first be exercised on Kaggle.
Parsing alone is not enough. A template hole that substitutes to a bare
identifier parses and then NameErrors, which costs the same session, so
every cell is also checked in execution order for names nothing before it
defines. Both checks were confirmed to go red: the first on the exact
shell-fragment-in-a-list-literal defect that cost session three, the
second on a run cell reading a name no cell binds.
The carried sources are also decoded the way the kernel will decode them
and compared byte for byte with the repo, so the T4 cannot quietly run
something other than what is committed.
* Commit the T4 reference the first green Kaggle run produced
Kernel danielhanchen/unsloth-t4-ci-e3c6661f, terminal state COMPLETE, both
payloads passing on their own Tesla T4 of one session. Two fresh processes
agreed bitwise on all ten steps on both cards, max_abs_diff exactly 0.0 for
loss and for grad_norm, and the two cards independently produced the same
ten values as each other, so four processes agree rather than two. All four
cycles emitted the canary exactly. The scaler skipped steps 1 to 3 every
time, which is the behaviour the ten-step length exists to accommodate.
The file is reports[0] of that run, copied whole by the recipe the README
already documented, and nothing in it is from other hardware.
One assumption the run contradicted, now recorded rather than repeated: the
session had no xformers. unsloth installs --no-deps, unsloth_zoo does not
carry xformers and the Kaggle image does not either, so the banner read
Xformers = None and this trace is the fallback attention path. Adding
xformers to the install would move these numbers and require a recapture.
The floor's smallest observed value moves from 0.1428 to 0.0871, still
above the 0.05 denominator floor, so the floor stays inert and the test
that re-derives that keeps passing.
The three tests that perturb the committed reference now run instead of
skipping. 45 passed, 0 skipped.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Name the in-flight tolerance the Kaggle gate stands down on
The gate declines to launch whenever any kernel of the account is in
flight, which is stricter than Kaggle's 2-kernel cap requires. That is
the intended policy, so it is now a named constant with the tradeoff
written next to it rather than an implicit property of the code, and a
--allow-in-flight argument that cannot be raised to the cap itself.
Default behaviour is unchanged.
Also files the workflow under 'never' in ci-preempt.json: cancelling a
run does not stop the Kaggle kernel it already pushed, and the orphan
then bills quota to its own ceiling with nobody watching.
* Run three steps, and refuse a reference captured at another count
max_steps drops from 10 to 3. Measured consequence: under fp16 the
gradient scaler starts at 65536 and skips every step it overflows on,
and the committed reference shows steps 1, 2 and 3 all skipped, so a
3-step run of the old configuration applied zero optimizer updates.
--init-loss-scale pins the scaler below the overflow point so those
three steps are real updates, and optimisation_failures() fails a run
whose every step was skipped rather than letting it report as training.
The committed reference was captured at 10 steps and no longer
describes the run. check_reference now reads the max_steps the
reference records and refuses to compare across counts, as a failure
with both counts named -- including when the reference does not say
what it was captured at. A quiet pass there would be a green check
comparing a run against a curve it has nothing to do with.
The reference is therefore stale until one recapture run. The workflow
gains skip_reference_band for exactly that dispatch, and the recipe is
in references/README.md.
Also brings the workflow onto the repo's conventions: pinned action
SHAs matching the rest of CI, a workflow-level concurrency group that
deliberately does not cancel in progress, a fork guard on the gate
job, typed dispatch inputs and an artifact name without the run id.
* Keep the run at ten steps, and say why three does not work
The committed reference is the evidence: the fp16 scaler reports grad_norm
NaN at steps 1, 2 and 3 and a finite value from step 4. It starts at 65536,
halves on each overflow, and skips the step it overflowed on, so a 3-step
run applies zero optimizer updates. Loss stays around 10, the canary never
forms, and the band would compare three points of a curve that never moved.
Shortening also saves nothing worth having: a launch costs about 0.08h and
that is almost all pip install, not training.
The step-count guard, the all-steps-skipped guard and --init-loss-scale all
stay, since they are what makes a shorter run possible later and what stops
a vacuous one reporting green. The pin is off by default so the committed
reference keeps applying.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Turn one T4 payload into four, and pair a pinned control with a canary
The workflow ran a single tiny SFT payload on both cards of one Kaggle
session and spent about 1.4 GPU-h a week doing it. The budget is now 40,
and the question asked of it is wider: catch regressions in trl,
transformers, accelerate, peft, bitsandbytes, torch and vllm, and catch
torch.compile falling over on sm_75.
Four legs now, two kernels of two T4s each, described once in
.github/scripts/kaggle_t4_ci/legs.py rather than restated in YAML:
control the existing tiny Qwen2.5-0.5B determinism run, pinned
canary the same run on the newest library set Unsloth's own
constraints allow
gptoss gpt-oss-20b LoRA, for torch.compile and the forced float32 path
grpo Qwen3-4B GRPO through a vLLM engine on the same card
control and canary are one instrument rather than two tests. Same payload,
same seed, same data, same step count, on the two cards of the same
session; the only difference between them is the installed versions. A red
canary beside a green control is a library release, and the job summary
already puts the two version sets side by side so the bisect is a diff
rather than an investigation. The reference band applies to the control
alone: two library sets do not produce one fp16 trajectory, so the canary
asserts what does not depend on versions instead.
The pins are evidence rather than preference. transformers and trl are the
pair the committed reference was captured with; peft, accelerate and
bitsandbytes come from probe kernels 8161ceb9 and 7ab727f1, two independent
T4 sessions that ran the identical base install and agreed. torch stays
unpinned and recorded, because it comes from the Kaggle image and replacing
it is the failure mode the grpo leg spent two probes on.
The gate now separates this workflow's own kernels from everybody else's.
It takes both of Kaggle's concurrency slots, and only when the account is
otherwise idle: one kernel belonging to anyone else still stands it down
entirely, which is the same policy as before stated at the right grain.
* Leave the vLLM leg unwired, and stop trusting Kaggle's own kernel timeout
Three probe sessions tried to run the Qwen3-4B GRPO leg on a T4 and none of
them reached a training step. None of them failed for a reason to do with
sm_75, memory or GRPO either: vllm==0.11.2 pins torch==2.9.0, so the leg has
to replace the Kaggle image's torch, and that is what killed all three.
With the image's site-packages visible, pip treats torch's pinned NVIDIA
runtime packages as already satisfied by the copies belonging to 2.10, and
the result is a torch that installs cleanly and cannot be imported --
libcusparseLt.so.0 first, then ncclCommWindowRegister once that one was
named explicitly, identically on vllm 0.11.2 and 0.15.1. An isolated venv
resolving the whole stack got further and then the session wedged.
So the leg keeps its payload, its assertions and its install, and moves to
legs.UNWIRED with the three kernel slugs and what each one measured. Wiring
it now would make the check permanently red and spend the budget doing it.
The second kernel keeps its spare T4, which costs nothing, and that is where
the leg goes when the install works.
The third probe also cost about an hour of quota teaching us that the
push-time kernel timeout is not a budget control. Pushed with -t 5400, its
own nbconvert crashed at t=406s and the session sat in RUNNING for over two
hours -- past that ceiling and past the launcher's own deadline, because one
status call had blocked with no socket timeout to bound it. Deleting the
kernel by hand took the account's used-hours figure back down, which is the
measurement that settles which control is real. The launcher now sets a
socket timeout and deletes every kernel it pushed on every path out, and
the workflow header says which of the three bounds is load-bearing.
Sampling recomputed on the payload that actually ships: 40%, from measured
per-kernel cost rather than the old estimate, with the note that wiring a
fourth leg means recomputing it rather than editing one line.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Re-solve the GRPO leg's install so it stops replacing the image's torch
Three probe sessions never reached a training step, and all three died the
same way: vllm==0.11.2 pins torch==2.9.0, so the leg had to replace Kaggle's
torch 2.10.0+cu128, and pip treats the image's NVIDIA runtime packages --
which belong to 2.10 -- as satisfying the new torch's pins. That imports as
libcusparseLt.so.0 missing, then as libtorch_cuda.so undefined symbol
ncclCommWindowRegister one package along, then as an isolated venv that spent
an hour of quota resolving a CUDA stack and never produced payload output.
None of that is a question about sm_75, memory or GRPO. It is one line: no
vLLM release in the 0.11-0.16 range pins the torch the image ships. Releases
0.17.0 through 0.19.1 pin torch==2.10.0 exactly, so pin 0.19.1 and the leg
replaces nothing, keeps system_site_packages, and installs like any other.
Drop xformers with it. Its vLLM attention backend was deleted in 0.12.0, so
it would be a package nothing selects. sm_75 has no FlashAttention and no
FlashInfer, and the ladder in vllm/platforms/cuda.py falls through both to
TRITON_ATTN; the leg names that in VLLM_ATTENTION_BACKEND so a release that
reorders it goes red here rather than quietly selecting something else. 7.5
is still in CUDA_SUPPORTED_ARCHS at v0.19.1 and fp16 is a supported dtype
below capability 8.0.
Still unwired. Two things are static evidence rather than a run: whether
0.19.1 starts on sm_75 at all, and whether 8GB of 16-bit weights plus an
engine plus a LoRA trainer fit in 14.56GB. UNWIRED says so in those words,
and a test asserts the note still names the pin and still says what is
unknown, so nobody wires this off prose that outlived its measurement.
Also ports the vacuous-test fix from the stacked Studio branch: pre-commit.ci
reformatting default=10 to default = 10 stopped the payload regex matching,
so the step-count agreement check was comparing nothing.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Stop the GRPO leg JIT-compiling flashinfer on a machine that cannot link it
First probe on the re-solved install, kernel unsloth-t4-ci-e2d9ce9b. The
install works: torch stayed at the image's 2.10.0+cu128, vllm 0.19.1
installed and imported, xformers was absent as intended, and the payload ran
on a real Tesla T4 reporting capability 7.5 with TRITON_ATTN accepted. It
reached engine construction, which is further than any earlier probe got by
the whole width of the install.
It died in flashinfer 0.6.6's JIT, and not for an sm_75 reason. All three .cu
files compiled cleanly for -gencode=arch=compute_75,code=sm_75. The LINK
failed:
/usr/bin/ld: cannot find -lcuda
-L/usr/local/cuda/lib64/stubs is already on the command line, so the image
has no driver stub libcuda.so, only the runtime libcuda.so.1. Nothing about
that is fixable from a payload.
Set VLLM_USE_FLASHINFER_SAMPLER=0 so nothing JITs. The sampler has a native
path, and skipping the build also saves a four-file nvcc compile inside a
session billed by wall clock. UNWIRED records what the probe measured and
narrows what is left to one question: whether the engine builds now.
* Make -lcuda resolvable so flashinfer can link what it compiled
Second probe, kernel unsloth-t4-ci-916d5986, failed identically to the first
with VLLM_USE_FLASHINFER_SAMPLER=0 set. That is the informative part: the
JIT is not reached only through the sampler, so switching off one consumer is
whack-a-mole.
The failure was never sm_75. Both sessions compiled all three .cu files
cleanly for -gencode=arch=compute_75,code=sm_75 and died at the link:
/usr/bin/ld: cannot find -lcuda
-L/usr/local/cuda/lib64/stubs is already on the command line; the image ships
no libcuda.so at all, only the versioned libcuda.so.1, and the linker will
not resolve -lcuda against a soname. Normally the toolkit's driver stub fills
that gap; this image has the directory and not the file.
So symlink libcuda.so at the real driver in a scratch dir and put it on
LIBRARY_PATH, which gcc and ld search for -l. No root, nothing written into
/usr/local, and it fixes every flashinfer op rather than one. Linking against
the real driver instead of a stub is correct here: the driver is present,
which is the whole reason a stub would have substituted for it.
Also widen the captured traceback to head AND tail. The last probe's
6000-char tail was entirely ninja's own output, so the Python frames naming
the caller were exactly what got dropped.
Probe 2 also measured the fit question the notebook raised: peak reserved
7.96GB of 14.56 with load_in_4bit false, so 16-bit weights are not what is
short of room here.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Only count a libcuda the linker will actually search for
Kernel unsloth-t4-ci-d0d480b6: the shim reported already_linkable and did
nothing, and the link failed anyway. It had found libcuda.so in
/usr/local/cuda/compat -- which is real, and which nothing passes with -L.
The failing ninja line names /usr/local/cuda/lib64 and
/usr/local/cuda/lib64/stubs, and those are the only two directories that can
answer -lcuda.
So the precondition now checks exactly those two, and compat becomes what it
is actually good for: a symlink TARGET when ldconfig and ctypes both come up
empty. A library the linker will not search for is not a library the linker
can find.
Two tests: one reproduces the compat-only machine and asserts the shim is
built anyway, one pins the searched directory list, since widening it is
precisely how the check went wrong.
* Give the base model a chat template, and record what the link shim did
The link shim works. Kernel unsloth-t4-ci-27b0dc2e reported
libcuda_shim: needed true, applied true,
searched [/usr/local/cuda/lib64, /usr/local/cuda/lib64/stubs],
real /usr/local/cuda/compat/libcuda.so
and flashinfer is gone from the failure entirely. Memory went from 7.96GB to
11.36GB of 14.56, which is the vLLM engine actually loading, and the run
reached trainer.train() -> _run_epoch -> training_step. Both questions
legs.UNWIRED was holding this leg for -- does vLLM start on sm_75, does it
fit in 14.56GB -- are answered by that.
It then failed on the payload's own gap:
ValueError: Cannot use chat template functions because
tokenizer.chat_template is not set
unsloth/Qwen3-4B-Base is a base model and ships none. The notebook this leg
comes from solves it with an SFT priming stage that installs a template
before GRPO starts; this leg has no priming stage, so it sets a minimal
ChatML template directly and records in the report which of the two worlds
the run was in.
The base model stays. GRPO on an instruct model would measure the instruct
tuning as much as the run, and the payload's rewards are format-and-digit
rewards a base model can move inside three steps.
A test renders the template rather than matching it as a string: a template
that does not render would trade a failure at step 1 for a failure at step 1
with a longer traceback.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Wire the GRPO leg: it runs on a T4 and the rewards move
Kernel unsloth-t4-ci-53efcc4e passed on a real Tesla T4. Step 2 logged
reward_std 0.707 and grad_norm 0.772, peak 13.60GB of 14.56GB, three steps
in 192s, and fast_generate returned coherent text. reward_std is the
criterion rather than loss, because TRL's GRPO loss is ~0 by construction at
num_iterations=1 and beta=0 and would pass on a run that learned nothing.
The config in args is the one that fit, not the notebook's. Two probes with
seq 2048, 4 generations, rank 32 and utilization 0.9 died in the backward at
unsloth_zoo/gradient_checkpointing.py:1013, peaking at 15.97GB in 16-bit and
19.25GB in 4-bit. 4-bit alone is not the lever: it peaked HIGHER, because
quantizing weights does nothing for activations while utilization 0.9 still
hands vLLM ~13GB up front, and UNSLOTH_VLLM_STANDBY returns the weights
during training but not the KV cache reservation.
An earlier probe reported this as 'CUDA error: an illegal memory access
was encountered' from empty_cache() inside vLLM standby. That was the async
shadow of the same OOM; CUDA_LAUNCH_BLOCKING=1 produced the honest error.
UNWIRED is now empty. The test that read its grpo entry is replaced by the
invariants that outlive any one leg: every leg is wired or explained, nothing
is both, and an unwired note says what is open. Two new tests pin the config
and the vLLM pin, so restoring either to a value that OOMs fails here rather
than on Kaggle.
* Add a CUDA_LAUNCH_BLOCKING switch to the GRPO payload
An illegal memory access is reported at whatever CUDA call synchronises
next, which on the first T4 GRPO run was empty_cache() inside vLLM standby,
nowhere near the kernel that faulted. Re-running with this flag produced the
honest error instead: torch.OutOfMemoryError in the backward. That is what
turned an unexplained CUDA fault into a memory budget, and the budget is now
the leg's config.
The environment variables are set before the first import torch in the
process rather than merely before train(), because they have to precede CUDA
context creation. It serialises every kernel launch, so it stays a switch and
never a default, and it is recorded in the report because it changes what a
timing means.
* Give Studio its first CUDA coverage, on a Kaggle T4
Every Studio workflow in this repo runs on ubuntu-latest, macos-15 or
windows-latest. macOS gives Metal and MLX some hardware; the CUDA path is
exercised by nothing at all, and studio-inference-smoke.yml uses a 270M
GGUF precisely because it has to decode on a CPU.
tests/kaggle/studio_gpu installs Studio the supported way on a Kaggle T4,
starts it headless, and asserts three things a CPU runner cannot: that a
GGUF is actually offloaded to the GPU rather than falling back to CPU and
returning text anyway, that a LoRA training run completes and leaves an
adapter on disk, and that a GGUF export against a CUDA llama.cpp produces a
file that loads. The repo's existing playwright_chat_ui.py then drives the
browser path, last, because it stops the server on its way out.
The gate and the launcher are the notebook leg's, reused unchanged: the
payload prints the same report line, so the transport needed no edit. The
two legs share a concurrency group because Kaggle's 2-kernel cap is per
account and is not workflow-aware.
Sampled at 4 percent of a filter measured at ~760 eligible invocations a
week, which is ~30 launches and ~23 GPU-h against the notebook leg's 15-20
and 1.4. The rate is lower and the spend is fifteen times higher because
the eligible stream is five times larger and a launch costs nine times as
much; the arithmetic is in the workflow header.
* Budget the two Kaggle GPU legs against one account, not two
The Studio leg's budget block was solved against the notebook leg's numbers
from before that leg was re-tuned: 0.08 h per launch, 10 percent, ~1.4
GPU-h/week. The notebook leg now costs 0.25 h at 40 percent for up to 23.1
GPU-h in a busy week, and Studio was sized at ~23 of its own. Two legs at 23
each is 46 of the 50 hours CI is allowed on a 60 h account, with the entire
margin spent on the cheaper one, and the two reserve floors were picked
independently.
Re-solve both against one shared allowance: Studio 35 h, notebooks 15 h.
Studio goes to 5 percent (~38 launches, ~28 GPU-h) and notebooks to 15
percent (13-35 launches, 3.3-8.7 GPU-h). The reserves now encode priority
rather than each leg's own ceiling: notebooks stand down once the account is
35 h spent, Studio runs on to 50.
The percentage is the one axis where Studio reads lower, and it cannot be
made to agree. Studio's eligible stream is ~3.3x larger and each launch
costs 3x more, so a point of rate is worth about ten times as much there;
matching the notebook leg's 15 percent would be ~86 GPU-h a week. Both
budget blocks now say so, and a test re-derives the joint spend from the two
headers instead of trusting either total.
Also fix a test that had gone vacuous: pre-commit.ci reformatting
default=10 to default = 10 stopped the payload regex matching, so the step
count agreement check was comparing nothing. Match count is asserted now.
* [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
* Write down where the Kaggle sampling percentages come from
The percentages in the two workflows were arithmetic nobody could check. This
records the measurements they came from, taken 2026-08-11 over a 28-day
window: 985 commits to main, 1157 PRs, and 8 of 60 sampled PRs touching the
paths a T4 training run could regress, so a 13% path-filter rate and roughly
75 candidate events per week.
Against 30 GPU-h/week for one account at ~0.75 GPU-h per launch, the ceiling
is about 33 launches per week if everything went to one workflow. The 15/5
split and the 25/10 reserve-hour floors follow from that, and the file says
what would change them: a second account, a wider path filter, or a session
cost above ~1 GPU-h.
* Give the GRPO leg its own kernel: pairing it with gpt-oss fails
Kernel unsloth-t4-ci-70a2f4eb ran the wired pair on the two cards of one
session. gptoss passed: 32 unique graphs, 779 calls captured, 2 graph breaks,
peak 12.78GB. grpo failed with the illegal memory access again, at exactly
the 13.60GB peak at which it PASSED alone on kernel unsloth-t4-ci-53efcc4e,
same config to the flag.
Same peak, different outcome, so the constraint is not GPU memory: the driver
pins one card per payload. gpt-oss offloads to host RAM and vLLM wants host
RAM, and a Kaggle session has one host.
A third kernel costs a queue slot rather than quota, since Kaggle runs two at
a time and bills wall clock per session. That is cheaper than a leg which
fails half the time for a reason the report cannot show.
Two tests pin the rule in both directions: grpo must not share a kernel with
gptoss, and control must still share one with canary, because those two are a
matched pair whose comparison is the whole instrument.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Unwire the GRPO leg: running it alone reproduced the failure I blamed on pairing
I gave grpo a kernel of its own earlier today on the reasoning that sharing a
session with gpt-oss was what broke it. It had failed paired
(unsloth-t4-ci-70a2f4eb) and passed alone (unsloth-t4-ci-53efcc4e), so pairing
looked like the variable, and I wrote two tests pinning the separation.
That reasoning was wrong. Running the leg ALONE again
(unsloth-t4-ci-c98f14be, built from this branch with --legs grpo) reproduced
the paired failure exactly:
unsloth_zoo/vllm_utils.py:601 sleep() -> torch.cuda.empty_cache()
torch.AcceleratorError: CUDA error: an illegal memory access was encountered
engine_built false, peak 13.8GB/13.6GB of 14.56GB. The passing session and both
failing ones agree to the digit on every recorded version -- torch 2.10.0+cu128,
transformers 5.5.0, trl 0.24.0, peft 0.19.1, vllm 0.19.1, unsloth 2026.8.15,
zoo 2026.8.10 -- on the same peak, with UNSLOTH_VLLM_STANDBY=1 throughout. One
contrasting observation was never enough to blame a shared host.
So what three sessions show is an intermittent illegal memory access on Turing,
one pass in three, not a pairing effect. A leg that passes one session in three
cannot tell CI anything: it would go red for a reason no reader could act on.
grpo moves to UNWIRED with the three session ids and the open question written
down, rather than back into a kernel.
The note keeps what the leg already earned -- the vLLM/torch pin, the attention
backend, the libcuda shim, the chat template, the T4-sized config -- so
re-wiring means answering the IMA rather than redoing the install.
test_grpo_does_not_share_a_session_with_gptoss is replaced rather than deleted:
it asserted the opposite thing for the disproven reason, and would now pass
vacuously since grpo is in no kernel at all. The replacement pins that grpo
stays unwired and that the note still names all three sessions, so the evidence
cannot be dropped on the way back in.
145 passed.
* Retire the bootstrap password at login, so the session can reach the routes
First hardware run of this payload, Kaggle kernel unsloth-t4-ci-412345d2 on two
Tesla T4s: preflight, studio_ready, authenticate and the Playwright chat_ui
driver all passed, and the four assertions the payload actually exists for did
not run.
gpu_inference: POST /api/inference/load -> HTTP 403: {"detail": "Password change required"}
lora_training: POST /api/train/start -> HTTP 403: {"detail": "Password change required"}
A bootstrap account is created with must_change_password set, and
get_current_subject turns that into a 403 on every route except the
password-change one. So login yielded a token that authenticated and could do
nothing, and the login step reported success while inference, tool calling,
training and export were all unmeasured behind it.
login now completes the change when Studio's own Token says
must_change_password, and carries the token that /api/auth/change-password
mints. The replacement is secrets.token_urlsafe(24): never whitespace, which
the route rejects, and never equal to the bootstrap value, which it also
rejects. It is not returned or stored -- nothing needs it again.
Four tests, all off-box against a scripted HTTP layer:
the change happens, and the session carries the post-change token
an account already past the gate does NOT get its password rotated
a refused change raises instead of carrying on with a token that cannot act
a change that answers without a token raises
The third is the one that matters: continuing past a failed change is how the
first run produced four unmeasured assertions and no explanation.
Two things this does NOT fix, recorded so the next run is read correctly.
gguf_export reported install_kind=None, i.e. no llama.cpp install under
STUDIO_HOME at all. That may well be downstream of the 403 rather than
independent: POST /api/inference/load is what pulls a llama.cpp for a GGUF
model, and it never ran. The collected kernel.log is 223 lines and holds none
of the install output, so this is not settled either way -- the re-run answers
it.
gpu_inference also failed its own evidence test: text came back, but no probe
could show the GPU was used. The payload calls that a failure rather than a
pass, which is the behaviour I want kept.
89 passed.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Hand the UI driver the password the session actually holds
Second hardware run, kernel unsloth-t4-ci-9ddd8ae4. Retiring the bootstrap
password fixed three assertions and broke a fourth:
gpu_inference FAIL -> PASS
tool_calling FAIL -> PASS
lora_training FAIL -> PASS
chat_ui_driver PASS -> FAIL: "the bootstrap password is gone, so the driver
cannot log in"
My own doing. assert_chat_ui read $STUDIO_HOME/auth/.bootstrap_password off
disk, and authenticate() has to retire that value to get past Studio's forced
password change, so the file is stale by the time the driver runs. The driver's
first phase rotates the password itself and asserts the old one stops working,
so it needs the value the session is CURRENTLY authenticated by -- not the
seeded one.
login now leaves that value on Studio.password and assert_chat_ui passes it,
falling back to the seeded file only when no login has happened. The commit
that introduced the retirement said the replacement was "never returned or
stored: nothing needs it again". That was wrong, and the docstring now says so
and says why.
Studio.password is a credential held for the run, unlike anything else in that
module, so the module docstring records it and assert_chat_ui adds it to the
scrub set before it can reach a log.
test_the_session_remembers_which_password_is_current pins both directions: the
retired session reports the replacement it minted, and a session that never had
to change reports the password it logged in with.
Still failing, and NOT downstream of the 403 after all: gguf_export reports
install_kind=None on a run where inference, tool calling and training all
passed, so no llama.cpp is installed under STUDIO_HOME regardless of the
auth gate. I said last time that this might resolve itself once the 403 was
fixed. It did not. It is a separate gap and it stays open.
90 passed.
* Record what launch blocking did to the GRPO leg: it removed the fault
The UNWIRED note asked for one experiment: run the leg with
--cuda-launch-blocking so the illegal memory access reports at its real call
site instead of surfacing at torch.cuda.empty_cache(). Done, kernel
unsloth-t4-ci-b1f23e34, and the answer is not the one the note expected.
With blocking on there was no illegal memory access at all. engine_built true,
three steps, same 13.8GB peak, same versions to the digit. A fault that
disappears when the launches are serialised is a race, which is what one pass
in four already suggested and which no amount of re-running the leg will pin
down.
The run also exposed a SECOND problem, and it is separate from the first. It
failed on reward_std = [0.0, 0.0, 0.0] with grad_norm 0.0 at every step. The
completions in the report are coherent prose, so this is not the model
collapsing into degenerate output. It is two completions scoring identically.
The leg runs num_generations = 2 because that is what fits on a 14.56GB card,
and at two samples a tie on a coarse reward is ordinary rather than a bug. So
the leg's pass criterion is fragile at the size it has to be to fit, which is
worth knowing before anyone reads a red grpo leg as a regression.
The note now records both, says which question each answered, and states the
two that remain open: where the race is, and what pass criterion is honest at
num_generations = 2. The test gains the fourth session id and a check that the
note no longer reads as though launch blocking were still the next thing to
try -- a stale "try this next" is how an experiment gets run twice.
145 passed.
* Re-seed the account before the UI driver, because the two want opposite states
Third hardware run. The API assertions all pass now:
preflight, studio_ready, authenticate, gpu_inference, tool_calling,
lora_training -- all PASS
and the Playwright driver still fails, for a third and final reason in this
chain:
[ui] STEP change-password through UI (Setup your account)
[ui] change-password form attempt 1 failed: Timeout 60000ms exceeded
waiting for locator("#new-password"); page.url=.../login
The driver's first UI step drives the forced-change form. authenticate() has
to retire that same bootstrap password over the API to reach /api/inference/load
and /api/train/start at all. So by the time the driver runs, Studio shows an
ordinary login and #new-password does not exist.
That is not a value problem, it is a state problem, and the previous commit
only fixed the value. The API path wants the change DONE; the driver wants it
PENDING. One account cannot be in both states, and three runs walked the whole
chain to get here: 412345d2 failed the API assertions on the gate, 9ddd8ae4
fixed those and failed the driver on a stale password, this one fixed the
password and failed the driver on the form being gone.
assert_chat_ui now restarts Studio before handing over. start_server() removes
$STUDIO_HOME/auth, which is what re-seeds the bootstrap password, so the driver
gets exactly the state its first step expects. It is cheap here specifically:
this assertion runs last and stops the server anyway, so nothing after it needs
the API session, and the driver loads its own model through the UI.
test_the_ui_driver_gets_a_freshly_seeded_account pins the restart, that the
password is read AFTER it, and that the retired session's password is not what
gets handed over -- the restart is the whole fix, and a refactor that drops it
puts the payload straight back to a driver that cannot find the form.
Unchanged and still open: gguf_export reports install_kind=None. Three runs
now, one of them with inference, tool calling and training all green, so no
llama.cpp is installed under STUDIO_HOME and this is not downstream of the auth
gate.
91 passed.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Test the newest transformers and trl, not the newest zoo permits
The canary leg is documented as "the newest permitted library set" and that
is exactly what it installs: it resolves WITH unsloth_zoo in the resolution,
so zoo's own metadata is a ceiling. Measured against the two runs that
carried it:
canary transformers 5.5.0 trl 0.24.0 peft 0.20.0 accelerate 1.14.0
control transformers 5.5.0 trl 0.24.0 peft 0.19.1 accelerate 1.13.0
PyPI the same day: transformers 5.15.0, trl 1.9.2, peft 0.20.0, accelerate
1.14.0. The canary moved peft and accelerate to genuine latest and moved
transformers and trl not at all, which is what made it look like it was
working. zoo pins transformers <=5.5.0 and trl <=0.24.0, ten minors and a
whole major behind, so this CI could not detect a transformers 5.6+ or trl
1.x regression: it never installed one.
The new frontier leg upgrades transformers and trl with their dependencies
and without zoo in the resolution, which clears the cap because pip enforces
only the requirements of packages in the resolution. It runs the same SFT
payload as control and canary, so all three are directly comparable.
I expected it red. It is green. On a real T4: transformers 5.15.0, trl 1.9.2,
datasets 5.0.1, ten steps, canary emitted, and two fresh processes agreeing
bitwise. Unsloth trains and generates correctly a whole trl major above what
zoo's metadata permits.
Getting there took two kernels. The first used --no-deps plus a blanket
tokenizers upgrade and died before running anything, because an unbounded
upgrade overshoots the ceiling transformers declares:
tokenizers<=0.23.0,>=0.22.0 is required, but found tokenizers==0.23.1
safetensors>=0.8.0 is required, but found safetensors==0.7.0
A test pins that shape out, along with the zoo-in-the-resolution mistake that
would silently turn this into a second canary.
It goes on the second kernel's idle T4, so it costs no quota: a Kaggle
session bills wall clock once, not per card.
What it does NOT catch is recorded in the leg: the loss trajectory is not the
control's, and step 1 is computed before any update on identical weights,
data and seed, so 10.3222 against 6.4367 is a different loss function rather
than drift. Both converge. Which objective is intended is separate work.
* Install a CUDA llama.cpp before asserting the export used one
Four hardware runs reported llama_cpp_install_kind=None and failed the GGUF
export assertion for it. The reason was never subtle: nothing had ever
installed a llama.cpp under STUDIO_HOME, so the assertion was measuring an
absence rather than a selection. install_llama_prebuilt.py resolves a real
"linux-cuda" kind on an x64 CUDA host, so the bundle was available the whole
time and simply never fetched.
The install runs before the server, so the export route sees a llama.cpp that
was already there rather than one appearing underneath it, and it is its own
assertion because "the CUDA bundle would not install on this box" and "it
installed and the export against it failed" are different findings.
Its result is deliberately not checked by execute(). A box where the bundle
cannot be installed still runs inference, tool calling, training and the UI,
and still reports the export failing for the reason it actually failed.
Three tests. One of them CALLS install_llama_cpp rather than reading its
source, because run() in this payload already applies capture_output and text
and passing either again is a TypeError that no source-reading test would
catch -- which is how the first draft of this was written wrong.
* Record what install.sh left behind before replacing it
build_kernel.py runs install.sh --local, whose own comment says it puts a
llama.cpp on disk. Four runs still reported install_kind=None, so either it
never installed one or it installed something else. Fetching a CUDA bundle
fixes both cases and would hide which one it was, so the prior state is
recorded first and the original question stays answerable.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Look for llama.cpp where it is, not where the payload assumed
Five hardware runs reported llama_cpp_install_kind=None and failed the GGUF
export for it. There was no missing llama.cpp. install.sh --local installs one
into ~/.unsloth/llama.cpp, which is what install_llama_prebuilt.py's own
default resolves to, and this payload read STUDIO_HOME/llama.cpp, a path
nothing ever wrote to.
Adding an explicit install last commit is what proved it, because it recorded
what was there first. The installer selected the right bundle for a T4
accept ...linux-x64-cuda13-older.tar.gz coverage=75-89 supported=75,80,86,89
reject ...linux-x64-cuda13-newer.tar.gz reasons=missing_sms=75 out_of_range_sms=75
and then said
existing llama.cpp install already matches selected release
b10360-mix-87da1a2; skipping download and install
while install_kind_before and install_kind_after at the requested directory
were both None and the return code was 0. So a CUDA llama.cpp was present the
whole time, the sm_75 selection works, and only the assertion was wrong.
llama_cpp_marker checks STUDIO_HOME first, then the canonical location, and
returns None when neither has a marker so a real absence stays a real absence.
Against the exact situation the last run recorded, the old read answers None
and the new one answers linux-cuda.
Separately, and not fixed here: install_llama_prebuilt.py --install-dir X
exits 0 having written nothing to X whenever the canonical directory already
holds a matching release. A caller who names a directory and gets a success
with an empty directory has been told something untrue.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Read the field the marker has, not the one it never had
Six hardware runs reported llama_cpp_install_kind=None and failed the GGUF
export assertion for it. There was no missing llama.cpp and no wrong
directory. install_llama_prebuilt.py writes install_kind ONLY into the JSON
its resolver prints to stdout. The marker it writes to disk records
requested_tag tag release_tag published_repo asset force_cpu llama_backend
asset_sha256 source_* ggml_tree bundle_profile runtime_line coverage_class
install_fingerprint prebuilt_fallback_used installed_at_utc
and no install_kind at any point. So reading install_kind from the marker
answered None for every bundle on every box, including a working CUDA one,
and the assertion turned that into a failure. The last run had the installer
saying "existing llama.cpp install already matches selected release
b10360-mix-87da1a2; skipping download and install" while the payload reported
nothing installed.
runtime_line is the field that answers the question: cuda12 / cuda13 for the
CUDA bundles, and the backend name otherwise. It is written into the same
dict as the asset, so the two cannot disagree, and asset is kept as a fallback
for a marker too old to carry it.
is_cuda_install now matches the runtime line's shape rather than a fixed set
of names. A fixed set needs a new entry per CUDA major and fails CLOSED when
it lacks one, reporting a working cuda14 install as not-CUDA, which is the
same failure this commit is undoing.
The existing test wrote {"install_kind": "linux-cuda"} into a marker and
asserted it came back. That is how this survived: the test agreed with the
payload about a field neither read from reality, and stayed green through all
six runs. It is rewritten against a marker the installer actually produces.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Measure the load, not the unload it follows
The GGUF export probe reported device_vram_delta_mib: -1866.0 and was scored
as a load that never reached the GPU. It had not: a 3004 MiB chat model was
evicted inside the same window the 531 MB GGUF loaded in, because POST /load
with force unloads first and the baseline was sampled before the request.
Unload explicitly, wait for the reading to stop falling, then take the
baseline. Best-effort throughout, since the delta is evidence and not the
assertion: a refused unload or a missing nvidia-smi leaves the probe running.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Read the status fields the response actually declares
Run 8 shipped the unload-before-baseline fix and came back with
device_vram_delta_mib: -1866.0, byte-identical to run 7. The fix ran and did
nothing: it looked for model_path, model and active_model_name, and
InferenceStatusResponse carries none of those, so no model was ever
identified and no unload was ever sent.
Read model_identifier (documented as the loadable identifier, which is what
/unload's model_path wants), falling back to active_model and then to
loaded[0]. A new test parses the field names out of InferenceStatusResponse
and asserts the payload only reads names it declares, so the next guess fails
in the suite rather than forty minutes into a kernel.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Delete the Kaggle kernel when the launcher is killed, not only when it returns
release() is the budget control: a kernel left behind bills to its own
ceiling with nobody reading the result, and that ceiling has been observed
not to stop a wedged one. But it was reachable only from finish(), which runs
only on paths that RETURN. KeyboardInterrupt was explicitly re-raised past
it, and the default SIGTERM disposition exits without running atexit or
finally. So Ctrl-C, `kill`, and a cancelled GitHub Actions workflow all left
the kernel running. The file's own comment claimed the kernels were released
"on this path and on every other", which was true for returns and false for
signals.
Four paths, four covers:
normal return / handled error finish()
unhandled exception atexit
Ctrl-C, kill, Actions cancel SIGINT/SIGTERM/SIGHUP handlers
kill -9 the orphan sweep at the next launch
The signal handler re-raises through SIG_DFL so the exit status still reads
"killed by signal N"; exiting 0 would make a cancelled job look completed.
kill -9 cannot be caught, so a pushed slug is now recorded to
logs/kaggle_inflight.json the instant it exists, and every launch sweeps
entries whose owning pid is gone. Keyed on pid deliberately: an entry whose
owner is still alive belongs to a run in progress, and deleting it would
destroy a concurrent run and report the absence as a failure of the code
under test. A delete that fails keeps its entry rather than forgetting it.
The sweep runs before pushing, so reclaimed session slots are available to
the run doing the reclaiming.
Tests drive real subprocesses and real signals, since the property is about
process death. Verified failing against the previous behaviour: SIGINT,
SIGTERM and the unhandled-exception case all leave the kernel behind without
these handlers.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Only forget a Kaggle kernel once it is actually gone
kaggle kernels delete answers a transient rejection with a nonzero exit and
subprocess.run does not raise on that, so both cleanup paths recorded a
refused delete as done: sweep_orphans dropped the registry entry and
release_kernels set released=True, leaving a running kernel billing with
nothing left to reclaim it. Check the return code in both.
Two more holes in the same registry. result["kernels"] was published after
the whole push loop, so a SIGTERM once the first kernel was up found no list
and deleted nothing; it is now the same list object, assigned before the
loop. And --keep-kernel left an entry naming a pid that dies with the
launcher, so the next sweep called the retained kernel an orphan and deleted
exactly what the flag asked to keep.
* Stop the Studio leg passing on evidence it never had
Six ways this job could go green without measuring anything.
The evidence bundle was never unpacked: the launcher writes each kernel into
kaggle_evidence/<slug>/ and the workflow hands collect_evidence.py the
parent, so a top-level glob found nothing and every run reported that the
payload emitted no bundle. Recurse, the way extract_reports already does.
An install.sh regression read as infrastructure. sh() raised SystemExit, so
papermill stopped before any T4_SMOKE_REPORT existed, the launcher filed the
run as infra and the reporter exited 0 -- silently passing the installer
failures this workflow's path filter selects for. Failures of the checkout
under test now emit a failing report first; a missing GPU or a clone that
would not download keep the no-report infra path. A payload killed by the
driver deadline does the same, but only once the payload itself had started.
The process-level VRAM probe could never fire: InferenceStatusResponse
declares neither llama_server_pid nor pid, and FastAPI drops what the
response model does not declare, so the lookup was always None. Find the
llama-server processes in /proc instead.
Offload evidence outlived its load. offloaded_layers() takes the last match
in the whole log, so a reload that logged nothing inherited the chat model's
line and passed on evidence from a different model. Read each load forward
from a mark taken just before it.
A diverged run counted as a trained one. A T4 has no bf16, and an fp16 run
that goes NaN still reaches completed and still saves an adapter, while
trained_steps counted anything that was not None. Only finite losses count,
and non-finite ones are named as a failure.
An export slower than its HTTP timeout crashed the payload. The route blocks
for the whole export and the transport timeout is 900s against a 1200s
deadline, so the TimeoutError escaped the StudioError handler; it is now
treated as in-flight and the status poll decides.
Alongside: the restart that re-seeds the account no longer truncates the
backend log of every earlier assertion, the over-cap evidence fallback ships
the logs it says it ships rather than the report alone, the result line is
printed after the evidence is packaged so a packaging crash cannot publish a
pass, the training and export polls stop when Studio dies, Studio is launched
from the interpreter running the payload rather than whatever is on PATH,
the gate is told this leg pushes one kernel rather than the default two, and
the advertised opt-in label can start the workflow.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Name the encoding on every checked-in read, and survive a box with no nvidia-smi
tests/test_source_read_encoding.py fails on 33 read_text() calls in the two
Kaggle harnesses: unpinned text I/O on a repo file is cp1252 on a stock
Windows install and dies the moment that file gains a non-ASCII byte.
The second one is the payload's own. environment() runs inside finish(), on
every path out of the run, so a session Kaggle handed no driver raised
FileNotFoundError from gpu_inventory() and ended with a traceback and no
report at all. run() now answers a missing binary the way a failing one does,
and the preflight says there is no GPU.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Spend a Kaggle session on the opt-in label, not on every label after it
Two ways this leg spends or loses a GPU session it should not.
GitHub has no per-label filter on the pull_request event, so adding
`labeled` to the activity types starts the workflow for EVERY label added
to an affected pull request. The gate is handed the whole label list, so
once kaggle-studio-gpu-ci sits on the pull request it reads every one of
those events as forced, and a routine label change launches another
70-minute Kaggle run. Guard the gate job on the label that fired the
event; push and the other pull_request activities carry no
`github.event.label` and fall through unchanged.
The other way is the reverse: a run that should be red exits 0. The
dependency probe raises out of the cell when the Studio venv reports no
CUDA, and with no T4_SMOKE_REPORT the shared launcher files the run as
`infra`. install.sh --local resolving a CPU-only torch is exactly the
CUDA install regression this workflow's path filter selects for, and it
took that path. Ask nvidia-smi which case it is: a GPU the host can see
and the venv cannot use is a payload failure, and no GPU at all stays
infra.
* Treat a stalled kaggle push as transport, not as a red verdict
Every other Kaggle transport failure -- a 503, a refused push, a capacity
rejection -- returns a reason from push() and exits 0 as infra. A push that
stalls past the 600s ceiling did not: TimeoutExpired escaped launch.py, so the
process ended before finish() could write launch_result.json, the launch step
exited nonzero and GitHub marked the job red. The reporter that reads that file
and calls the run NOT RUN never got a say, and a red there says the code under
test failed when nothing ever reached a T4.
Catch it in push() and report push_timeout.
* Do not let a truncated copy of a chunk overwrite the complete one
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Name the encoding on every checked-in file the T4 harness reads
Repo tests (CPU) is red on this branch:
test_source_read_encoding.py::test_checked_in_file_reads_name_an_encoding
AssertionError: 17 file reads in the test trees touch a checked-in file
with the platform default encoding, so they break on Windows as soon as
that file gains a non-ASCII byte.
All 17 are in tests/kaggle/test_t4_smoke_harness.py, which this branch adds,
so this is ours and not inherited. The reads pull in the workflow YAML, the
smoke scripts, the committed reference JSON and ci-preempt.json, and none of
them said what encoding to read them as. On Linux that resolves to UTF-8 and
nobody notices; on a Windows runner it resolves to the ANSI code page and the
first non-ASCII byte in any of those files takes the test out with a
UnicodeDecodeError that has nothing to do with what the test is checking.
Every one now passes encoding = "utf-8", matching how the rest of the suite
spells it. Reads of files the test itself just wrote under tmp_path are left
alone: the gate only covers checked-in files, and those are ours end to end.
test_source_read_encoding.py 1 passed
tests/kaggle/test_t4_smoke_harness.py 148 passed
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Flatten Kaggle's kernel log before extracting the reports from it
Kaggle's kernels/output returns the log as a JSON array of {stream_name,
time, data} records, one record per line, so no line in the file starts
with T4_SMOKE_REPORT. extract_reports read it verbatim, so the log
fallback -- the one that covers a run whose executed notebook never came
back -- recovered nothing and a failed assertion was scored as infra with
the workflow exiting 0.
report.py::kernel_log_text and collect_evidence.py::iter_text already
flatten the records; extract_reports was the reader that did not. The
records are also cut by write rather than by line, so the join has to
happen before any line splitting.
* Keep the slug a timed out kaggle push may have created
A push whose CLI stalls past the 600s ceiling says nothing about whether
Kaggle took it. Kaggle can accept the push and start the kernel billing
before the response is lost, and the branch returned infra while keeping
neither the slug nor an inflight entry, so finish() had nothing to delete
and the next launcher's orphan sweep had nothing to see. The kernel ran
to its ceiling with nobody reading the result.
The slug is ours and already decided at that point, so record it in the
inflight registry and hand it back as orphan_slug: release_kernels
deletes it on the way out, and a launcher killed first leaves the entry
for the next sweep. It is deliberately not returned as slug, because the
caller must not wait on a kernel that may never have been created.
* Align the Kaggle budget guide with the workflow it documents
The guide named --percent 15 and --reserve-hours 25 against a 30h account;
the workflow runs 40 and 20 against the 60h figure measured on 2026-08-11,
and it is the only consumer of that quota today. Following the guide would
have tuned the reserve from a baseline that no longer exists.
Re-derived from the same live numbers as the workflow header, and says which
of the two is the source of truth when they disagree.
* Stand the gate down on any unreadable kernel status
The concurrency verdict refused only when EVERY in-window status came back
an error. One unreadable status among readable ones was treated as an idle
account, and driving concurrency_verdict with
{busy: [], own: [], foreign: [], surveyed: 2, unreadable: 1, complete: True}
returned (True, ""). That kernel can be the human session the zero-foreign
policy exists to yield to, and proceeding takes the account's last slot: the
first push wins it, the second comes back at capacity, and the run reports a
subset of its legs.
A 404 is kept separate and stays benign. A deleted kernel is not an unknown
state and the launcher deletes every kernel it pushes, so blocking on that
would wedge the gate shut rather than make it careful.
* Close six gaps in the Kaggle T4 notebook CI workflow
- The pull_request trigger used GitHub's default activity types, which are
opened, synchronize and reopened only. The kaggle-t4-ci label the gate
advertises as a human override could therefore never start a run on its
own. Subscribe to labeled as well.
- pyproject.toml was in neither paths filter, though every payload installs
the commit under test as a distribution built from it. BUDGET.md counted
it as a watched path already.
- The job deadline of 120 minutes was shorter than the launcher's worst
case. Two sequential pushes can take 2 x (4 x 600s + 315s of backoff) and
the 5400s polling window only starts after them, so about 187 minutes with
the deletions; a runner killed at 120 takes finish() -> release() with it
and leaves pushed kernels billing quota. 200, with the arithmetic written
down beside it.
- The gate surveys quota and in-flight kernels before t4-smoke queues on the
account-wide concurrency group, so a second sampled run could launch on an
approval a full run old. Re-ask with the slot in hand, immediately before
the push; --force true skips the sampling draw and nothing else.
- --zoo-ref main let each payload's own pip resolve a different unsloth-zoo
commit, including between the control and the canary within one session,
and zoo is not in pins/control.txt either. Resolve it once with git
ls-remote and pass that SHA to every leg.
- A dispatch with max_steps != 10 still shipped the 10-step reference, and
check_reference reports step_count_mismatch as a failure, so the run could
only ever go red on arithmetic. Drop the band automatically when the counts
differ, and say so.
Also: check out the same head SHA the launcher installs, so the harness and
the package under test are one snapshot rather than the merge tree and the
head; and run tests/kaggle/test_t4_smoke_harness.py on the runner before
anything is built or pushed, since testpaths limits pytest to tests/security
and nothing else collected it.
* Read back what the Kaggle kernel actually reported
Two ways a real payload verdict was being lost in transport, both ending in
verdict=infra and a green workflow.
The kernel log fallback could never work. Kaggle's kernels/output hands the
log over as a JSON array of {stream_name, time, data} records, which is why
report.kernel_log_text flattens it before reading; extract_reports scanned
the file as text, so no line began with the report prefix and the fallback
that exists for the run whose notebook never came back found nothing. Record
boundaries are not line boundaries either, so the records are joined before
the scan rather than read one at a time.
push() documented a fresh slug per attempt and used one for all four. Pushing
to an id that already exists does not replace it: Kaggle files a new version
and starts a second batch session, and kernels status and kernels/output send
no version label, so they answer for the newest session only. A retry after a
lost response therefore collected the wrong execution's evidence while the
first kept a session slot and billed unseen. Each attempt now takes its own
slug and discards the previous one first, and every slug filed is returned
and recorded, since a push that reported an error may still have landed.
* Give each payload its own everything, and report a probe failure
Four ways the two payloads of one kernel were not actually isolated, plus
the import probe exiting without a verdict.
The probe was the P1. When the commit under test breaks import unsloth, the
verify cell raises before the run cell can write a report, the launcher
extracts none, and no reports is classified as infra -- so a deterministic
import regression exits green. A probe failure is a verdict, not missing
evidence, so it now emits its own failing report.
nvidia-smi failing or a one-GPU allocation fell back to N_GPU = 1 and pinned
both payloads to device 0. Each child still saw exactly one card and passed
its own visibility assertion, so the contention came back looking like a code
failure. A shortfall is infrastructure and stands the kernel down before a
thread starts, which is what the comment above it already claimed.
A payload whose virtualenv could not be built kept the base python3 kernel
and installed into the shared system site-packages. That is the one thing the
per-child venv exists to prevent: the legs install deliberately different
library sets, so the last writer wins and the resulting import error reads as
a regression. The payload is skipped instead.
Both payloads materialised into one source directory and compiled into one
unsloth_compiled_cache. The sources are byte-identical copies but write_bytes
truncates first, and the compile cache is a relative path resolved against a
working directory both papermill children inherit while compiling the same
modules against different transformers versions. Each payload now owns both,
and the prune globs still reach them.
Finally, --smoke-args is appended after every leg's own arguments, so the
workflow's --max-steps 10 landed after the gpt-oss leg's --max-steps 3 and
argparse took the last one. The 20B leg was training for ten steps rather
than the three that were measured to fit. A shared argument no longer reaches
a leg that sets that option itself.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
---------
Co-authored-by: danielhanchen <unslothshared@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|