Run the Mac GGUF inference phases in the Mac UI job, not on a second runner by danielhanchen · Pull Request #9213 · unslothai/unsloth · GitHub
Skip to content

Run the Mac GGUF inference phases in the Mac UI job, not on a second runner - #9213

Merged
danielhanchen merged 3 commits into
mainfrom
mac-absorb-inference
Aug 19, 2026
Merged

Run the Mac GGUF inference phases in the Mac UI job, not on a second runner#9213
danielhanchen merged 3 commits into
mainfrom
mac-absorb-inference

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

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.yml, 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. Measured over the last 8 green main runs of each:

job exec median queue median
Chat UI, API and Update 1154s 17438s
GGUF inference smoke 402s 13252s

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. Running the two in sequence costs about 400s of wall clock, 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

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. Directly under the install, the implicit success() it inherited meant "the install worked". Bundled behind the UI phases it means "and every Playwright test passed", so one flaky browser run would drop all of phase 1 as a skip, which reads green. 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 intact. 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, and step order is what an edit changes. 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 step.

Trigger and budget

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-minutes goes 65 to 100, against a measured 1154s plus a 483s worst case plus three cold model downloads.

Guard

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 out of the script, because the collision that actually occurred was two phases both omitting --log, which a text scan cannot see.

Six mutations confirmed red, including reintroducing each of the four defects above. test_macos_slots_per_commit.py, test_smoke_workflows_share_one_script.py and test_mac_host_offload_optin.py are repointed at the merged workflow rather than having their coverage dropped.

pytest tests/studio/ -q -n 16: the only failures are the 71 that are already red on main and are fixed by #9189.

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.

@chatgpt-codex-connector

Copy link
Copy Markdown

danielhanchen added a commit that referenced this pull request Aug 18, 2026
…ckage

Five tests guard themselves with `pytest.importorskip("playwright")` and then
import a harness that does `from playwright.sync_api import Page`. On the Repo
tests (CPU) runner the top-level name resolves as a namespace directory with no
sync_api inside it, so the guard passes and the import dies with

  ImportError: cannot import name 'Page' from 'playwright.sync_api' (unknown location)

A skip condition reported as a failure, on every branch, for as long as that
runner stays that way. It is red on #9202 and #9213 too, neither of which touches
any of this.

One of the two files already said what the guard was really for: "importing a
harness pulls in playwright.sync_api". It now checks that.
danielhanchen added a commit that referenced this pull request Aug 19, 2026
…t 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 (54b6ca4).

72 tests, in two different shapes:

1. tests/studio/test_chat_autoload_failure_gate.py, 71 failures. The harness
   slices autoLoadSmallestModel verbatim out of chat-adapter.ts and runs it, so
   anything the slice references must exist in PREAMBLE. mmprojFallbackMessage
   did not, which is a bare ReferenceError inside the retry loop; the loop
   catches it and scores it as a failed load, so every scenario fails as a
   wrong-model assertion. The file's own guard caught this and named the symbol,
   which is what it was written for after #7699 did the same thing.

   Stubbed as a function of the reason rather than a copy of the real
   three-message record. The value only reaches `options.description`, and these
   scenarios assert on which model loaded, never on toast copy, so copying
   user-facing strings in here would give them a second home to drift from.

2. tests/studio/test_model_picker_contracts.py, 1 failure. It asserted the
   literal `description: cpuFallbackReason`, and the mmproj branch went in front
   of it. The property held; only the spelling moved. Its own comment records
   this happening once already, when the CPU-fallback branch first appeared, so
   it now pins the property: the description varies on both fallback reasons and
   still has an undefined arm for the ordinary path.

   Scoped to the description EXPRESSION, not the whole helper. `cpuFallbackReason`
   is also a parameter name in the signature above, so a substring test over the
   block stays green with the CPU branch deleted outright -- the first cut of this
   check did exactly that, and mutation caught it.

Three mutations verified red: the description no longer driven by any fallback
reason, the CPU branch dropped, and the mmproj branch dropped.

4223 passed, 4 skipped.

* Say both fallbacks when both fire, not just the projector one

Both load paths wrote the toast description as

  mmprojFallbackReason ? mmprojMessage : cpuFallbackReason ? cpuMessage : undefined

so whenever both reasons are set the CPU-fallback sentence is dropped. The user
is told "loaded without vision" and never told the model is running on the CPU,
which reads as a deliberate, explained degradation rather than an unaccelerated
session.

The combination is reachable. On a CPU-fallback replay llama_cpp.py preserves
_cpu_fallback_reason (it clears it only when not _replaying_cpu_fallback) and
resets _mmproj_fallback_reason so the projector can fail again inside that same
launch. A low VRAM machine whose Vulkan backend crashed is exactly where the
projector then falls back too.

