Run the three loaded-models-indicator engines at once - #9158
Conversation
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.
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5469760d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
In this workflow, install.sh installs the managed environment under the default ~/.unsloth/studio, but this override makes each fresh directory the CLI's installation root, not merely a data root. unsloth_cli/commands/studio.py consequently checks for $UNSLOTH_STUDIO_HOME/unsloth_studio/bin/python; because none of these new directories contains that environment, every background launch exits with “Unsloth Studio not set up” before binding its port, so the cross-browser job always fails. Isolate the mutable auth/data without redirecting the CLI away from the installed Studio environment, or make the isolated roots reference that environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is exactly what staging showed: all three engines exited with "Unsloth Studio not set up. Run install.sh first." before binding a port. My premise that UNSLOTH_STUDIO_HOME selects a data root was wrong; _studio_venv_python() resolves $UNSLOTH_STUDIO_HOME/unsloth_studio/bin/python and _find_run_py() globs under the same directory. Fixed in 3e743f7: each per-engine home now symlinks the 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 exists first, so a missing install is one clear error instead of three misleading ones.
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.
for more information, see https://pre-commit.ci
|
@codex review |
1 participant
Loaded-models indicator (cross-browser)is the longest Linux job in CI, and almost all of it is one step waiting on itself.Measured over recent main runs, the job totals roughly 1000s, of which about 870s is this:
Three runs of the same suite, one after another. They are disjoint: each boots its own server and drives its own browser. Nothing about the work required them to be sequential, only shared state did.
What was shared
Two things, and each is now split rather than serialised.
The port. One per engine, so three servers coexist.
UNSLOTH_STUDIO_HOME. This is the reason it was not already parallel. The script does:On a shared home that is a destructive race: one engine's wipe lands between another's mint and its read. Running the current step body concurrently against a single home has two of the three engines read a password that a different engine minted, which would surface as an intermittent auth failure rather than anything pointing at the cause.
Why a per-engine home is cheap
UNSLOTH_STUDIO_HOMEselects a data root, not an install:unslothon PATH still resolves the installed venv, so there is no second install._DEFAULT_FRONTEND_PATHinstudio/backend/run.pyis<pkg>/../frontend/dist), not fromstudio_root(), so no per-engine frontend build.UNSLOTH_API_ONLYwith the four/statusendpoints stubbed viapage.route, so a fresh home needs no model, no GPU and no llama.cpp build.That last point is what limits this to this suite. A job whose home holds a downloaded model would pay for the fresh home three times over, so the guard test pins the API-only property the cheapness depends on.
Failure reporting
The step now waits on all three engines before failing, rather than stopping at the first. A sequential run reported only the first failing engine; a run where two engines regress now reports both. Each engine's output is echoed in its own log group, and the per-engine logs are added to the uploaded artifacts.
On the expected saving
The three engines now share one runner's cores, so this will not be a clean 3x. The step should go from about 870s to roughly the slowest single engine plus contention. The staging run on this branch is the measurement, not the estimate above.
Tests
tests/studio/test_indicator_browsers_run_in_parallel.pyasserts the properties that are silent when they break: the engines are backgrounded, all three still run, ports are distinct, the home varies by the same token the engine does, the script still honours the override it is given, a failing engine still fails the step, and all engines are waited on before the step gives up. It also pins the two facts the cheapness rests on, so that if either changes the next person re-measures rather than inheriting the assumption.test_the_linux_job_still_drives_all_three_browser_enginesmatched the literal...sh 18899 <engine>call form, which the loop no longer produces. It now asserts the property instead. Scoping matters there: matching engine names across the whole job would readplaywright install --with-deps chromium firefox webkitas coverage, so it is scoped to the steps that actually invoke the helper. Verified by dropping firefox and webkit in turn and confirming it goes red.