chore(llama): refresh upstream patch queue - #1229
Closed
i386 wants to merge 2 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThe patch queue updates llama.cpp RPC behavior, adds Inkling text and multimodal support, and introduces the Skippy ABI, staged runtime, state management, activation frames, sampling, chat handling, build integration, tests, and upstream compatibility changes. ChangesRPC transport and scheduler diagnostics
Inkling model and multimodal support
Skippy staged runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SkippySession
participant ActivationFrame
participant LlamaModel
participant Sampler
SkippySession->>LlamaModel: Submit prefill or decode request
LlamaModel->>ActivationFrame: Produce activation frame and sidebands
ActivationFrame->>Sampler: Provide staged activation output
Sampler->>SkippySession: Return sampled tokens and signals
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
Contributor
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch (2)
773-789: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet the session position from the imported state.
std::maxkeeps the oldsession->n_pastwhen a caller imports a shorter state. The session then decodes at positions beyond the restored state and retains stale token and signal history.Set
session->n_pastto the validated imported cell count before trimming the histories.🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 773 - 789, Update skippy_update_session_state_after_import to assign session->n_past directly from the validated imported_cells count rather than taking the maximum with its previous value, then trim token_history and signal_history against the restored position.
475-486: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the external-decode graph filter only for its active session.
skippy_session_end_external_decodeclears the filter for any session.skippy_session_freeclears the active-session pointer without clearing the filter. Both paths can leave an active decode with no filter or leave a stale filter after free.
third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch#L475-L486: reject an end call for a session that is notg_skippy_external_decode_session.third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch#L610-L645: clear the graph filter before clearing the active-session pointer during session free.🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 475 - 486, The external-decode graph filter must only be cleared for the active session. In skippy_session_end_external_decode (third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch:475-486), reject sessions that are not g_skippy_external_decode_session before clearing the filter; in skippy_session_free (same file:610-645), clear the graph filter before clearing the active-session pointer.third_party/llama.cpp/patches/0008-Add-Skippy-activation-frame-handling.patch (2)
1507-1533: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the public session handle.
A null
sessionreachesskippy_prepare_output_activation_frame, which dereferencessession->stage_modelwhile populatingoutput_desc.Reject a null or inactive session before preparing the frame.
🤖 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-activation-frame-handling.patch` around lines 1507 - 1533, Update skippy_session_copy_output_activation_frame to validate that session is non-null and active before calling skippy_prepare_output_activation_frame. Return the established invalid-argument/error status for null or inactive handles, and preserve the existing token_count validation and frame-copy flow for valid sessions.
850-889: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPreserve the Inkling MTP sideband in checkpoint prefixes.
This code retains GLM-DSA and RWKV7 sidebands but omits
SKIPPY_ACTIVATION_FLAG_INKLING_MTP_EMBD. It keeps the Inkling flag ininput_desc.
skippy_decode_activation_framethen reads an embedding sideband after the hidden payload. The reconstructed vector does not contain that data. This causes an out-of-bounds read during verify-prefix restore.Include the Inkling hidden-size sideband in both source-size validation and prefix payload copying.
🤖 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-activation-frame-handling.patch` around lines 850 - 889, Update the checkpoint-prefix reconstruction logic around the payload size calculations and copy offsets to account for SKIPPY_ACTIVATION_FLAG_INKLING_MTP_EMBD. Include the Inkling embedding sideband size in source validation and allocate/copy the corresponding prefix bytes after the hidden payload, alongside the GLM-DSA and RWKV7 sidebands, while preserving the flag in input_desc.third_party/llama.cpp/patches/0013-Pass-generic-chat-template-kwargs-through-Skippy.patch (1)
23-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply disabled-thinking overrides after generic kwargs.
A caller can pass
{"enable_thinking": true}afteroverride_enable_thinkingset the aliases to false. The later code only restoresreasoning_effort.Apply every disabled-thinking alias after the generic kwargs loop. This keeps
override_enable_thinking=falseauthoritative.🤖 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/0013-Pass-generic-chat-template-kwargs-through-Skippy.patch` around lines 23 - 34, Update the chat-template kwargs handling around skippy_common_parse_json_or_null so all disabled-thinking aliases are applied after the generic kwargs loop. Ensure override_enable_thinking=false overrides caller-provided aliases such as enable_thinking, while preserving the existing reasoning_effort override.third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch (2)
888-909: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse
positionsfor first-stage positioned prefill.When
activation_inputis false, this function callsskippy_decode_tokens. That path reconstructs positions fromsession->n_pastand ignorespositionsandposition_count.Use a decode path that builds the batch from the supplied positions for both first and downstream stages.
🤖 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-staged-execution-paths.patch` around lines 888 - 909, Update the non-activation branch in the staged prefill flow to use a decode path that consumes the supplied positions and position_count, rather than skippy_decode_tokens reconstructing positions from session->n_past. Preserve the existing skippy_decode_activation_frame path for activation_input and keep the prefill phase hint and error propagation unchanged.
609-635: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not silently ignore raw activation buffers.
Both public raw activation APIs ignore their activation input and output arguments. They can return success with zero output bytes. This contradicts the staged-runtime API contract.
third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch#L609-L635: validate and process raw activation input and output, or returnSKIPPY_STATUS_UNSUPPORTED.third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch#L637-L672: validate and process raw activation input and output, or returnSKIPPY_STATUS_UNSUPPORTED.🤖 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-staged-execution-paths.patch` around lines 609 - 635, Update skippy_prefill_chunk (third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch:609-635) and the corresponding raw activation API at lines 637-672 so they no longer ignore activation buffers: validate and process the input/output buffers according to the staged-runtime contract, or return SKIPPY_STATUS_UNSUPPORTED when unsupported; do not report success with zero output bytes.third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch (1)
1272-1312: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate MTP state after every uncommitted sidecar mutation.
A failed sidecar decode leaves cache and pending proposal state available for later use. A successful proposal with
out_mtp_draft == nullptralso advances the sidecar without recording a pending draft. Later proposals can reuse stale sidecar state.
third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch#L1272-L1312: callskippy_mtp_clear_session_state(session)before returning a sidecar sync failure.third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch#L1447-L1452: clear sidecar session state before returning a chained proposal failure.third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch#L1495-L1497: clear sidecar session state before returning a proposal failure.third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch#L1516-L1524: record the pending proposal independently ofout_mtp_draft, or avoid proposal execution when the caller does not request its result.Based on learnings: if an external MTP sync fails, clear the MTP sidecar session state so later proposals cannot reuse stale sidecar state.
🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1272 - 1312, Invalidate pending MTP sidecar state whenever an uncommitted mutation cannot be safely reused. In third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch lines 1272-1312, 1447-1452, and 1495-1497, call skippy_mtp_clear_session_state(session) before each sidecar, chained-proposal, or proposal failure return. In lines 1516-1524, record the pending proposal independently of out_mtp_draft, or skip proposal execution when its result is not requested, so successful sidecar advancement cannot leave untracked stale state.Source: Learnings
third_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patch (1)
204-207: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the
unpadded_vocab_sizehandling consistent.
get_vocab_basetreatsunpadded_vocab_sizeas optional at line 206 (hp.get("unpadded_vocab_size") or n_vocab).set_gguf_parametersreadshp["unpadded_vocab_size"]at line 275 and again at line 284. A checkpoint without that key passes vocab construction and then fails with aKeyErrorduring metadata writing.Set the key once in
__init__with thevocab_sizefallback, then read it directly everywhere.🐛 Proposed fix
+ # normalize the optional padded-vocab key so every writer path sees a value + if not hp.get("unpadded_vocab_size"): + hp["unpadded_vocab_size"] = hp["vocab_size"] + if hp.get("gate_activation", "sigmoid") != "sigmoid":Also applies to: 275-275
🤖 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/0004-Add-Inkling-model-and-multimodal-support.patch` around lines 204 - 207, Update the tokenizer/model initialization in the constructor containing get_vocab_base so it assigns hparams["unpadded_vocab_size"] once, falling back to hparams["vocab_size"] when absent. Then change get_vocab_base and both unpadded_vocab_size reads in set_gguf_parameters to access the normalized key directly, preventing KeyError for checkpoints without the optional field.third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patch (2)
36-38: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the optional
sinkssplit.Keep the existing
src[3]mask assertion. AddGGML_ASSERT(tensor->src[4] == nullptr || src_ss[4].axis == GGML_BACKEND_SPLIT_AXIS_0);for the optionalsrc[4]sinkstensor.🤖 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/0001-Add-RPC-backend-tensor-transport.patch` around lines 36 - 38, In the tensor split validation alongside the existing src[3] assertion, add validation for optional src[4] sinks: allow it to be null, otherwise require src_ss[4].axis to equal GGML_BACKEND_SPLIT_AXIS_0. Preserve the existing src[3] and src[5] assertions unchanged.Source: MCP tools
13-20: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject incompatible RPC patch versions during HELLO.
negotiate_hellochecksresponse.majorandresponse.minor, but ignoresresponse.patch. A 5.0.5 client therefore accepts a 5.0.0 server. Since this patch changesGGML_OP_COUNT, the client can send operation values that the older server does not define. Reject incompatible patch versions or negotiate operation capabilities, and add a mixed-version test.🤖 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/0001-Add-RPC-backend-tensor-transport.patch` around lines 13 - 20, Update negotiate_hello to validate response.patch alongside response.major and response.minor, rejecting incompatible 5.0.x RPC peers instead of accepting older patch versions. Preserve successful negotiation only when the protocol versions are compatible, and add a mixed-version test covering a 5.0.5 client against a 5.0.0 server.Source: MCP tools
🧹 Nitpick comments (3)
third_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patch (1)
2244-2252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the original indentation in the
MUSE_GLIMMERloop nest.Line 2249 re-indents
for (int rx = 0; rx < f; rx++)out of alignment with the enclosing loops. The behavior does not change. The hunk is unrelated to Inkling and makes future patch replay against upstream noisier.♻️ Proposed fix
for (int oy = 0; oy < grid_h / f; oy++) for (int ox = 0; ox < grid_w / f; ox++) for (int ry = 0; ry < f; ry++) - for (int rx = 0; rx < f; rx++) + for (int rx = 0; rx < f; rx++) dsp.push_back((oy * f + ry) * grid_w + (ox * f + rx));🤖 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/0004-Add-Inkling-model-and-multimodal-support.patch` around lines 2244 - 2252, Restore the original indentation of the inner `for (int rx = 0; rx < f; rx++)` loop in `clip_encode` within the `MUSE_GLIMMER` case so it aligns with the surrounding nested loops; do not alter loop behavior or unrelated patch content.third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch (1)
1105-1107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe Inkling arch stays untested until fixture hparams exist.
arch_supportedreturnsfalseforLLM_ARCH_INKLING, sotest-llama-archsnever exercises the new arch added in patch 0004. The TODO lists the missing fixture keys:d_rel,rel_extent,shortconv,logit_scale_denom.The Inkling loader asserts on these values (
GGML_ASSERT(hparams.inkling_d_rel > 0),GGML_ASSERT(logit_scale_denom != 0.0f)), so the fixture must set them before the arch can run.I can draft the
get_gguf_ctxfixture branch that adds the Inkling keys. Do you want me to open an issue to track it?🤖 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/0012-Wire-staged-runtime-builds-and-tests.patch` around lines 1105 - 1107, Update arch_supported for LLM_ARCH_INKLING so it is no longer excluded, and add the corresponding get_gguf_ctx fixture branch with valid d_rel, rel_extent, shortconv, and logit_scale_denom values required by the Inkling loader assertions. Ensure test-llama-archs exercises the new architecture using those fixture parameters.third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch (1)
359-484: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the unused sequence-to-row mapping. The tensor vectors contain one entry per output row, and
rows[i]preserves that position. Remove the unusedseq_to_output_rowlocal andbuild_seq_to_output_rowhelper.🤖 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/0016-Fix-staged-runtime-upstream-compatibility.patch` around lines 359 - 484, Remove the now-unused seq_to_output_row local from llama_context::decode and delete the build_seq_to_output_row helper. Keep copy_tensor_async_rows using the tensor-vector index and n_outputs_prev row offset as the sole row mapping, and remove any related includes or references that become unused.
🤖 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 1218-1230: Remove the unconditional GGML_USE_WEBGPU assignment
that sets skip for every backend configuration in test_backends. Preserve the
existing arch_supported filtering and only skip configurations that are known to
fail under WebGPU; ensure skipped rows are reflected in the test summary so the
test cannot silently report full coverage.
In
`@third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch`:
- Around line 733-802: Update mtmd_inkling_resize to accept and use the
configured pad mode instead of hard-coding PAD_CEIL, and have
mtmd_image_preprocessor_inkling::preprocess forward hparams.image_resize_pad
alongside hparams.image_resize_algo. Remove the default value for algo from the
public mtmd_image_preprocess_inkling declaration so callers must provide it
explicitly.
- Around line 186-196: Update ggml_metal_device_supports_op to remove the
GGML_OP_LIGHTNING_INDEXER support advertisement, matching the removal from
ggml_metal_op_encode_impl. Do not advertise Lightning Indexer support unless its
encoder implementation is retained.
- Around line 698-708: Remove the remaining llama_model_glm_dsa::graph_mtp MTP
dispatch and constructor implementation introduced by
0003-Add-GLM-DSA-backend-execution-support.patch, not just its nested
declaration. Ensure no graph_mtp references or constructor definitions remain so
the replayed source compiles.
- Around line 51-68: Remove the unused dev parameter from ggml_metal_rsets_init
and update its call in ggml_metal_device_init to use the no-argument signature,
preserving the existing initialization behavior.
---
Outside diff comments:
In `@third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patch`:
- Around line 36-38: In the tensor split validation alongside the existing
src[3] assertion, add validation for optional src[4] sinks: allow it to be null,
otherwise require src_ss[4].axis to equal GGML_BACKEND_SPLIT_AXIS_0. Preserve
the existing src[3] and src[5] assertions unchanged.
- Around line 13-20: Update negotiate_hello to validate response.patch alongside
response.major and response.minor, rejecting incompatible 5.0.x RPC peers
instead of accepting older patch versions. Preserve successful negotiation only
when the protocol versions are compatible, and add a mixed-version test covering
a 5.0.5 client against a 5.0.0 server.
In
`@third_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patch`:
- Around line 204-207: Update the tokenizer/model initialization in the
constructor containing get_vocab_base so it assigns
hparams["unpadded_vocab_size"] once, falling back to hparams["vocab_size"] when
absent. Then change get_vocab_base and both unpadded_vocab_size reads in
set_gguf_parameters to access the normalized key directly, preventing KeyError
for checkpoints without the optional field.
In
`@third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch`:
- Around line 773-789: Update skippy_update_session_state_after_import to assign
session->n_past directly from the validated imported_cells count rather than
taking the maximum with its previous value, then trim token_history and
signal_history against the restored position.
- Around line 475-486: The external-decode graph filter must only be cleared for
the active session. In skippy_session_end_external_decode
(third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch:475-486),
reject sessions that are not g_skippy_external_decode_session before clearing
the filter; in skippy_session_free (same file:610-645), clear the graph filter
before clearing the active-session pointer.
In
`@third_party/llama.cpp/patches/0008-Add-Skippy-activation-frame-handling.patch`:
- Around line 1507-1533: Update skippy_session_copy_output_activation_frame to
validate that session is non-null and active before calling
skippy_prepare_output_activation_frame. Return the established
invalid-argument/error status for null or inactive handles, and preserve the
existing token_count validation and frame-copy flow for valid sessions.
- Around line 850-889: Update the checkpoint-prefix reconstruction logic around
the payload size calculations and copy offsets to account for
SKIPPY_ACTIVATION_FLAG_INKLING_MTP_EMBD. Include the Inkling embedding sideband
size in source validation and allocate/copy the corresponding prefix bytes after
the hidden payload, alongside the GLM-DSA and RWKV7 sidebands, while preserving
the flag in input_desc.
In `@third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch`:
- Around line 888-909: Update the non-activation branch in the staged prefill
flow to use a decode path that consumes the supplied positions and
position_count, rather than skippy_decode_tokens reconstructing positions from
session->n_past. Preserve the existing skippy_decode_activation_frame path for
activation_input and keep the prefill phase hint and error propagation
unchanged.
- Around line 609-635: Update skippy_prefill_chunk
(third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch:609-635)
and the corresponding raw activation API at lines 637-672 so they no longer
ignore activation buffers: validate and process the input/output buffers
according to the staged-runtime contract, or return SKIPPY_STATUS_UNSUPPORTED
when unsupported; do not report success with zero output bytes.
In
`@third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Around line 1272-1312: Invalidate pending MTP sidecar state whenever an
uncommitted mutation cannot be safely reused. In
third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch
lines 1272-1312, 1447-1452, and 1495-1497, call
skippy_mtp_clear_session_state(session) before each sidecar, chained-proposal,
or proposal failure return. In lines 1516-1524, record the pending proposal
independently of out_mtp_draft, or skip proposal execution when its result is
not requested, so successful sidecar advancement cannot leave untracked stale
state.
In
`@third_party/llama.cpp/patches/0013-Pass-generic-chat-template-kwargs-through-Skippy.patch`:
- Around line 23-34: Update the chat-template kwargs handling around
skippy_common_parse_json_or_null so all disabled-thinking aliases are applied
after the generic kwargs loop. Ensure override_enable_thinking=false overrides
caller-provided aliases such as enable_thinking, while preserving the existing
reasoning_effort override.
---
Nitpick comments:
In
`@third_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patch`:
- Around line 2244-2252: Restore the original indentation of the inner `for (int
rx = 0; rx < f; rx++)` loop in `clip_encode` within the `MUSE_GLIMMER` case so
it aligns with the surrounding nested loops; do not alter loop behavior or
unrelated patch content.
In
`@third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch`:
- Around line 1105-1107: Update arch_supported for LLM_ARCH_INKLING so it is no
longer excluded, and add the corresponding get_gguf_ctx fixture branch with
valid d_rel, rel_extent, shortconv, and logit_scale_denom values required by the
Inkling loader assertions. Ensure test-llama-archs exercises the new
architecture using those fixture parameters.
In
`@third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch`:
- Around line 359-484: Remove the now-unused seq_to_output_row local from
llama_context::decode and delete the build_seq_to_output_row helper. Keep
copy_tensor_async_rows using the tensor-vector index and n_outputs_prev row
offset as the sole row mapping, and remove any related includes or references
that become unused.
🪄 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: b3e68f48-bda2-432c-ab9e-d85b7131a6bf
📒 Files selected for processing (17)
third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patchthird_party/llama.cpp/patches/0002-Add-staged-model-graph-and-family-support.patchthird_party/llama.cpp/patches/0003-Add-GLM-DSA-backend-execution-support.patchthird_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patchthird_party/llama.cpp/patches/0005-Add-Skippy-public-ABI-surface.patchthird_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patchthird_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patchthird_party/llama.cpp/patches/0008-Add-Skippy-activation-frame-handling.patchthird_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patchthird_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patchthird_party/llama.cpp/patches/0011-Add-Skippy-tokenization-and-stage-chat.patchthird_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patchthird_party/llama.cpp/patches/0013-Pass-generic-chat-template-kwargs-through-Skippy.patchthird_party/llama.cpp/patches/0014-skippy-remove-legacy-chat-template-ABI.patchthird_party/llama.cpp/patches/0015-docs-annotate-Skippy-native-API-headers.patchthird_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patchthird_party/llama.cpp/upstream.txt
Comment on lines
+1218
to
+1230
| @@ -614,6 +1643,12 @@ static int test_backends(const llm_arch target_arch, const size_t seed, const gg | ||
| std::string status_roundtrip = "\033[1;33mSKIP\033[0m"; | ||
| char nmse_str[12] = {0}; | ||
| bool skip = !arch_supported(arch) || (dc.split_mode == LLAMA_SPLIT_MODE_TENSOR && dc.devs.empty()); | ||
| + if (arch == LLM_ARCH_GLM_DSA && !glm_dsa_backend_config_supported(dc.devs)) { | ||
| + skip = true; | ||
| + } | ||
| #if defined(GGML_USE_WEBGPU) | ||
| skip = true; // FIXME | ||
| #endif // GGML_USE_WEBGPU | ||
| -- | ||
| 2.54.0 (Apple Git-157) | ||
|
|
||
| +#if defined(GGML_USE_WEBGPU) | ||
| + skip = true; // FIXME | ||
| +#endif // GGML_USE_WEBGPU | ||
| if (!skip) { | ||
| if (logits_cpu.empty()) { | ||
| model_and_ctx_cpu = get_model_and_ctx(gguf_ctx.get(), nullptr, seed, {}, LLAMA_SPLIT_MODE_LAYER, encode); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The unconditional WebGPU skip disables the whole backend matrix and reports success.
Under GGML_USE_WEBGPU, skip = true applies to every arch and every device config. Every row becomes SKIP, all_ok stays true, and the test exits with success. The per-arch WebGPU filter in arch_supported (lines 1109-1111) also becomes unreachable in that build.
Scope the skip to the configurations that actually fail, or keep the blanket skip and record the reason in the test summary so a WebGPU build cannot silently report full coverage.
I can prepare a scoped skip plus a summary line that reports the skipped count. Do you want me to open an issue to track the FIXME?
🤖 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/0012-Wire-staged-runtime-builds-and-tests.patch`
around lines 1218 - 1230, Remove the unconditional GGML_USE_WEBGPU assignment
that sets skip for every backend configuration in test_backends. Preserve the
existing arch_supported filtering and only skip configurations that are known to
fail under WebGPU; ensure skipped rows are reflected in the test summary so the
test cannot silently report full coverage.
Comment on lines
+51
to
+68
| @@ -566,7 +566,7 @@ void ggml_metal_encoder_end_encoding(ggml_metal_encoder_t encoder) { | ||
| dispatch_group_t d_group; | ||
| }; | ||
|
|
||
| -ggml_metal_rsets_t ggml_metal_rsets_init(void) { | ||
| +ggml_metal_rsets_t ggml_metal_rsets_init(ggml_metal_device_t dev) { | ||
| ggml_metal_rsets_t res = calloc(1, sizeof(struct ggml_metal_rsets)); | ||
|
|
||
| res->lock = [[NSLock alloc] init]; | ||
| @@ -873,7 +873,7 @@ ggml_metal_device_t ggml_metal_device_init(int device) { | ||
| } | ||
|
|
||
| if (dev->props.use_residency_sets) { | ||
| - dev->rsets = ggml_metal_rsets_init(); | ||
| + dev->rsets = ggml_metal_rsets_init(dev); | ||
| } else { | ||
| dev->rsets = nil; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate every declaration and use of ggml_metal_rsets_init in the patch queue.
rg -n -C4 'ggml_metal_rsets_init' third_party/llama.cpp/patchesRepository: Mesh-LLM/mesh-llm
Length of output: 5432
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- matching declarations and calls in all patch files ---'
rg -n -C6 'ggml_metal_rsets_init' third_party/llama.cpp/patches
printf '%s\n' '--- declaration hunk in patch 0016 ---'
sed -n '65,105p' third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch
printf '%s\n' '--- repository files containing the symbol ---'
rg -n -C3 'ggml_metal_rsets_init' --glob '!third_party/llama.cpp/patches/**' .Repository: Mesh-LLM/mesh-llm
Length of output: 8066
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- patch 0016 symbol context ---'
rg -n -C20 'ggml_metal_rsets_init|ggml_metal_device_t' \
third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch
printf '%s\n' '--- patch 0003 symbol context ---'
sed -n '6300,6355p' \
third_party/llama.cpp/patches/0003-Add-GLM-DSA-backend-execution-support.patch
printf '%s\n' '--- tracked llama.cpp paths ---'
git ls-files third_party/llama.cpp | sed -n '1,120p'
printf '%s\n' '--- patch metadata and order ---'
rg -n '0016|0003|series|quilt|patch' third_party/llama.cpp .github Makefile CMakeLists.txt 2>/dev/null | sed -n '1,160p'Repository: Mesh-LLM/mesh-llm
Length of output: 31655
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- warning and error flags ---'
rg -n -C3 --glob '!third_party/llama.cpp/patches/**' \
'Wunused-parameter|Werror|warnings-as-errors|-Wall|-Wextra' \
. | sed -n '1,220p'
printf '%s\n' '--- exact patch 0016 device source hunk ---'
sed -n '47,70p' \
third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch
printf '%s\n' '--- standalone Objective-C unused-parameter probe ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/probe.m" <<'EOF'
typedef void * ggml_metal_device_t;
void f(ggml_metal_device_t dev) {
}
EOF
if command -v clang >/dev/null 2>&1; then
clang -fsyntax-only -Werror -Wunused-parameter "$tmpdir/probe.m" 2>&1
status=$?
printf 'clang_status=%s\n' "$status"
else
printf '%s\n' 'clang_unavailable'
fiRepository: Mesh-LLM/mesh-llm
Length of output: 1339
Remove the unused dev parameter.
The patch updates the definition and its only call consistently. The patch queue contains no separate declaration. The function body does not use dev, so builds with -Wunused-parameter and -Werror can fail. Restore the void signature or explicitly consume dev.
🤖 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/0016-Fix-staged-runtime-upstream-compatibility.patch`
around lines 51 - 68, Remove the unused dev parameter from ggml_metal_rsets_init
and update its call in ggml_metal_device_init to use the no-argument signature,
preserving the existing initialization behavior.
Comment on lines
+698
to
+708
| @@ -1295,10 +1295,6 @@ struct llama_model_glm_dsa : public llama_model_base { | ||
| void load_arch_hparams(llama_model_loader & ml) override; | ||
| void load_arch_tensors(llama_model_loader & ml) override; | ||
|
|
||
| - struct graph_mtp : public llm_graph_context { | ||
| - graph_mtp(const llama_model & model, const llm_graph_params & params); | ||
| - }; | ||
| - | ||
| struct graph : public llm_graph_context { | ||
| graph(const llama_model & model, const llm_graph_params & params); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find remaining references to the GLM_DSA graph_mtp type in the patch queue.
rg -n -C6 'graph_mtp' third_party/llama.cpp/patchesRepository: Mesh-LLM/mesh-llm
Length of output: 21388
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- patch queue order ---'
git ls-files 'third_party/llama.cpp/patches/*.patch' | sort -V
printf '%s\n' '--- GLM_DSA graph_mtp references in relevant patches ---'
rg -n -C8 'llama_model_glm_dsa::graph_mtp|std::make_unique<graph_mtp>|struct graph_mtp|graph_mtp::graph_mtp' \
third_party/llama.cpp/patches/0003-Add-GLM-DSA-backend-execution-support.patch \
third_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patch \
third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch || true
printf '%s\n' '--- all graph_mtp lines in patch 0016 ---'
rg -n -C4 'graph_mtp' third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch || true
printf '%s\n' '--- patch 0016 hunk headers near GLM_DSA source ---'
rg -n -C3 'glm_dsa|GLM_DSA' third_party/llama.cpp/patches/0016-Fix-staged-runtime-upstream-compatibility.patch || trueRepository: Mesh-LLM/mesh-llm
Length of output: 12656
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
root = Path("third_party/llama.cpp/patches")
patterns = {
"glm_dsa_mtp_dispatch": re.compile(r"std::make_unique<graph_mtp>\(\*this, params\)"),
"glm_dsa_mtp_definition": re.compile(r"llama_model_glm_dsa::graph_mtp::graph_mtp"),
"glm_dsa_mtp_declaration": re.compile(r"struct graph_mtp|^\s*graph_mtp\(const llama_model"),
}
for path in sorted(root.glob("*.patch")):
additions = []
deletions = []
for number, raw in enumerate(path.read_text().splitlines(), 1):
if raw.startswith(("+++", "---")):
continue
body = raw[1:] if raw[:1] in "+-" else raw
if any(pattern.search(body) for pattern in patterns.values()):
if raw.startswith("+"):
additions.append((number, body))
elif raw.startswith("-"):
deletions.append((number, body))
if additions or deletions:
print(path)
for number, body in additions:
print(f" ADD {number}: {body}")
for number, body in deletions:
print(f" DEL {number}: {body}")
PY
printf '%s\n' '--- exact GLM_DSA hunks in patch 0003 ---'
sed -n '30830,30858p;31268,31294p' \
third_party/llama.cpp/patches/0003-Add-GLM-DSA-backend-execution-support.patchRepository: Mesh-LLM/mesh-llm
Length of output: 4476
Remove the remaining llama_model_glm_dsa::graph_mtp implementation
0003-Add-GLM-DSA-backend-execution-support.patch retains the MTP dispatch and constructor definition, while patch 0016 removes only the nested declaration. Remove these remaining references, or the replayed source will fail to compile.
🤖 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/0016-Fix-staged-runtime-upstream-compatibility.patch`
around lines 698 - 708, Remove the remaining llama_model_glm_dsa::graph_mtp MTP
dispatch and constructor implementation introduced by
0003-Add-GLM-DSA-backend-execution-support.patch, not just its nested
declaration. Ensure no graph_mtp references or constructor definitions remain so
the replayed source compiles.
Comment on lines
+733
to
+802
| +static clip_image_u8 mtmd_inkling_resize(const clip_image_u8 & img, resize_algo algo) { | ||
| + constexpr int patch_size = 40; | ||
| + const clip_image_size source_size = img.get_size(); | ||
| + const clip_image_size target_size = { | ||
| + std::max(patch_size, ((source_size.width + patch_size - 1) / patch_size) * patch_size), | ||
| + std::max(patch_size, ((source_size.height + patch_size - 1) / patch_size) * patch_size), | ||
| + }; | ||
| + | ||
| + clip_image_u8 resized; | ||
| + img_tool::resize(img, resized, target_size, algo, PAD_CEIL); | ||
| + return resized; | ||
| +} | ||
| + | ||
| +mtmd_inkling_image_preproc_out mtmd_image_preprocess_inkling( | ||
| + const clip_image_u8 & img, | ||
| + resize_algo algo) { | ||
| + constexpr int patch_size = 40; | ||
| + constexpr int temporal_patch_size = 2; | ||
| + | ||
| + const clip_image_u8 resized = mtmd_inkling_resize(img, algo); | ||
| + const clip_image_size resized_size = resized.get_size(); | ||
| + mtmd_inkling_image_preproc_out output; | ||
| + output.source_size = img.get_size(); | ||
| + output.resized_size = resized_size; | ||
| + output.patch_rows = resized_size.height / patch_size; | ||
| + output.patch_cols = resized_size.width / patch_size; | ||
| + output.resized_rgb = resized.get_ro_buf(); | ||
| + output.pixel_values_bthwc.reserve( | ||
| + (size_t) output.patch_rows * output.patch_cols * temporal_patch_size * patch_size * patch_size * 3); | ||
| + | ||
| + for (int row = 0; row < output.patch_rows; ++row) { | ||
| + for (int col = 0; col < output.patch_cols; ++col) { | ||
| + for (int temporal = 0; temporal < temporal_patch_size; ++temporal) { | ||
| + (void) temporal; | ||
| + for (int y = 0; y < patch_size; ++y) { | ||
| + for (int x = 0; x < patch_size; ++x) { | ||
| + const auto pixel = resized.get_pixel(col * patch_size + x, row * patch_size + y); | ||
| + output.pixel_values_bthwc.push_back((float) pixel[0] / 255.0f); | ||
| + output.pixel_values_bthwc.push_back((float) pixel[1] / 255.0f); | ||
| + output.pixel_values_bthwc.push_back((float) pixel[2] / 255.0f); | ||
| + } | ||
| + } | ||
| + } | ||
| + } | ||
| + } | ||
| + | ||
| + return output; | ||
| +} | ||
| + | ||
| +mtmd_image_preproc_out mtmd_image_preprocessor_inkling::preprocess(const clip_image_u8 & img) { | ||
| + constexpr int patch_size = 40; | ||
| + constexpr int temporal_patch_size = 2; | ||
| + | ||
| + const clip_image_u8 resized = mtmd_inkling_resize(img, hparams.image_resize_algo); | ||
| + const clip_image_size resized_size = resized.get_size(); | ||
| + const int patch_rows = resized_size.height / patch_size; | ||
| + const int patch_cols = resized_size.width / patch_size; | ||
| + | ||
| + mtmd_image_preproc_out output; | ||
| + output.entries.reserve((size_t) patch_rows * patch_cols * temporal_patch_size); | ||
| + for (int row = 0; row < patch_rows; ++row) { | ||
| + for (int col = 0; col < patch_cols; ++col) { | ||
| + clip_image_u8 patch; | ||
| + img_tool::crop(resized, patch, col * patch_size, row * patch_size, patch_size, patch_size); | ||
| + output.append(hparams, patch, true); | ||
| + output.append(hparams, patch, true); | ||
| + } | ||
| + } | ||
| + return output; | ||
| +} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The Inkling image preprocessor ignores the declared resize and pad configuration.
Patch 0004 sets hparams.image_resize_algo = RESIZE_ALGO_BICUBIC_PILLOW and hparams.image_resize_pad = PAD_NONE for PROJECTOR_TYPE_INKLING. mtmd_inkling_resize hard-codes PAD_CEIL at line 742, and mtmd_image_preprocess_inkling defaults algo to RESIZE_ALGO_LANCZOS in the header declaration.
Two consequences follow. The pad mode declared in the model config never applies. Any caller of the public mtmd_image_preprocess_inkling helper that omits algo resizes with a different filter than the runtime path, so the pixel values differ from the model contract.
Pass the configured pad mode through, and require an explicit algo argument on the public helper.
🐛 Proposed fix
-static clip_image_u8 mtmd_inkling_resize(const clip_image_u8 & img, resize_algo algo) {
+static clip_image_u8 mtmd_inkling_resize(const clip_image_u8 & img, resize_algo algo, padding_mode pad) {
constexpr int patch_size = 40;
const clip_image_size source_size = img.get_size();
const clip_image_size target_size = {
std::max(patch_size, ((source_size.width + patch_size - 1) / patch_size) * patch_size),
std::max(patch_size, ((source_size.height + patch_size - 1) / patch_size) * patch_size),
};
clip_image_u8 resized;
- img_tool::resize(img, resized, target_size, algo, PAD_CEIL);
+ img_tool::resize(img, resized, target_size, algo, pad);
return resized;
}Then update the member function to forward hparams.image_resize_pad, and drop the default algo value in tools/mtmd/mtmd-image.h.
🤖 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/0016-Fix-staged-runtime-upstream-compatibility.patch`
around lines 733 - 802, Update mtmd_inkling_resize to accept and use the
configured pad mode instead of hard-coding PAD_CEIL, and have
mtmd_image_preprocessor_inkling::preprocess forward hparams.image_resize_pad
alongside hparams.image_resize_algo. Remove the default value for algo from the
public mtmd_image_preprocess_inkling declaration so callers must provide it
explicitly.
…-canary-2026-08-11 # Conflicts: # third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patch # third_party/llama.cpp/patches/0001-Add-staged-model-graph-and-family-support.patch # third_party/llama.cpp/patches/0002-Add-GLM-DSA-backend-execution-support.patch # third_party/llama.cpp/patches/0003-Add-Inkling-model-and-multimodal-support.patch # third_party/llama.cpp/patches/0004-Add-Skippy-public-ABI-surface.patch # third_party/llama.cpp/patches/0005-Add-Skippy-model-lifecycle-and-package-support.patch # third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch # third_party/llama.cpp/patches/0007-Add-Skippy-activation-frame-handling.patch # third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch # third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch # third_party/llama.cpp/patches/0010-Add-Skippy-tokenization-and-stage-chat.patch # third_party/llama.cpp/patches/0011-Wire-staged-runtime-builds-and-tests.patch # third_party/llama.cpp/patches/0012-Pass-generic-chat-template-kwargs-through-Skippy.patch # third_party/llama.cpp/patches/0013-skippy-remove-legacy-chat-template-ABI.patch # third_party/llama.cpp/upstream.txt
Collaborator
Author
This was referenced Aug 10, 2026
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
1 participant
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
030ebb558a5820b444a8f836ed5cdd46c9b4bd7a.31431178275, job93594710354.Validation
scripts/prepare-llama.sh pinned— clean replay of all 16 patches.LLAMA_STAGE_BACKEND=cpu LLAMA_STAGE_LINK_MODE=static scripts/build-llama.sh— 302/302 targets.just build— dynamic Metal product build and packaging, 310/310 native targets.cargo fmt --all --check.Summary by CodeRabbit