fix(ci): make the "no Metal" macOS job actually build without Metal - #418
Merged
Conversation
The job passed -DLLAMA_METAL=OFF. Upstream's deprecation shim is
function (llama_option_depr TYPE OLD)
if (${OLD})
set(${ARGV2} ON PARENT_SCOPE)
...
so it forwards only a TRUTHY old variable: -DLLAMA_METAL=OFF makes the guard
false, the function does nothing, and GGML_METAL keeps its APPLE default of ON.
CMake prints no "manually-specified variables were not used" warning either,
because the variable IS read -- it just has no effect. The job has therefore
been building Metal for as long as it has existed, which its own logs show: 44
MTL0 / ggml_metal lines, including
ggml_metal_log_allocated_size warnings, in the run that is supposed to have no
Metal at all.
That matters beyond the job's name. The macOS-15 investigation concluded "the
no-Metal job fails identically, so this is not a Metal problem" -- a conclusion
drawn from comparing a Metal build against a Metal build. Passing -DGGML_METAL=OFF
makes that comparison mean something for the first time.
The two -DLLAMA_METAL_EMBED_LIBRARY=ON sites are left alone: the shim forwards a
truthy value correctly, so those do what they say.
Also records the investigation in docs/history/llama-cpp-breaking-changes.md and
retargets the two rows that carried the superseded reasoning. Its findings, all
from primary evidence rather than from the previous write-up:
* The #28323 (n_expert_used_max) suspicion is REFUTED. n_expert_used_arr is a
std::array read through operator[] behind a bounds guard, so it cannot raise
out_of_range("vector"); both failing models are dense, so the two changed
call sites are unreachable; and the change is a widening, which cannot
narrow a value out of range.
* The recorded symptom was half wrong. The first failure loads codellama-7b,
not the draft model, and both failures are REPEAT loads of a model that had
already loaded and generated fine dozens of times in the same JVM.
* The bisect window was never established. The last observed green macOS Java
run is #875 (v5.1.0 dispatch) at pin b10618; b10731 was never observed,
because run #887 died at the spotless gate and skipped everything
downstream. The window is b10618 -> b10797, not b10792 -> b10797.
* Memory pressure is not the discriminator: the GREEN run logged more
over-budget Metal warnings (8) than the red one (6), with more free pages.
No fix for the failure itself. It needs Apple-silicon hardware or one dispatch
per bisect step, neither available where this was investigated -- and per the
note in the row, even "is it still red at b10819" is unverified, since every run
after #897 was cancelled.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
bernardladenthin
had a problem deploying
to
maven-central
September 5, 2026 19:20 — with
GitHub Actions
Failure
bernardladenthin
had a problem deploying
to
maven-central
September 5, 2026 19:20 — with
GitHub Actions
Failure
bernardladenthin
had a problem deploying
to
startgate
September 5, 2026 19:20 — with
GitHub Actions
Error
Code Review: PR 418 - ApprovedThis PR correctly fixes a subtle CMake issue where -DLLAMA_METAL=OFF was silently ignored by upstream's deprecation shim, causing the 'no Metal' job to build with Metal enabled. Key Strengths:
Code Quality:
Recommendation: Approved for merge. This enables genuine Metal vs. no-Metal comparison for the first time, which is essential for proper investigation of the macOS-15 regression. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
The
no MetalmacOS job has never been a no-Metal job. It passes-DLLAMA_METAL=OFF, and upstream's deprecation shim only forwards a truthy old variable:GGML_METALtherefore keeps itsAPPLEdefault ofON. CMake prints no "manually-specified variables were not used" warning either, because the variable is read — it just has no effect. The job's own logs prove it: 44MTL0/ggml_metallines,ggml_metal_log_allocated_sizewarnings included, in the run that is supposed to have no Metal at all. Fixed by passing-DGGML_METAL=OFF.The two
-DLLAMA_METAL_EMBED_LIBRARY=ONsites are deliberately left alone — the shim forwards a truthy value correctly, so those do what they say.Records the macOS-15 investigation in
docs/history/llama-cpp-breaking-changes.mdand retargets the two rows carrying the now-superseded reasoning.Why this is more than a cosmetic flag fix
The standing conclusion was "the no-Metal job fails identically, so this is not a Metal problem." That was a comparison of a Metal build against a Metal build, and it carried no information. This makes the comparison mean something for the first time.
Investigation findings (all from primary evidence — job logs and upstream source, not from the previous write-up)
n_expert_used_max) is refuted, three independent ways:n_expert_used_arris astd::array<uint32_t, LLAMA_MAX_LAYERS>read throughoperator[]behind anil < n_layer_allguard, so it cannot raiseout_of_range("vector")(its failure mode isGGML_ABORT, which aborts rather than throws); both failing models are dense (n_expert == 0), so the two changed call sites inweight_buft_supportedare unreachable and theload_tensorsguard short-circuits before ever calling it; and the change is a widening (max(il)≥[0]), which cannot narrow a value out of range.MemoryManagementTest#testPromptCacheCompleteMissAfterWarmup); the draft-model one is the second (LlamaModelTest#testSpeculativeDecoding). Both are repeat loads of a model that had already loaded and generated fine dozens of times in the same JVM.Code style (spotless) + package graphand skipped everything downstream (30 jobs, noJava Tests macOSamong them). The window is b10618 → b10797, ~180 upstream builds, not the 5-commit b10792 → b10797.ggml_metal_log_allocated_sizeover-budget warnings and more free pages than the red run, which logged 6.Not fixed here
No fix for the failure itself. It needs Apple-silicon hardware or one
workflow_dispatchper bisect step. Note also that "is it still red at b10819" is unverified — every run after #897 was cancelled, so nothing on the currentmainhas been validated by CI at all. The recommended next step is one dispatch onmainafter this merges, which answers that and gives the first genuine Metal-vs-no-Metal comparison in one go.Test plan
yaml.safe_loadonpublish.yml— parses, 62 jobsb10819source, not recalled:CMakeLists.txt:174(llama_option_depr) andggml/CMakeLists.txt:95-96,236(GGML_METAL_DEFAULT ONonAPPLE)Related issues / PRs
Supersedes the macOS reasoning in the
b10797–b10817andb10817–b10819rows (#415, #417).Checklist
CONTRIBUTING.mdandCODE_OF_CONDUCT.md🤖 Generated with Claude Code
https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
Generated by Claude Code