chore: update pinned llama.cpp revision - #1216
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
📝 WalkthroughWalkthroughThe change refreshes the llama.cpp revision and patch queue, removes the obsolete RPC transport patch, preserves staged Skippy runtime support, and finalizes chat-template ABI changes. ChangesStaged Skippy runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant skippy_model
participant skippy_session
participant llama_decode
Client->>skippy_model: Open model and select execution lane
skippy_model->>skippy_session: Create or reset session
Client->>skippy_session: Submit staged prefill or decode request
skippy_session->>llama_decode: Execute filtered graph
llama_decode-->>skippy_session: Return tokens, activation frame, or state
skippy_session-->>Client: Return output and runtime signals
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch`:
- Around line 1232-1234: Update the GLM-DSA contract test flow so WebGPU builds
skip before executing the GLM-DSA contract block. Move the GGML_USE_WEBGPU skip
ahead of that block, or make glm_dsa_backend_config_supported() return false for
WebGPU devices, while preserving existing arch_supported() and backend skip
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9ec5fcc-12fd-4112-851c-3e3cd30ad5b0
📒 Files selected for processing (3)
third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patchthird_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patchthird_party/llama.cpp/upstream.txt
💤 Files with no reviewable changes (1)
- third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patch
18941bb to
015210f
Compare
015210f to
157d96e
Compare
157d96e to
5662c59
Compare
5662c59 to
69a822b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch (1)
299-304: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject duplicate execution lanes in batch requests.
Both APIs accept the same
skippy_sessiontwice. This creates multiple batch rows with oneseq_idat the same position. The decode can corrupt that session's KV state.
third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch#L299-L304: Track and reject duplicatesession->seq_idvalues before frame-batch decode.third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch#L760-L765: Apply the same uniqueness check before token-batch decode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch` around lines 299 - 304, Reject duplicate execution lanes by tracking each session->seq_id during request validation and returning SKIPPY_STATUS_INVALID_ARGUMENT before decoding when a sequence ID repeats. Apply this uniqueness check in both frame-batch and token-batch validation loops at third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch lines 299-304 and 760-765.third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch (1)
1287-1326: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear MTP sidecar state after every MTP decode failure.
These paths return after
llama_decodefails but retain pending hidden state and sidecar cache data. A later proposal can use stale state.
third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch#L1287-L1326: Clear the MTP session state before returning a sidecar-sync failure.third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch#L1462-L1467: Clear the MTP session state before returning a chained-proposal failure.third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch#L1510-L1512: Clear the MTP session state before returning a single-step proposal failure.Based on learnings, clear sidecar state after MTP failures to prevent stale proposals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1287 - 1326, Clear all pending MTP hidden state and sidecar cache data before returning from each MTP failure path: the sidecar-sync failure at third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch lines 1287-1326, the chained-proposal failure at lines 1462-1467, and the single-step proposal failure at lines 1510-1512. Reuse the existing MTP session-state reset mechanism so later proposals cannot consume stale state.Source: Learnings
third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch (1)
793-809: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReset sampling state after external session-state changes.
Imported state can represent tokens that differ from
token_history. The existing sampling chain, grammar sampler, and accepted-token count then retain state from a different sequence.
third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch#L793-L809: Clear token and signal history, then reset or rebuild sampling state after full or partial state import.third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch#L1242-L1248: Reset sampling state after KV-page import changes the resident context.third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch#L1496-L1504: Rebuild sampling and grammar state from the restored prefix tokens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch` around lines 793 - 809, Update skippy_update_session_state_after_import to clear token_history and signal_history for imported state, then reset or rebuild the sampling chain, grammar sampler, and accepted-token count from the restored prefix. In third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch lines 1242-1248, reset sampling state after KV-page import changes the resident context. In lines 1496-1504, rebuild sampling and grammar state from the restored prefix tokens.third_party/llama.cpp/patches/0005-Add-Skippy-model-lifecycle-and-package-support.patch (2)
1581-1582: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse 64-bit file positioning for GGUF data.
On Windows,
longis 32-bit. The casts at lines 1582 and 1838 truncate offsets at the 2 GiB boundary. This can corrupt metadata parsing or tensor packaging. Use one checked 64-bit seek helper for both calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0005-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1581 - 1582, Update skippy_skip and the other seek call around the GGUF parsing/packaging logic to use a shared checked 64-bit file-positioning helper instead of casting byte offsets to long. Ensure both seek paths handle offsets beyond 2 GiB correctly on Windows and preserve failure reporting when the seek cannot be completed.
1540-1545: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPrevent exceptions from crossing the C ABI.
If a GGUF tensor name contains an oversized layer index,
std::stolthrowsstd::out_of_rangebeforeskippy_model_info_opencan return. Use a checked, non-throwing parser, validate theint32_trange, and returnSKIPPY_STATUS_MODEL_ERRORfor malformed metadata. The same path is used byskippy_write_gguf_from_parts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0005-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1540 - 1545, Update the tensor-name layer-index parsing in the helper used by skippy_model_info_open and skippy_write_gguf_from_parts to avoid throwing std::stol; use checked non-throwing conversion, validate the parsed value fits int32_t, and return SKIPPY_STATUS_MODEL_ERROR for malformed or oversized indices without allowing exceptions across the C ABI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@third_party/llama.cpp/patches/0005-Add-Skippy-model-lifecycle-and-package-support.patch`:
- Around line 1581-1582: Update skippy_skip and the other seek call around the
GGUF parsing/packaging logic to use a shared checked 64-bit file-positioning
helper instead of casting byte offsets to long. Ensure both seek paths handle
offsets beyond 2 GiB correctly on Windows and preserve failure reporting when
the seek cannot be completed.
- Around line 1540-1545: Update the tensor-name layer-index parsing in the
helper used by skippy_model_info_open and skippy_write_gguf_from_parts to avoid
throwing std::stol; use checked non-throwing conversion, validate the parsed
value fits int32_t, and return SKIPPY_STATUS_MODEL_ERROR for malformed or
oversized indices without allowing exceptions across the C ABI.
In
`@third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch`:
- Around line 793-809: Update skippy_update_session_state_after_import to clear
token_history and signal_history for imported state, then reset or rebuild the
sampling chain, grammar sampler, and accepted-token count from the restored
prefix. In
third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch
lines 1242-1248, reset sampling state after KV-page import changes the resident
context. In lines 1496-1504, rebuild sampling and grammar state from the
restored prefix tokens.
In `@third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch`:
- Around line 299-304: Reject duplicate execution lanes by tracking each
session->seq_id during request validation and returning
SKIPPY_STATUS_INVALID_ARGUMENT before decoding when a sequence ID repeats. Apply
this uniqueness check in both frame-batch and token-batch validation loops at
third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch lines
299-304 and 760-765.
In
`@third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Around line 1287-1326: Clear all pending MTP hidden state and sidecar cache
data before returning from each MTP failure path: the sidecar-sync failure at
third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch
lines 1287-1326, the chained-proposal failure at lines 1462-1467, and the
single-step proposal failure at lines 1510-1512. Reuse the existing MTP
session-state reset mechanism so later proposals cannot consume stale state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45b2ff03-9386-42c5-9a5d-907218069969
📒 Files selected for processing (13)
third_party/llama.cpp/patches/0001-Add-staged-model-graph-and-family-support.patchthird_party/llama.cpp/patches/0002-Add-GLM-DSA-backend-execution-support.patchthird_party/llama.cpp/patches/0003-Add-Inkling-model-and-multimodal-support.patchthird_party/llama.cpp/patches/0004-Add-Skippy-public-ABI-surface.patchthird_party/llama.cpp/patches/0005-Add-Skippy-model-lifecycle-and-package-support.patchthird_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patchthird_party/llama.cpp/patches/0007-Add-Skippy-activation-frame-handling.patchthird_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patchthird_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patchthird_party/llama.cpp/patches/0010-Add-Skippy-tokenization-and-stage-chat.patchthird_party/llama.cpp/patches/0011-Wire-staged-runtime-builds-and-tests.patchthird_party/llama.cpp/patches/0012-Pass-generic-chat-template-kwargs-through-Skippy.patchthird_party/llama.cpp/patches/0013-skippy-remove-legacy-chat-template-ABI.patch
💤 Files with no reviewable changes (5)
- third_party/llama.cpp/patches/0002-Add-GLM-DSA-backend-execution-support.patch
- third_party/llama.cpp/patches/0010-Add-Skippy-tokenization-and-stage-chat.patch
- third_party/llama.cpp/patches/0003-Add-Inkling-model-and-multimodal-support.patch
- third_party/llama.cpp/patches/0007-Add-Skippy-activation-frame-handling.patch
- third_party/llama.cpp/patches/0001-Add-staged-model-graph-and-family-support.patch
Main deliberately removed the RPC backend tensor transport patch in #1216 (no live mesh-llm source references its symbols); the regenerated queue resurrected it unintentionally. Drop it again and renumber back to a contiguous 15-patch series. Validated with a fresh-checkout scripts/prepare-llama.sh pinned replay (patched tree matches the 16-patch tree minus only the RPC hunks) and a full just build.
* chore(llama): regenerate patch queue for latest upstream * fix(skippy): allow filtered stage tensor subsets * chore(llama): drop obsolete RPC transport patch and renumber queue Main deliberately removed the RPC backend tensor transport patch in #1216 (no live mesh-llm source references its symbols); the regenerated queue resurrected it unintentionally. Drop it again and renumber back to a contiguous 15-patch series. Validated with a fresh-checkout scripts/prepare-llama.sh pinned replay (patched tree matches the 16-patch tree minus only the RPC hunks) and a full just build. * fix(llama): remove stale upstream Metal lightning-indexer kernel Patch 0015 replaced ggml_metal_kargs_lightning_indexer with the GLM-DSA layout and deduped the host-side C++, but left upstream's kernel_lightning_indexer body in ggml-metal.metal. That body reads args.n_kv/nbk2/nbk3, which no longer exist in the struct. Metal shaders compile at runtime on-device, so every build passed CI while every model open on Metal failed with: error: no member named 'n_kv' in 'ggml_metal_kargs_lightning_indexer' Extend patch 0015 to also drop the stale kernel block; the GLM-DSA patch provides working kernels under the same host_name entry points. Validated: fresh prepare-llama.sh pinned replay, clean just build, Qwen2.5-3B loads on Metal with zero shader errors and serves chat. * fix(llama): restore missing arch cases in rope-type switch Patch 0015's llama-model.cpp reconciliation dropped three upstream cases from llama_model_rope_type: MUSE_GLIMMER, GRANITE_SWITCH, and QWEN3TTS. Those arches then fell through to GGML_ABORT/legacy rope mode, so opening Muse Glimmer on Metal died with: GGML_ASSERT((mode & 1) == 0 && "mode & 1 == 1 is no longer supported") Restore the cases to their upstream groups (NORM for Muse Glimmer and Granite Switch, IMROPE for Qwen3-TTS). Validated: fresh queue replay, clean just build, Muse-Glimmer-30B UD-Q4_K_XL loads on Metal and answers chat completions. * docs(skill): require model-load spot check for llama patch queue changes Build-only validation cannot catch two failure classes proven by PR 1232: Metal shaders are JIT-compiled on-device at model open, and machine reconciliation can silently drop arch cases from llama-model.cpp switches. Add a required local model-load spot check and an arch-parity diff step to the llama-patch-changes validation guidance. * ci: add macOS Metal inference smoke leg for backend changes Metal shaders are JIT-compiled on-device at first model open, so a broken .metal source builds green everywhere and only fails at load time. Parameterize the reusable smoke workflow with runner and artifact_path inputs and add a macos-15 leg that loads SmolLM2-135M on a real Metal GPU whenever backend inputs change. This would have caught both regressions fixed in 61409ca and 2a0b2b6 before push. * ci: register macOS Metal smoke in the shared PR build plan Route macos_metal_inference_smoke_tests through required_jobs_json via plan-pr-build-jobs.py (eligible + macos product + backend_changed), add it to the summary needs, and keep the credential-bearing Linux smoke on its hardcoded ubuntu-24.04 runner with a separate macos-15 job so the runner-trust and composed-product contract tests hold. --------- Co-authored-by: Michael Neale <michael.neale@gmail.com>
Split serving is broken on main: a worker assigned a layer range that does not start at layer 0 fails to load its stage with check_tensor_dims: tensor 'blk.0.attn_norm.weight' not found and never comes up, so no split can serve inference. The re-pin in #1216 moved the skippy stage filter into the buft_for_tensor lambda. On the file-backed path that lambda is not called until after check_tensor_dims, so a filtered stage is asked for blk.0 — which it legitimately does not contain — before the filter can skip it. The virtual (files.empty()) path calls buft_for_tensor before any lookup, which is why builds and existing tests stayed green. Extract the filter decision into a stage_filter_excludes helper and consult it ahead of check_tensor_dims, keeping the size_data / n_created / g_skippy_last_tensor_filtered bookkeeping added in #1216. buft_for_tensor calls the same helper, so the virtual path is unchanged. Add a regression test that writes a real mid-stage artifact and opens it through the runtime. A filtered config over an unfiltered GGUF does not reproduce the bug, because a full GGUF still contains block 0. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com>
Split serving is broken on main: a worker assigned a layer range that does not start at layer 0 fails to load its stage with check_tensor_dims: tensor 'blk.0.attn_norm.weight' not found and never comes up, so no split can serve inference. The re-pin in #1216 moved the skippy stage filter into the buft_for_tensor lambda. On the file-backed path that lambda is not called until after check_tensor_dims, so a filtered stage is asked for blk.0 — which it legitimately does not contain — before the filter can skip it. The virtual (files.empty()) path calls buft_for_tensor before any lookup, which is why builds and existing tests stayed green. Extract the filter decision into a stage_filter_excludes helper and consult it ahead of check_tensor_dims, keeping the size_data / n_created / g_skippy_last_tensor_filtered bookkeeping added in #1216. buft_for_tensor calls the same helper, so the virtual path is unchanged. Add a regression test that writes a real mid-stage artifact and opens it through the runtime. A filtered config over an unfiltered GGUF does not reproduce the bug, because a full GGUF still contains block 0. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com>
* fix(llama): apply the stage tensor filter before the tensor lookup Split serving is broken on main: a worker assigned a layer range that does not start at layer 0 fails to load its stage with check_tensor_dims: tensor 'blk.0.attn_norm.weight' not found and never comes up, so no split can serve inference. The re-pin in #1216 moved the skippy stage filter into the buft_for_tensor lambda. On the file-backed path that lambda is not called until after check_tensor_dims, so a filtered stage is asked for blk.0 — which it legitimately does not contain — before the filter can skip it. The virtual (files.empty()) path calls buft_for_tensor before any lookup, which is why builds and existing tests stayed green. Extract the filter decision into a stage_filter_excludes helper and consult it ahead of check_tensor_dims, keeping the size_data / n_created / g_skippy_last_tensor_filtered bookkeeping added in #1216. buft_for_tensor calls the same helper, so the virtual path is unchanged. Add a regression test that writes a real mid-stage artifact and opens it through the runtime. A filtered config over an unfiltered GGUF does not reproduce the bug, because a full GGUF still contains block 0. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com> * ci: give skippy-model-package the correctness model and patched llama The new mid-stage load regression test needs both the patched llama.cpp checkout and SKIPPY_CORRECTNESS_MODEL. Both were gated on skippy-runtime only, and rust-tests batches hold up to 4 crates, so skippy-model-package can land in a batch without them and skip the test silently -- exactly the kind of quiet pass that let this regression through. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com> * test: clean up the mid-stage temp dir when artifact writing fails The regression test used `?` on write_stage_artifact, so a write failure returned before the only remove_dir_all call and leaked the temp dir. Capture the result and clean up on the error path before propagating. Addresses CodeRabbit review feedback on #1290. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com> * test(skippy-model-package): use RAII temp dir and drop model before cleanup Addresses review on #1290: the mid-stage regression test could leak its temp dir on the write-error path, and removed the directory while the opened StageModel may still have the artifact mapped, which fails on Windows. Switch to tempfile::tempdir() and drop the model before close. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com> * docs(skills): require mid-stage load proof when re-pinning llama.cpp The re-pin that broke split serving passed every check in the skill: the patch queue applied, the native build succeeded, and the Rust tests were green. What changed upstream was load *ordering*, and nothing exercised a stage whose first block is not block 0. Add a re-pin section to llama-stage-patch-changes: read the upstream loader diff for ordering changes, run the mid-stage regression test, and verify it actually ran instead of skipping on an unset SKIPPY_CORRECTNESS_MODEL. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com> --------- Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com>
Summary
8190848bb36c7df4251db4352bd81bc07d0a4385toe9fa0781f1c25fc4fe8c86be1edc6970661ad6f0.ggml_backend_rpcorRPC_PROTO_*, and the remaining Skippy queue applies without it.Stack
This PR is based on PR #1211 (
jd/fix-issue-986-reasoning-effort-on-1194).Validation
0001–0013numbering.scripts/prepare-llama.sh pinnedreplay produced the exact same patched tree; idempotence verified.just llama-build— patched CPU/Metal native libraries built successfully.just build— full debug product build passed.git diff --check— passed.Summary by CodeRabbit
New Features
Bug Fixes
Refactor