loadFallbackNotice() in mmproj-fallback.ts is now the single composition of the
title suffix, the description and the degraded flag, and both call sites delegate
to it. CPU_FALLBACK_MESSAGE moves there as well, so the two paths cannot describe
the same condition differently again.

Tests: four combined-case cases in mmproj-fallback.test.ts, verified red against
the shipped nested ternary. test_model_picker_contracts.py now asserts the call
sites delegate and pass both reasons rather than matching the old inline ternary,
and test_chat_autoload_failure_gate.py's stub mirrors the composition so a call
site dropping a reason stays detectable.

* Repin the CPU-fallback toast test to behaviour, and unbreak the queued-capabilities test

Two frontend suites were red.

auto-load-cpu-fallback-toast.test.ts matched the warn-vs-success choice and the
message text as substrings of showAutoLoadSuccess. Both moved into
loadFallbackNotice, which is the single definition the explicit-load path now
shares, so the match went stale. Matching the inline form again would go red on a
refactor that changes nothing a user can see, and would stay green if only one of
the two load paths kept the behaviour. It now calls loadFallbackNotice and asserts
the verdict, and separately asserts the call site delegates to it.

queued-model-capabilities.test.ts was red on main before this branch. #9173 added
`import { isTextOnlyMmprojFallback } from "./mmproj-fallback"` to
image-input-support.ts, which the test imports statically. Extensionless is the
right form -- 2314 of the 2367 relative imports under src/ are written that way,
and vite and tsconfig's "bundler" mode resolve them -- but the bare node loader
does not, and a static import resolves before any registration can run. The test
now registers the bundler resolver and imports dynamically, which is what
mmproj-fallback.test.ts already does for the same module.

Both files reformatted by biome; regex literals hoisted out of the test bodies
for useTopLevelRegex.

Full frontend suite: 3734 pass, 0 fail. typecheck clean. The 4 biome errors in
chat-adapter.ts and use-chat-model-runtime.ts are byte-identical on origin/main.

* Order the split-axis abort against the mmproj strip, not against its argument

test_tensor_split_abort_raises_early_to_layer_fallback has been red on main
since #9173, which renamed the text-only strip's argument from _last_spawn_cmd
to _vision_gpu_cmd. That rename is right: the strip should read the vision GPU
command rather than whatever was spawned last, and #9173 refreshes
_last_spawn_cmd from the result immediately after. The test was pinned to the
old argument name, so a rename with no behavioural content took it down.

The failure also misreported itself. `assert raise_idx < src.find(needle)` reads
as an ordering check but is two claims at once, and when the landmark is gone it
fails with "assert 249423 < -1" -- which says the ordering broke, when what
happened is that the landmark moved. Each landmark is now required to exist
before it is ordered, and says so.

