Skip to content

preflight: pin the payload on native hosts, not just containerised ones - #363

Merged
glennneuber merged 1 commit into
mainfrom
fix/payload-pin-reads-native-payloads
Sep 20, 2026
Merged

glennneuber merged 1 commit into
mainfrom
fix/payload-pin-reads-native-payloads

Conversation

@glennneuber

@glennneuber glennneuber commented Sep 20, 2026 •

Copy link
Copy Markdown

What

check_payload_pin returned SKIP whenever there was no container, and the native
Metal path has none. This adds the second route — the payload the listening
executable
resolves to, by ollama's own exeDir rule — so the pin works on native
hosts as well as containerised ones.

Why this was found, and why it matters

Not one of the seven metal/mlx-metal profiles carries a llama_cpp_build. That
is not an oversight: the field would have asserted nothing, because the only check
that consumes it could not run there.

The cost showed up promoting 0.34.2 on Metal. LLAMA_CPP_VERSION moved
b10864 → b10969 while MLX_VERSION stayed put, and every Metal expectation was
inherited across that move untouched. The payload move is not cosmetic —
5d806aa25..391fac164 is +1214/−339 across 17 Metal backend files, including a
fusion subsystem that did not exist before and is default-on, and a restructured
tensor-path matmul inside kernel_mul_mm_id.

Nothing in the harness said so. I found it by diffing the file by hand.

That is precisely the accident payload_pin was written for — b10091/b10353 passing
under one version string — occurring on the platform where the check could not run.

How

The resolution already existed for check_metal_tensor_payload:
local_listener_exe() → lib_ollama_llama_server(). This binds it to payload_pin
too, and to the run artefact's meta.llama_cpp_build, so a native run names its
payload the way a containerised one does.

Three deliberate behaviours, each with a test:

  • A remote host still SKIPs. llama-server --version runs on the harness
    host, so a remote server's path either does not exist here or belongs to
    something else. Naming the wrong binary is worse than declining to answer — the
    same rule check_metal_tensor_payload already follows.
  • A resolved path that does not exist SKIPs, it does not FAIL. ollama takes the
    first lib/ollama directory that exists and looks no further, so the binary may
    genuinely be absent. That is cannot-answer, not wrong-payload.
  • llama_cpp_build(None, path=...) now works. With neither exec_cmd nor
    container it used to build ["docker", "exec", None, ...] and die on the None
    in argv — the same shape as the container_logs bug fixed in preflight: gate the M5 Neural Accelerators in all three places they can be lost #341. It now runs
    the payload directly with no shell, so a path containing spaces or braces needs
    no quoting and cannot be re-interpreted.

Verification

  • python3 test_verdicts.py — 186 tests, OK (skipped=6); 9 of them new.

  • Live on this host, against the 0.34.2 candidate on :11437 with no container:

    native llama_cpp_build: 391fac164
    

    which is b10969, matching llama-server --version read directly from the install.

Not in this PR

Measured on macbook-pro-m5-max-128GB/mlx-metal.

🤖 Generated with Claude Code

check_payload_pin returned SKIP whenever there was no container, and the
native Metal path has none. That is why not one of the seven metal/mlx-metal
profiles carries a llama_cpp_build: the field would have asserted nothing, so
nobody added it.

Found promoting 0.34.2 on Metal. Its LLAMA_CPP_VERSION moved b10864 -> b10969
-- a Metal fusion subsystem that did not exist before and is default-on, plus
a restructured tensor-path matmul inside kernel_mul_mm_id -- while every Metal
expectation was inherited across the move untouched. Nothing in the harness
said so; the move was found by diffing the file by hand. That is the same
accident payload_pin was written for (b10091/b10353 under one version string),
on the platform where it could not run.

The resolution already existed for check_metal_tensor_payload:
local_listener_exe() -> lib_ollama_llama_server(), which is ollama's own exeDir
rule. This binds it to payload_pin too, and to the run artefact's
meta.llama_cpp_build, so a native run names its payload like a containerised
one does.

Three specifics worth keeping:
  - remote hosts still SKIP. `llama-server --version` runs on the HARNESS
    host, so a remote server's path either does not exist here or belongs to
    something else; naming the wrong binary is worse than declining.
  - a resolved path that does not exist SKIPs rather than failing. ollama takes
    the first lib/ollama DIRECTORY that exists and looks no further, so the
    binary may genuinely be absent; that is cannot-answer, not wrong-payload.
  - llama_cpp_build with neither exec_cmd nor container used to build
    ["docker", "exec", None, ...] and die on the None in argv. It now runs the
    payload directly, with no shell, so a path with spaces or braces needs no
    quoting.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@glennneuber

Copy link
Copy Markdown
Author

Independent corroboration from the ROCm side, for the "payload move is not cosmetic" argument.

I spent today measuring ROCm 10.0.0 on gfx1151 (#362) and ran into the same b10864 → b10969 move from the other direction. It changes runtime behaviour on ROCm too, not just the Metal backend files you diffed:

gemma4:31b goes from 5 of 27 blocks hitting KV cache to 20 of 27 across that bump — prefill time for the 27-block suite drops 301.6s → 117.0s, −61%. Cold-prefill compute is unchanged (167 → 172 tok/s), so this is purely a cache-reuse change.

I eliminated the fork's own causes one at a time: not direct-I/O (on and off both give 22/5), not OLLAMA_NUM_PARALLEL (gate4main is 0.34.1 at the same 2 and still 22/5), not compat 906, not ADR 0036 (inert on gfx1151 by its own recorded measurement), and not the fork's cache code (git diff 16649e8c..f67b1aef -- '*cache*' is MLX path renames only). The payload bump is what is left.

Two things that may be useful here:

  1. This is a second platform where a version string covered a behavioural move. Your case is Metal expectations inherited across the bump untouched; mine is a 61% prefill change on ROCm that no expectation would have caught either. Same accident, two platforms — which strengthens the case that payload_pin belongs on every route rather than just the containerised one.
  2. I did not attribute it within the bump. That is 105 upstream commits and bisecting at ~1h/build was not proportionate for a change that is an improvement. If your Metal work ever narrows which upstream commit restructured things, I would be interested — it may be the same one, and my side has a cheap 27-block reproducer for it.

No conflict with #362, which touches only summarize_tps.py, the SPEC and docs. This builds cleanly on the toolchain pin from #355.

@glennneuber
glennneuber merged commit 72e8a0f into main Sep 20, 2026
2 checks passed
@glennneuber
glennneuber deleted the fix/payload-pin-reads-native-payloads branch September 20, 2026 14:01
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