Run six pinned-symbol jobs on one runner instead of six by danielhanchen · Pull Request #9385 · unslothai/unsloth · GitHub
Skip to content

Run six pinned-symbol jobs on one runner instead of six - #9385

Merged
danielhanchen merged 2 commits into
mainfrom
version-compat-bundle
Aug 20, 2026
Merged

Run six pinned-symbol jobs on one runner instead of six#9385
danielhanchen merged 2 commits into
mainfrom
version-compat-bundle

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

Version Compat CI spent six runner slots on 287s of combined work. On a sampled main commit each of the six waited 23 to 31 minutes to be admitted, in order to execute for 15 to 128 seconds. The queue is the cost, so the slots are the target.

 15s bitsandbytes | 128s TRL/GRPO | 38s vLLM | 52s transformers
 21s sentence-transformers | 33s PEFT

Why these six can share a runner

Not a general claim about the suite, a specific one about these tests: they fetch raw source from raw.githubusercontent.com and grep it for symbols (tests/version_compat/_fetch.py, has_def / first_match). Nothing is pip-installed — that is the whole point of the canary, and it is also what makes co-location safe: there is no venv for "transformers 4.57.6" and "transformers main" to fight over, because neither is installed.

The repo had already proved the shape: daily-fresh-fetch runs all of these in one process on cron. This makes the PR path do the same, in parallel.

Measured

mode result wall
serial 1606 passed, 182 skipped 182.8s
-n 4 1606 passed, 182 skipped 27.8s
-n 8 1606 passed, 182 skipped 75.2s

The pass counts are identical in all three, which is the number that matters: the bundle tests exactly what the six jobs tested.

-n 4, not higher. -n 8 is 2.7x slower, which for a fetch-bound suite is upstream throttling rather than core contention — the job's own comment already notes the 5000/h authenticated raw-fetch ceiling. 4 also matches the hosted runner's core count and the -n 4 every other pytest job here uses. --dist loadfile keeps a suite on one worker.

Deliberately NOT folded in

zoo-imports-under-spoof and grpo-fake-run stay separate. Both install a torch stack, and they install mutually exclusive TRL pins ('trl>=0.22,<0.26' against pip install --upgrade trl), so one venv cannot hold both. A guard asserts they still exist, because folding them into a job with no torch at all is the plausible next step and it would not fail loudly.

Two pre-existing gaps this found

The new guard derives coverage from the filesystem rather than from a list, and on its first run reported two suites that no pull_request job runs at all:

suite why it cannot join the bundle
test_import_leaves_torch_globals_alone.py import torch inside a subprocess probe
test_trl_vllm_generation_lora_patch.py needs an installed TRL

Both pre-date this change and both are swept daily by the cron job, so they are checked daily rather than never. The second belongs with grpo-fake-run, which already has that stack, but is not in that job's list either. I left it alone rather than wiring it in blind: I could not verify it passes in that environment first, and an unverified suite would have put an unrelated red on the change that noticed the gap. Recorded in CRON_ONLY with the reasons.

Check names

Six checks become one. Branch protection naming any of the six needs updating.

removed
vLLM pinned-symbol matrix (>= 0.9.0 + main)
TRL / GRPO pinned-symbol matrix
PEFT pinned-symbol matrix (pyproject window + main)
sentence-transformers pinned-symbol matrix
bitsandbytes pinned-symbol matrix
transformers pinned-symbol matrix (4.57.6 + 5.x + main)

Added: pinned-symbol matrix (vLLM, TRL, PEFT, ST, bitsandbytes, transformers)

Verified

  • Mutation-tested, each failing exactly one guard: drop a suite from the bundle; remove -n 4; remove --dist loadfile; sweep the directory instead of naming files.
  • tests/studio 4584 passed.
  • lint_workflow_triggers.py OK across 41 files.
  • No dangling references to the six removed job ids anywhere in the tree.

Version Compat CI spent six runner slots on 287s of combined work. On a sampled main
commit each of the six waited 23 to 31 minutes to be admitted, in order to execute for
between 15 and 128 seconds. The queue is the cost, so the slots are the target.

Why these six can share a runner
------------------------------------------------------------------------
Not a general claim about the suite, a specific one about these tests: they fetch raw
source from raw.githubusercontent.com and grep it for symbols (tests/version_compat/
_fetch.py, has_def / first_match). Nothing is pip-installed, which is the whole point of
the canary and is also what makes co-location safe -- there is no venv for "transformers
4.57.6" and "transformers main" to fight over, because neither is installed.

The repo had already proved the shape: `daily-fresh-fetch` runs all of these in one
process on cron. This makes the PR path do the same thing, in parallel.

Measured on the full set, locally:

    serial   1606 passed, 182 skipped   182.8s
    -n 4     1606 passed, 182 skipped    27.8s
    -n 8     1606 passed, 182 skipped    75.2s

The pass counts are identical in all three, which is the number that matters: the bundle
tests exactly what the six jobs tested.

-n 4 rather than higher. -n 8 is 2.7x SLOWER, which for a fetch-bound suite is upstream
throttling and not core contention -- the job's own comment already notes the 5000/h
authenticated raw-fetch ceiling. 4 also matches the hosted runner's core count and the
-n 4 every other pytest job here uses. --dist loadfile keeps a suite on one worker.

What was deliberately NOT folded in
------------------------------------------------------------------------
zoo-imports-under-spoof and grpo-fake-run stay separate. Both install a torch stack, and
they install mutually exclusive TRL pins ('trl>=0.22,<0.26' against `pip install --upgrade
trl`), so one venv cannot hold both. A guard asserts they still exist, because folding
them into a job with no torch at all is the plausible next step and it would not fail
loudly.

Two pre-existing gaps this found
------------------------------------------------------------------------
The new guard derives coverage from the filesystem rather than from a list, and on its
first run it reported two suites that no pull_request job runs at all:

  test_import_leaves_torch_globals_alone.py   `import torch` inside a subprocess probe
  test_trl_vllm_generation_lora_patch.py      needs an installed TRL

Both pre-date this change and both are swept daily by the cron job, so they are checked
daily rather than never. Neither can join a bundle that installs only pytest. The second
belongs with grpo-fake-run, which already has that stack, but it is not in that job's list
either; it was left alone rather than wired in blind, because I could not verify it passes
in that environment first and an unverified suite would have put an unrelated red on the
change that noticed the gap. Recorded in CRON_ONLY with the reasons.

Check names
------------------------------------------------------------------------
Six checks become one. Removed:

  vLLM pinned-symbol matrix (>= 0.9.0 + main)
  TRL / GRPO pinned-symbol matrix
  PEFT pinned-symbol matrix (pyproject window + main)
  sentence-transformers pinned-symbol matrix
  bitsandbytes pinned-symbol matrix
  transformers pinned-symbol matrix (4.57.6 + 5.x + main)

Added:

  pinned-symbol matrix (vLLM, TRL, PEFT, ST, bitsandbytes, transformers)

Branch protection naming any of the six needs updating.

Verified
------------------------------------------------------------------------
Mutation-tested, each failing exactly one guard: drop a suite from the bundle; remove
-n 4; remove --dist loadfile; sweep the directory instead of naming files.
tests/studio 4584 passed. lint_workflow_triggers OK across 41 files. No dangling
references to the six removed job ids anywhere in the tree.

@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: cd90a76a5a

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# rather than never. Anything added here needs the same kind of reason written down.
CRON_ONLY = {
"tests/version_compat/test_import_leaves_torch_globals_alone.py",
"tests/version_compat/test_trl_vllm_generation_lora_patch.py",

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 Run the cron-only TRL suite with its dependencies

The recorded exception does not actually receive the promised daily coverage: the checked daily-fresh-fetch job installs only pytest before sweeping tests/version_compat/, while test_trl_vllm_generation_lora_patch.py skips the entire module at collection when torch is unavailable. Repository-wide workflow search finds no other invocation of this suite, so its LoRA propagation checks never execute in CI and this exception makes the new coverage guard accept that gap indefinitely; add it to an install-bearing job or provide the required stack in the cron job.

Useful? React with 👍 / 👎.

@danielhanchen

Copy link
Copy Markdown
Member Author

Checked that the bundled job still runs everything the six jobs ran, rather than trusting the green tick.

The job reports 1604 passed / 184 skipped in 93.96s on 4 workers. All eight suites are present in the log by name:

tests/version_compat/test_bitsandbytes_pinned_symbols.py
tests/version_compat/test_peft_conversion_symbol_backfill.py
tests/version_compat/test_peft_pinned_symbols.py
tests/version_compat/test_sentence_transformers_pinned_symbols.py
tests/version_compat/test_transformers_pinned_symbols.py
tests/version_compat/test_trl_grpo_pinned_symbols.py
tests/version_compat/test_unsloth_zoo_save_merged_pinned_symbols.py
tests/vllm_compat/test_vllm_pinned_symbols.py

Running the identical command locally gives 1606 passed / 182 skipped. Same 1788 collected either way, so nothing was dropped. Diffing the skipped test IDs rather than the counts, the two that differ are:

test_peft_conversion_symbol_backfill.py::test_the_moe_snapshot_matches_the_installed_transformers
test_vllm_pinned_symbols.py::test_unsloth_zoo_standby_guards_present

Both skip when their package is not installed (pytest.skip("unsloth_zoo not installed on runner")). They pass locally only because this box already has transformers and unsloth_zoo in its venv. Every one of the six jobs this replaces installed pytest>=8 and nothing else (peft also took packaging), so both tests skipped there too. The split is a property of the runner environment, not of the bundling.

Removed the hardcoded 1606/182 from the guard docstring for that reason, and recorded the CI figure with the explanation, so the next reader does not chase the same discrepancy.

@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: 8f230d8ed4

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# Both are swept by the cron-only `daily-fresh-fetch` job, so they are checked daily
# rather than never. Anything added here needs the same kind of reason written down.
CRON_ONLY = {
"tests/version_compat/test_import_leaves_torch_globals_alone.py",

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 Run the torch-global behavioral probe with torch installed

This exception does not provide the claimed daily coverage for the suite's primary behavioral check: daily-fresh-fetch installs only pytest (version-compat-ci.yml:361-369), while test_importing_the_cpu_modules_leaves_torch_globals_alone skips whenever torch is absent (test_import_leaves_torch_globals_alone.py:77-79). A repo-wide search at this commit finds no other invocation, so CI runs only the static AST check and never verifies that importing the guarded modules actually preserves torch globals; include this suite in an install-bearing job.

Useful? React with 👍 / 👎.

Comment on lines +84 to +85

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 Honor step conditions when calculating PR coverage

Filter steps by their own if condition before counting their test paths. _named_paths currently scans every step unconditionally, and this workflow already has a scheduled/dispatch-only step at version-compat-ci.yml:317-334; if a suite is removed from the preceding PR step but remains in that scheduled step, test_every_suite_still_runs_on_a_pull_request still reports it as PR-covered, defeating the guard's stated purpose.

Useful? React with 👍 / 👎.

@danielhanchen
danielhanchen merged commit 46c9b65 into main Aug 20, 2026
27 of 28 checks passed
@danielhanchen
danielhanchen deleted the version-compat-bundle branch August 20, 2026 14:44
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.

2 participants