Run the repo CPU test suite on all four runner cores by danielhanchen · Pull Request #9019 · unslothai/unsloth · GitHub
Skip to content

Run the repo CPU test suite on all four runner cores - #9019

Merged
danielhanchen merged 18 commits into
mainfrom
ci-parallelise-backend-suite
Aug 17, 2026
Merged

Run the repo CPU test suite on all four runner cores#9019
danielhanchen merged 18 commits into
mainfrom
ci-parallelise-backend-suite

Conversation

@danielhanchen

@danielhanchen danielhanchen commented Aug 16, 2026

Copy link
Copy Markdown
Member

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-latest has 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:

step time result
serial 17.75 min 1 failed
-n 4 7.67 min 1 failed

The 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 subtests result.

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.py module 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 3.10 3.11 3.12 3.13
test_safetensors_reasoning_stream.py (3 tests) X X X X
test_streaming_stripper.py::test_early_markup_is_not_slower... X X X
test_transformers_version.py::TestProbeGating (2 tests) X X X
test_stt_transcription_cancellation.py::test_disconnected_raw... X X
test_diffusion_backend.py::test_begin_load_never_refuses_auto X

My 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 load and --dist loadfile each 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:

test_early_markup_is_not_slower_than_the_code_it_replaces asserts 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: 30 comment 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.

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
@danielhanchen

Copy link
Copy Markdown
Member Author

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 -n 4 gave identical 38-failed / 26152-passed sets twice over. On a runner the timing win is real (the Backend tests step goes 22.5-27.8 min to 7:41) but so is a failure set that serial does not produce:

test 3.10 3.11 3.12 3.13
test_safetensors_reasoning_stream.py (3 tests) X X X X
test_streaming_stripper.py::test_early_markup_is_not_slower... X X X
test_transformers_version.py::TestProbeGating (2 tests) X X X
test_stt_transcription_cancellation.py::test_disconnected_raw... X X
test_diffusion_backend.py::test_begin_load_never_refuses_auto X

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:

  • test_early_markup_is_not_slower_than_the_code_it_replaces asserts a wall-clock ratio with a 10% margin. Four workers on four vCPUs is exactly the condition that breaks it. Genuinely incompatible with running alongside anything.
  • test_safetensors_reasoning_stream fails at pytest.importorskip("core.inference.inference") with PackageNotFoundError: No package metadata was found for unsloth_zoo, which importorskip does not treat as importable-or-skip. The backend job never installs unsloth_zoo, so these only pass when something earlier in the same process already put that module in sys.modules. Same shape as the stt_unload bug this PR is stacked on.
  • The rest are still being separated from pre-existing failures. A serial run of the same tree in the same staging repo is in flight; test_disconnected_raw_transcription_cancels_its_sidecar on 3.12 in particular predates all of this.

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.
@danielhanchen
danielhanchen force-pushed the ci-parallelise-backend-suite branch from 9377f00 to d86109f Compare August 16, 2026 17:10
@danielhanchen danielhanchen changed the title Run the Backend CI suites on all four runner cores Run the repo CPU test suite on all four runner cores Aug 16, 2026
@danielhanchen
danielhanchen marked this pull request as ready for review August 16, 2026 17:10
danielhanchen and others added 2 commits August 17, 2026 01:21
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.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

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.
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: f0e3504334

ℹ️ 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".

chatgpt-codex-connector[bot]

This comment was marked as resolved.

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
chatgpt-codex-connector[bot]

This comment was marked as resolved.

danielhanchen and others added 2 commits August 17, 2026 03:03
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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 17, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 17, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 17, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 17, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

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".

danielhanchen and others added 3 commits August 17, 2026 03:12
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
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 6e09614fc4

ℹ️ 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".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +68 to +69

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@danielhanchen
danielhanchen deleted the branch main August 17, 2026 04:43
@danielhanchen danielhanchen reopened this Aug 17, 2026
@danielhanchen
danielhanchen changed the base branch from fix-stt-unload-no-backend to main August 17, 2026 04:44
@danielhanchen
danielhanchen merged commit 6c124b6 into main Aug 17, 2026
19 of 23 checks passed
@danielhanchen
danielhanchen deleted the ci-parallelise-backend-suite branch August 17, 2026 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant