Run the repo CPU test suite on all four runner cores - #9019
Conversation
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
|
Marking this draft. Staging CI on real 4-vCPU runners does not agree with my local measurement, so the safety claim above is not yet earned. Local, on the same dependency shape the workflow installs, serial and
Why local missed it: my venv is missing several optional extras, so some of these were already in the 38 pre-existing failures and could not change state, and the machine has 192 cores, so four pytest workers never contend the way they do on a 4-vCPU runner. The causes are not all the same, and at least one is not an xdist artifact:
I will not weaken any of these assertions to get the number down. Back with the serial comparison. |
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.
9377f00 to
d86109f
Compare
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.
for more information, see https://pre-commit.ci
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.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
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
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
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e08e6d435c
ℹ️ 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".
| # under 0.1 minutes on the runner, so there is nothing to win and isolation to lose. | ||
| run: | | ||
| python -m pytest tests/ -q --tb=short \ | ||
| python -m pytest tests/ -q --tb=short -n 4 \ |
There was a problem hiding this comment.
Isolate the PowerShell probe scripts per worker
On the ubuntu-latest job, PowerShell is available and these tests do not skip; under xdist's default load distribution, the parametrized cases in tests/python/test_windows_setup_output_encoding.py can run on separate workers. _run_capturing_bytes() uses the same repository path for every case with a given stem/shape (:_setup_output_probe_0.ps1, etc., at lines 164-177), so one case can unlink the script after another has written it but before its pwsh child opens it, producing an intermittent file-not-found failure. Give each invocation a worker/UUID-specific script or keep this file out of the parallel command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and I reproduced it rather than reasoning about it. The probe path was _{stem}probe{shape}.ps1, which several tests share, and pwsh is installed on ubuntu-latest so these do not skip there. Reverting to the shared name and running -n 4 four times gives 1 failed every time. Each invocation now gets a uuid in the name: 35 passed and 8 skipped serial, the same on each of four -n 4 runs, and no probe scripts left behind. Third one of these on this PR, all the same shape, so I also swept the rest of the parallel selection for shared writable paths.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
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.
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
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eba1d102a
ℹ️ 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".
| # under 0.1 minutes on the runner, so there is nothing to win and isolation to lose. | ||
| run: | | ||
| python -m pytest tests/ -q --tb=short \ | ||
| python -m pytest tests/ -q --tb=short -n 4 \ |
There was a problem hiding this comment.
Isolate temporary requirement files from repository scans
When xdist schedules tests/python/test_no_torch_filtering.py::TestInstallPythonStackSubprocessMock alongside tests/studio/install/test_diffusers_pin.py::test_only_the_pin_file_names_diffusers, the former's no-torch cases temporarily create .diffusers-pin-filtered-*.txt beside the real requirements while the latter scans REQ_ROOT.rglob("*.txt"). Because filtering does not remove the Diffusers entries, the scanner reports that temporary file as a second source of the pin; this can be reproduced by leaving _filter_requirements(diffusers-pin.txt, NO_TORCH_SKIP_PACKAGES) alive during the scan. Keep these cases serial or make the scanner ignore generated filtered files so the new parallel CI command does not fail intermittently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and reproduced: calling _filter_requirements on diffusers-pin.txt and then running test_only_the_pin_file_names_diffusers fails exactly as you describe. It is not only an xdist problem either, since that function passes delete=False, so the file also survives a real install on any machine. I fixed the scanner rather than the production write: writing beside the source is deliberate, so relative -r and -c includes resolve. The scan now skips dotfiles, which is what it means (no CHECKED-IN requirements file other than the pin may name diffusers) rather than a narrowing: 5 passed with a filtered file present, and a probe file naming diffusers still fails it.
…n 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
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 249d405454
ℹ️ 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.
Restrict the exemption to generated filter files
If a checked-in hidden requirements file such as .constraints.txt names Diffusers, this blanket exemption skips it and the one-source-of-truth test passes even though a later install phase can override the pinned revision. Only files matching the generated .{stem}-filtered-*.txt shape should be ignored; other dotfiles under the requirements tree still need scanning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, the blanket dotfile skip was wider than the problem. A checked-in .constraints.txt is a real requirements file and a real place the pin could be overridden from. It now matches the exact shape _filter_requirements writes, a dot then the source stem then -filtered- then NamedTemporaryFile eight character suffix. Verified both ways: 5 passed with a generated filtered file present, and 1 failed with a checked-in .constraints.txt naming diffusers.
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

Backend CI is the most expensive workflow in the repo at 126.7 runner-min per push, out of ~575 across the 38 workflows that fire on
pull_request. Install is cached at ~1.2 min, so almost all of it is pytest.ubuntu-latesthas 4 vCPUs and this suite is CPU-bound and GPU-free, so it was using one core of four.This PR parallelises one of the two jobs:
Repo tests (CPU).Result on a real runner
Same tree, same staging repo, one run serial and one at
-n 4:-n 4The failure is the same one both ways (
test_multi_chat_prompt_queue_contract.py::test_composer_only_queues_behind_the_current_chat), pre-existing on this tree.Locally, on the dependency shape the workflow installs: 806.1s -> 219.7s with an identical
2 failed, 8066 passed, 178 skipped, 40 subtestsresult.Saves ~10 runner-min per push.
The three small pytest steps in this job stay serial on purpose. The hardware-spoof step exists precisely because those files mutate
hardware.pymodule globals, and all three already measure under 0.1 min, so there is nothing to win and isolation to lose.Why the four-interpreter matrix is not in here
It is the bigger prize, ~79 runner-min. An earlier revision of this PR did both, and I was wrong to.
Locally the matrix suite gave identical 38-failed / 26152-passed sets serial and
-n 4, twice. On a real 4-vCPU runner it did not:test_safetensors_reasoning_stream.py(3 tests)test_streaming_stripper.py::test_early_markup_is_not_slower...test_transformers_version.py::TestProbeGating(2 tests)test_stt_transcription_cancellation.py::test_disconnected_raw...test_diffusion_backend.py::test_begin_load_never_refuses_autoMy local evidence was not evidence. On a 192-core box four pytest workers never contend the way they do on four vCPUs, and my venv already had some of these failing, so they could not change state. Re-run pinned to four cores,
--dist loadand--dist loadfileeach produce a different extra failure set, and not the same one twice.So this is not a flag away. Each dependency has to be found and fixed. Two are done and neither is an xdist bug:
stt_unloadanswered 500 before any backend was resident. The two tests covering it fail standalone onmaintoday; they were green only because an earlier test left a backend loaded.test_safetensors_reasoning_stream.pynever stubsunslothand reachescore.inference.inferenceviaimportorskipinside test bodies, which the repo's own stub guard treats as lazy and skips. It passed by inheriting another file'ssys.modulesentry. Against the dependency set this workflow installs: 3 failed before, 13 passed after.test_early_markup_is_not_slower_than_the_code_it_replacesasserts a wall-clock ratio with a 10% margin, which four workers on four vCPUs breaks. That one is genuinely incompatible with running alongside anything, and I am not widening the margin to make it green.The
timeout-minutes: 30comment is corrected here too: it claimed the suite runs "14.1 to 15.0 minutes", and the 3.11 leg now measures 27.75, i.e. 2.25 minutes under a limit that would cancel it.