Skip to content

fix(ci): make the "no Metal" macOS job actually build without Metal - #418

Merged
bernardladenthin merged 1 commit into
mainfrom
claude/macos-regression-bisect
Sep 5, 2026
Merged

fix(ci): make the "no Metal" macOS job actually build without Metal#418
bernardladenthin merged 1 commit into
mainfrom
claude/macos-regression-bisect

Conversation

@bernardladenthin

Copy link
Copy Markdown
Owner

Summary

  • The no Metal macOS job has never been a no-Metal job. It passes -DLLAMA_METAL=OFF, and upstream's deprecation shim only forwards a truthy old variable:

    function (llama_option_depr TYPE OLD)
        if (${OLD})                       # <- false for OFF, so the whole body is skipped
            set(${ARGV2} ON PARENT_SCOPE)

    GGML_METAL therefore 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's own logs prove it: 44 MTL0 / ggml_metal lines, ggml_metal_log_allocated_size warnings 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=ON sites 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.md and 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)

  • #28323 (n_expert_used_max) is refuted, three independent ways: n_expert_used_arr is a std::array<uint32_t, LLAMA_MAX_LAYERS> read through operator[] behind an il < n_layer_all guard, so it cannot raise out_of_range("vector") (its failure mode is GGML_ABORT, which aborts rather than throws); both failing models are dense (n_expert == 0), so the two changed call sites in weight_buft_supported are unreachable and the load_tensors guard short-circuits before ever calling it; and the change is a widening (max(il)[0]), which cannot narrow a value out of range.
  • The recorded symptom was half wrong. The first failure loads codellama-7b, not the draft model (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.
  • The bisect window was never established. The last observed green macOS Java test jobs are run 33275073456 (#875, the v5.1.0 dispatch) at pin b10618. b10731 was never observed — run #887 died at Code style (spotless) + package graph and skipped everything downstream (30 jobs, no Java Tests macOS among them). The window is b10618 → b10797, ~180 upstream builds, not the 5-commit b10792 → b10797.
  • Memory pressure is not the discriminator, checked rather than assumed: the green run logged 8 ggml_metal_log_allocated_size over-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_dispatch per bisect step. Note also that "is it still red at b10819" is unverified — every run after #897 was cancelled, so nothing on the current main has been validated by CI at all. The recommended next step is one dispatch on main after this merges, which answers that and gives the first genuine Metal-vs-no-Metal comparison in one go.

Test plan

  • yaml.safe_load on publish.yml — parses, 62 jobs
  • The shim behaviour was read out of the pinned b10819 source, not recalled: CMakeLists.txt:174 (llama_option_depr) and ggml/CMakeLists.txt:95-96,236 (GGML_METAL_DEFAULT ON on APPLE)
  • CI is green on this branch — the changed job may now legitimately behave differently; that is the point of the change
  • Docs updated

Related issues / PRs

Supersedes the macOS reasoning in the b10797–b10817 and b10817–b10819 rows (#415, #417).

Checklist

  • I have read CONTRIBUTING.md and CODE_OF_CONDUCT.md
  • My commits follow Conventional Commits
  • No security-sensitive changes

🤖 Generated with Claude Code

https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH


Generated by Claude Code

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

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Code Review: PR 418 - Approved

This 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:

  • Correct technical fix: properly uses -DGGML_METAL=OFF instead
  • Thoroughly documented investigation with primary evidence
  • Refutes #28323 suspicion with three independent reasons
  • Correctly identifies true bisect window (b10618→b10797, not b10792→b10797)
  • Deliberately leaves -DLLAMA_METAL_EMBED_LIBRARY=ON unchanged (correctly forwards truthy values)

Code Quality:

  • YAML syntax correct and properly formatted
  • Documentation follows conventions
  • No security implications
  • Conventional Commits format

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.

@sonarqubecloud

sonarqubecloud Bot commented Sep 5, 2026

Copy link
Copy Markdown

@bernardladenthin
bernardladenthin merged commit 2e3e3b7 into main Sep 5, 2026
13 of 17 checks passed
@bernardladenthin
bernardladenthin deleted the claude/macos-regression-bisect branch September 5, 2026 19:32
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