The strip is matched on the call rather than on what is passed to it. What this
test is about is that the abort raises BEFORE the projector is discarded (#6659);
which command the strip reads from is that code's own business.

Checked both ways: removing the strip call from load_model goes red with a
message naming the missing landmark, and renaming the argument again stays green.

* Name the endpoints when the heavy-thread harness catches a stray request

The harness records every /api/ URL issued during a measured action, then keeps
only the count, so the failure reads

  let 2 /api/ requests reach the network during the measured actions

and stops there. It says an interaction paid for a round trip without saying
which one, and the reader has to bisect the frontend to learn what the harness
already knew and discarded.

It now reports the endpoints. Deduplicated and capped at eight, because the case
this instrument exists to catch is a request issued once per message, which would
otherwise print hundreds of copies of one line.

This is why it surfaced now: the step has not run on main since #9173, whose unit
test break fails earlier in the same job and short-circuits it. It was last green
at 54b6ca4. With the unit tests repaired on this branch the job reaches the
step again, and the first thing it needed to say was the one thing it did not.

* Skip the playwright harness tests on the module they need, not the package

Five tests guard themselves with `pytest.importorskip("playwright")` and then
import a harness that does `from playwright.sync_api import Page`. On the Repo
tests (CPU) runner the top-level name resolves as a namespace directory with no
sync_api inside it, so the guard passes and the import dies with

  ImportError: cannot import name 'Page' from 'playwright.sync_api' (unknown location)

A skip condition reported as a failure, on every branch, for as long as that
runner stays that way. It is red on #9202 and #9213 too, neither of which touches
any of this.

One of the two files already said what the guard was really for: "importing a
harness pulls in playwright.sync_api". It now checks that.

* Stop the fork-count store asking the server about threads it has never seen

Two fixes, both found by the heavy-thread smoke once it could name what it caught.

The smoke reported "let 2 /api/ requests reach the network during the measured
actions" and, with the endpoints now printed, they were

  POST /api/chat/threads/__LOCALID_lsQbsDZ/forks

A `__LOCALID_` thread has no server record, so that request can only 404, and
getThreadForkCounts already maps 404 to the empty map the entry starts as. It is
a round trip whose answer is known before it is sent.

Not a rounding error. A new chat is in exactly that state, and this store
refreshes on CHAT_HISTORY_UPDATED_EVENT, which fires once per streaming chunk, so
the first reply in a new chat paid one useless request per debounce window for as
long as it streamed. #8992 added the store to stop the chat getting slower as a
thread fills; excluding threads the server has never seen is the same intent.
thread-ids.ts already had the predicate.

Two tests: a local thread must not fetch on subscribe or on a burst of history
events, and a saved thread on screen beside it must still refresh -- the guard has
to be per thread, not a global off switch. They import the real
isAssistantLocalThreadId rather than restating the prefix, so the rule under test
cannot drift from the app's. Both go red with the guard removed.

Second fix, same job: the playwright skip guard. The previous commit moved it from
"playwright" to "playwright.sync_api" and it still failed, because sync_api
resolves as a namespace package on that runner too. Only the symbol the harnesses
import distinguishes a usable install, so the guard now checks for Page the way
the harness does. Verified both ways against a Page-less sync_api: it skips, and
it still proceeds when Page is there.

Frontend suite 3789 pass, typecheck clean, tests/studio 4237 pass.

* Bound and retry the Playwright browser install so a stall is not a silent 30 minutes

This step stalls. Three times in one day it sat in apt's download loop until the
job's 30-minute timeout killed it, while the sibling shards finished the whole job
in 4 to 9 minutes. Twice on #9202 and once on #9189, always the same step.

The cost is out of proportion to the cause. GitHub scores a job timeout as
"cancelled" rather than a failure, prints no reason, and skips every step after
it, so the chat shard reported nothing about the chat surface for what both times
turned out to be an infrastructure hiccup that cleared on a plain re-run of the
same commit.

A per-attempt `timeout` turns the stall into a failure instead of a silent wait,
and the retry is what actually recovers. The healthy time is about 2 minutes, so 8
per attempt is 4x headroom and a merely slow mirror will not trip it.
timeout-minutes bounds the pair in case `timeout` is outlived by an unkillable
child. Same reasoning, and the same wording, as the bounded prime-hf step in
studio-mac-ui-smoke.yml.

Both install steps in this file, since ui-smoke and ui-indicator run the identical
command. Left alone on mac and windows: neither passes --with-deps, so neither has
the apt phase this is about, and neither has been observed to stall.

* Make the Playwright install retry able to actually recover

The bound added in the previous commit worked: the stall became an 8m37s step
FAILURE with a complete log instead of a silent 30-minute cancellation, and the
log named the cause on the first try.

It also showed the retry could not work. playwright shells out to apt-get as root,
so terminating the python parent leaves that child alive holding the lock, and
attempt 2 died two seconds later with

  E: Could not get lock /var/lib/dpkg/lock-frontend. It is held by process 4578 (apt-get)

A retry that cannot succeed is worse than no retry: it buries the real reason
under a second, different failure. Attempt 2 now waits for the lock to clear, up
to two minutes, and only then takes it -- the holder is our own orphan and the
runner is a throwaway.

Two smaller things the same log exposed. --kill-after was missing, so a process
that ignores SIGTERM would have been waited on forever inside the step bound. And
the warning said "did not finish within 8 minutes" about a two-second exit, which
sends the next reader looking for a stall that never happened; it now separates
timeout's 124/137 from playwright refusing outright, and prints the status.

timeout-minutes 18 to 22 to cover two 8-minute attempts plus the lock wait, still
inside the job's 30.
danielhanchen and others added 3 commits August 19, 2026 03:58
…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.
studio-mac-ui-smoke went red at 35672fc and again at bfcaea4, both times on

  waiting for locator("button[aria-label=\"Permission level for tool calls\"]:visible")

with green runs on either side. The permission block reloads the page four times
with `wait_until="domcontentloaded"` and then asserts the pill visible on the
default 5s expect timeout. DOMContentLoaded fires long before React has mounted
the composer, and on a 3-core macOS runner that gap is regularly wider than 5s.

The composer-mount step at the top of the script already settles the network
before waiting, and its comment says why: "on macos-14 racing straight into
wait_for() either burns the timeout or crashes the renderer mid-mount". The
permission block never got the same treatment. It does now, through one helper
used at all four sites.

This asserts exactly what it asserted before. It just stops asking before the
answer can exist.

It rides with this PR because this PR is what makes that job the sole carrier of
Mac GGUF coverage as well as the UI coverage, so a flake there now costs three
more phases than it did.
@danielhanchen
danielhanchen merged commit 566272b into main Aug 19, 2026
23 of 24 checks passed
@danielhanchen
danielhanchen deleted the mac-absorb-inference branch August 19, 2026 04:00
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