Update pinned llama.cpp - #927
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTen sequential patch files add the "Skippy" experimental C ABI to a vendored llama.cpp fork. The patches introduce a public ChangesSkippy Staged-Inference ABI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (16)
third_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patch (3)
1506-1527: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdvertise the implemented chat-tool and grammar features.
skippy_abi_features()defines support bits forSKIPPY_FEATURE_CHAT_TEMPLATE_TOOLSandSKIPPY_FEATURE_CHAT_SAMPLING_GRAMMAR, but does not return them, so feature-probing clients will treat these implemented APIs as unavailable.Suggested fix
SKIPPY_FEATURE_SESSION_CHECKPOINT | SKIPPY_FEATURE_PACKAGE_PART_LOAD | SKIPPY_FEATURE_GENERATION_SIGNALS | - SKIPPY_FEATURE_EXTERNAL_MEDIA_PREFILL; + SKIPPY_FEATURE_EXTERNAL_MEDIA_PREFILL | + SKIPPY_FEATURE_CHAT_TEMPLATE_TOOLS | + SKIPPY_FEATURE_CHAT_SAMPLING_GRAMMAR;🤖 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-lanes-external-media-and-chat-grammar-support.patch` around lines 1506 - 1527, Update skippy_abi_features() so it advertises every implemented capability currently supported by the ABI, including SKIPPY_FEATURE_CHAT_TEMPLATE_TOOLS and SKIPPY_FEATURE_CHAT_SAMPLING_GRAMMAR, by adding their bits to the returned feature mask alongside the existing SKIPPY_FEATURE_GENERATION_SIGNALS and SKIPPY_FEATURE_EXTERNAL_MEDIA_PREFILL entries. Keep the feature list in sync with the actual APIs exposed in this patch so clients probing skippy_abi_features() see the new chat-tool and grammar support as available.
1751-1757: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn an error for invalid chat sampling metadata.
skippy_session_configure_chat_sampling()clears sampling and returns success when JSON parsing or grammar setup throws. That hides malformed metadata and silently disables grammar-constrained sampling.Suggested fix
} catch (const std::exception & e) { skippy_clear_chat_sampling(session); - return skippy_success(out_error); + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, e.what()); + return SKIPPY_STATUS_INVALID_ARGUMENT; } catch (...) { skippy_clear_chat_sampling(session); - return skippy_success(out_error); + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "invalid chat sampling metadata"); + return SKIPPY_STATUS_INVALID_ARGUMENT; }🤖 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-lanes-external-media-and-chat-grammar-support.patch` around lines 1751 - 1757, In skippy_session_configure_chat_sampling(), the current catch blocks for std::exception and ... clear sampling and return success, which hides malformed metadata; change this flow so JSON parsing or grammar setup failures propagate as an error instead of silently succeeding. Use the existing out_error and skippy_success/skippy_clear_chat_sampling handling in this function to return a failure status with a clear invalid chat sampling metadata message, while keeping the success path unchanged for valid metadata.
1538-1569: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSynchronize shared context and lane ownership.
Sessions now share
stage_model->ctxand mutatelane_in_use, but there is no mutex around lane allocation/freeing or decode/reset/trim calls on the sharedllama_context. Concurrent sessions can race and corrupt per-lane KV/state.Also applies to: 1621-1663
🤖 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-lanes-external-media-and-chat-grammar-support.patch` around lines 1538 - 1569, `stage_model->ctx` and `lane_in_use` are shared across sessions without synchronization, so lane allocation/freeing and shared-context operations can race. Add a mutex/lock on the stage model and guard all lane ownership changes plus every decode/reset/trim path that touches the shared `llama_context`. Update the affected session methods around `skippy_finish_model_open` and the later lane-management code referenced by the review so they acquire the same lock before reading or mutating `lane_in_use` or calling into `stage_model->ctx`.third_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patch (3)
946-960: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed position sidebands instead of silently falling back.
If
positions != nullptrbutposition_countis wrong, this path ignores caller-supplied positions and usesn_pastpositions. For external media/M-RoPE prefill, that can decode at the wrong positions without surfacing an error.Suggested fix
- if (positions != nullptr && position_count == expected_position_count) { + if ((positions == nullptr) != (position_count == 0)) { + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "positions and position_count must be provided together"); + return SKIPPY_STATUS_INVALID_ARGUMENT; + } + if (positions != nullptr && position_count != expected_position_count) { + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "position_count does not match model position layout"); + return SKIPPY_STATUS_INVALID_ARGUMENT; + } + if (positions != nullptr) { std::memcpy(pos_storage.data(), positions, expected_position_count*sizeof(llama_pos));🤖 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-external-decode-media-prefill-and-newer-family-s.patch` around lines 946 - 960, The position handling in the external prefill path silently falls back to n_past when positions is non-null but position_count is invalid. Update the logic around the pos_storage fill branch to reject malformed caller-supplied sidebands instead of entering the fallback path, and surface an error from this decode/prefill function so external media/M-RoPE prefill cannot proceed with incorrect positions.
393-405: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard the transposed-V loops when a selected layer has no V stream.
The new selection path permits
has_v_stream == false, settingv_element_bytes/v_row_bytesto zero, but only the non-transposed V loops are skipped. Inv_transmode, the existing branch will still dereferencelayer->v_stream[strm].Suggested fix
} else { for (const auto * layer : selected) { + if (v_element_bytes == 0) { + continue; + } auto * v = layer->v_stream[strm];} else { for (const auto * layer : selected) { + if (desc.v_element_bytes == 0) { + continue; + } auto * v = layer->v_stream[strm];Also applies to: 409-442
🤖 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-external-decode-media-prefill-and-newer-family-s.patch` around lines 393 - 405, The selected-layer export path in llama_kv_cache::stage_export_kv_page still enters the transposed-V handling even when a layer has no V stream, which can dereference layer->v_stream[strm]. Add an early skip or equivalent guard in the v_trans branch alongside the existing v_row_bytes check so layers with no V stream are ignored in both transposed and non-transposed paths, and make sure the guard applies to the loops that build V payloads for selected layers.
28-95: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBump the staged-runtime ABI for these new exports. This patch adds public flags and session/prefill entrypoints, so
SKIPPY_ABI_VERSION_*should move here and the Rust FFI constants should stay aligned.🤖 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-external-decode-media-prefill-and-newer-family-s.patch` around lines 28 - 95, The staged-runtime ABI needs to be bumped to account for the new public exports added here. Update the SKIPPY_ABI_VERSION constant(s) alongside the new skippy_session_begin_external_decode, skippy_session_end_external_decode, skippy_prefill_chunk_frame_sampled, skippy_prefill_chunk_frame_with_positions, and skippy_prefill_chunk_frame_sampled_with_positions declarations, and make sure the Rust FFI-side ABI constants remain in sync with the C header.Source: Coding guidelines
third_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patch (1)
279-295: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftKeep ordered GGUF part contexts alive.
ctx_ggufis local to each loop iteration, butweights_mapstores pointers derived from it (ctx_gguf.get()andcur). Ifllama_tensor_weightretains those pointers, later tensor loading will dereference freed GGUF/tensor metadata.🤖 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/0003-Add-staged-sampling-checkpoints-and-part-loading.patch` around lines 279 - 295, The staged part-loading logic in the tensor-loading loop is storing `llama_tensor_weight` entries that depend on `ctx_gguf.get()` and `cur`, but `ctx_gguf` is currently a per-iteration local in the GGUF part handling code. Update the part-context management so the GGUF contexts created in this block stay alive for as long as `weights_map` may use them, using the existing `contexts`/`weights_map` flow around `gguf_init_from_file`, `weights_map.emplace`, and `llama_tensor_weight` to retain ordered per-part metadata instead of letting it be destroyed at the end of the loop iteration.third_party/llama.cpp/patches/0005-Add-resident-prefix-cache-and-session-refinements.patch (3)
1268-1274: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSynchronize before mutating native KV tensors.
Export synchronizes before reading KV state, but import writes cells/tensors without first draining pending backend work. Add a pre-import sync before
stage_import_kv_page.Proposed fix
if (kv == nullptr) { return out_error != nullptr && *out_error != nullptr ? (*out_error)->status : SKIPPY_STATUS_RUNTIME_ERROR; } + session->ctx->synchronize(); + std::string error; if (!kv->stage_import_kv_page(session->seq_id, *desc, input, input_bytes, error)) {🤖 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-resident-prefix-cache-and-session-refinements.patch` around lines 1268 - 1274, Import path needs a pre-write synchronization before mutating native KV tensors in the session import flow. In the function that calls skippy_get_kv_cache and then kv->stage_import_kv_page, insert a sync/drain of pending backend work immediately after the cache lookup succeeds and before any KV page import or tensor writes happen. Use the same synchronization mechanism already used by the export/read path so the import path is consistent and safe.
884-891: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet
n_pastto the imported state length.Using
max(session->n_past, imported_cells)leaves the session ahead after restoring a shorter state, so the next decode can append at the wrong position.Proposed fix
- session->n_past = std::max<int32_t>( - session->n_past, - static_cast<int32_t>(std::min<uint32_t>( - imported_cells, - static_cast<uint32_t>(std::numeric_limits<int32_t>::max())))); + session->n_past = static_cast<int32_t>(std::min<uint32_t>( + imported_cells, + static_cast<uint32_t>(std::numeric_limits<int32_t>::max())));🤖 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-resident-prefix-cache-and-session-refinements.patch` around lines 884 - 891, Update skippy_update_session_state_after_import so it assigns session->n_past from imported_cells instead of taking the max with the existing value; the restore path should reflect the imported state length exactly, with the same clamping to int32_t bounds already present. Keep the fix localized to this helper and ensure any callers relying on restored session position now advance from the imported length rather than the prior session state.
520-526: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid deleting existing KV cells before import allocation succeeds.
seq_rmruns beforefind_slot; if allocation fails, the original page range has already been removed. Preflight capacity or make this path rollback-safe before mutating the cache.🤖 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-resident-prefix-cache-and-session-refinements.patch` around lines 520 - 526, The import path in seq_rm/find_slot is mutating the KV cache too early: existing cells are removed before allocation is confirmed. Update the logic around seq_rm, find_slot, and apply_ubatch so capacity is checked first or the operation is rollback-safe, and only call seq_rm after a slot has been successfully reserved to avoid losing the original page range on failure.third_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patch (5)
1953-1955: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
out_errorbefore returning unsupported for AltUp batch input.This branch returns
SKIPPY_STATUS_UNSUPPORTEDwithout an error message, unlike the surrounding unsupported cases.Proposed fix
float * dst = embd_storage.data() + static_cast<size_t>(i)*n_embd_inp; if ((input_desc->flags & SKIPPY_ACTIVATION_FLAG_GEMMA3N_ALTUP) != 0) { + skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched activation decode does not support Gemma3n AltUp sidebands yet"); return SKIPPY_STATUS_UNSUPPORTED; } else if (n_embd_inp == n_embd) {🤖 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-MTP-execution-support-and-sampling-cleanup.patch` around lines 1953 - 1955, In the AltUp batch-input guard inside the Skippy execution path, the early return from the branch that checks SKIPPY_ACTIVATION_FLAG_GEMMA3N_ALTUP should also populate out_error before returning SKIPPY_STATUS_UNSUPPORTED. Update the same conditional block near the n_embd_inp/n_embd handling so it matches the surrounding unsupported-case behavior by setting a clear error message first, then returning.
1446-1454: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t mutate the MTP sidecar when no draft output is requested.
With
out_mtp_draft == nullptr,skippy_mtp_propose_next()still decodes intomtp_ctxbut never recordsmtp_has_pending_draft, leaving sidecar KV state inconsistent for the next sync.Proposed fix
if (!skippy_mtp_available(session) || predicted_token < 0 || !session->mtp_has_pending_h) { return skippy_success(out_error); } + if (out_mtp_draft == nullptr) { + return skippy_success(out_error); + } llama_context * mtp_ctx = session->stage_model->mtp_ctx;Also applies to: 1857-1861
🤖 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-MTP-execution-support-and-sampling-cleanup.patch` around lines 1446 - 1454, The MTP draft bookkeeping in skippy_mtp_propose_next is updating session state only inside the out_mtp_draft branch, which leaves mtp_has_pending_draft and related sidecar state inconsistent when no draft output is requested. Update skippy_mtp_propose_next so the pending-draft/session state is recorded whenever a draft is actually produced, and avoid mutating the MTP sidecar or leaving stale pending state when out_mtp_draft is null; make the same adjustment in the duplicate location referenced by the patch.
330-330: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPass a DSA
top_kinto MTP attention before enabling GLM-DSA MTP.Line 330 now asserts
top_kis non-null, but the MTP graph’s MLA attention call still passesnullptr, and GLM-DSA aliases this graph. A native MTP draft on GLM-DSA can abort the process; compute/share the indexertop_kfor MTP or gate GLM-DSA MTP off until it exists.Also applies to: 602-604, 799-800
🤖 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-MTP-execution-support-and-sampling-cleanup.patch` at line 330, The MTP MLA attention path now asserts that top_k is available, but the MTP graph still passes nullptr, so GLM-DSA MTP can abort when this alias is used. Update the MTP attention setup in the relevant draft/execution path to compute and pass the DSA indexer top_k instead of nullptr, or disable the GLM-DSA MTP path until that value exists. Make the same fix wherever this MTP/GLM-DSA alias is wired so the GGML_ASSERT(top_k != nullptr) condition is always satisfied.
1735-1738: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject token-only batch decode on non-output runtime slices.
This API has no activation output, but a filtered first slice with
include_output=falsepasses the current checks and then samples from logits that the graph does not produce. Add an unsupported check for filtered non-output stages before decoding.Proposed guard
if (skippy_is_filtered(first) && first->stage_model->config.layer_start > 0) { skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched token decode requires the first runtime slice or a full model"); return SKIPPY_STATUS_UNSUPPORTED; } + if (skippy_is_filtered(first) && !first->stage_model->config.include_output) { + skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched token decode requires an output stage"); + return SKIPPY_STATUS_UNSUPPORTED; + }Also applies to: 1771-1778
🤖 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-MTP-execution-support-and-sampling-cleanup.patch` around lines 1735 - 1738, Add an unsupported guard in the batched token decode path so a filtered first stage with include_output=false is rejected before sampling. In the decode/sampling logic around the existing skippy_is_filtered(first) check, ensure first->stage_model->config.layer_start > 0 or another non-output runtime slice is treated as unsupported and returns SKIPPY_STATUS_UNSUPPORTED via skippy_set_error. Apply the same fix in both affected decode entry points referenced by the diff so token-only batch decode never proceeds when the graph does not produce logits.
1771-1778: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSync or clear MTP state after batched decodes.
The single-request paths call
skippy_mtp_sync_target_tokens(), but these batch loops advancen_pastand token history without updatingmtp_pending_hor sidecar KV. Subsequent MTP drafts can be based on stale hidden state; add a batch-aware sync using the correct row per request, or clear MTP state after each advanced session.Also applies to: 2014-2034
🤖 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-MTP-execution-support-and-sampling-cleanup.patch` around lines 1771 - 1778, The batch decode loop in the skippy session path advances token history but never refreshes or clears MTP state, unlike the single-request flow. Update the batched decode handling around skippy_session, skippy_record_tokens, and skippy_sample_token_ith to either call skippy_mtp_sync_target_tokens for each request using the matching row/index or explicitly clear mtp_pending_h and the sidecar KV after advancing n_past. Keep the fix applied consistently to both affected batch loops so subsequent MTP drafts use current state.third_party/llama.cpp/patches/0006-Expand-staged-execution-across-dense-and-recurrent-f.patch (1)
1264-1268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t silently skip Qwen3VL deepstack injections for mid-stage splits.
When
stage_filtered && il_start > 0, this disablesdeepstack_outfor every layer in the slice. If the slice still containsil < n_deepstack_layers, those projector residuals are omitted and staged activations diverge from full-model execution. Reject splits inside the deepstack range or carry the required sideband through the ABI.🤖 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-Expand-staged-execution-across-dense-and-recurrent-f.patch` around lines 1264 - 1268, The deepstack injection guard in the staged execution path is now skipping `deepstack_out` whenever `stage_filtered` is set and `il_start > 0`, which omits required projector residuals in slices that still cover early layers. Update the loop in the staged execution logic to preserve deepstack additions for any `il < n_deepstack_layers`, or explicitly reject stage splits that start inside the deepstack range; if the split must remain, pass the needed sideband through the staging ABI so `deepstack_out` stays consistent with full-model execution.
🧹 Nitpick comments (1)
third_party/llama.cpp/patches/0009-Add-chat-grammar-device-enumeration-and-runtime-even.patch (1)
512-570: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse a scratch buffer during compaction copies.
This allocates a fresh
std::vectorfor every K/V row copy, and transposed V copies do that per element. Reuse one buffer across calls to avoid allocator churn on large compactions.Proposed refactor
static void llama_kv_cache_copy_tensor_bytes( ggml_tensor * tensor, size_t src_offset, size_t dst_offset, - size_t size) { + size_t size, + std::vector<uint8_t> & scratch) { if (tensor == nullptr || size == 0 || src_offset == dst_offset) { return; } - std::vector<uint8_t> bytes(size); - ggml_backend_tensor_get(tensor, bytes.data(), src_offset, size); - ggml_backend_tensor_set(tensor, bytes.data(), dst_offset, size); + scratch.resize(size); + ggml_backend_tensor_get(tensor, scratch.data(), src_offset, size); + ggml_backend_tensor_set(tensor, scratch.data(), dst_offset, size); } void llama_kv_cache::copy_compacted_cells(const std::vector<compaction_move> & moves) const { + std::vector<uint8_t> scratch; for (const auto & layer : layers) { for (const compaction_move & move : moves) {🤖 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-chat-grammar-device-enumeration-and-runtime-even.patch` around lines 512 - 570, The compaction copy path in llama_kv_cache::copy_compacted_cells allocates a new std::vector<uint8_t> for every tensor slice copy, and the transposed V branch does this per element. Refactor llama_kv_cache_copy_tensor_bytes to take a reusable scratch buffer (or otherwise reuse one buffer across the loop in copy_compacted_cells) so K/V row copies and the v_trans path avoid repeated allocator churn. Keep the fix localized to llama_kv_cache_copy_tensor_bytes and llama_kv_cache::copy_compacted_cells.
🤖 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/0003-Add-staged-sampling-checkpoints-and-part-loading.patch`:
- Around line 279-295: The staged part-loading logic in the tensor-loading loop
is storing `llama_tensor_weight` entries that depend on `ctx_gguf.get()` and
`cur`, but `ctx_gguf` is currently a per-iteration local in the GGUF part
handling code. Update the part-context management so the GGUF contexts created
in this block stay alive for as long as `weights_map` may use them, using the
existing `contexts`/`weights_map` flow around `gguf_init_from_file`,
`weights_map.emplace`, and `llama_tensor_weight` to retain ordered per-part
metadata instead of letting it be destroyed at the end of the loop iteration.
In
`@third_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patch`:
- Around line 1506-1527: Update skippy_abi_features() so it advertises every
implemented capability currently supported by the ABI, including
SKIPPY_FEATURE_CHAT_TEMPLATE_TOOLS and SKIPPY_FEATURE_CHAT_SAMPLING_GRAMMAR, by
adding their bits to the returned feature mask alongside the existing
SKIPPY_FEATURE_GENERATION_SIGNALS and SKIPPY_FEATURE_EXTERNAL_MEDIA_PREFILL
entries. Keep the feature list in sync with the actual APIs exposed in this
patch so clients probing skippy_abi_features() see the new chat-tool and grammar
support as available.
- Around line 1751-1757: In skippy_session_configure_chat_sampling(), the
current catch blocks for std::exception and ... clear sampling and return
success, which hides malformed metadata; change this flow so JSON parsing or
grammar setup failures propagate as an error instead of silently succeeding. Use
the existing out_error and skippy_success/skippy_clear_chat_sampling handling in
this function to return a failure status with a clear invalid chat sampling
metadata message, while keeping the success path unchanged for valid metadata.
- Around line 1538-1569: `stage_model->ctx` and `lane_in_use` are shared across
sessions without synchronization, so lane allocation/freeing and shared-context
operations can race. Add a mutex/lock on the stage model and guard all lane
ownership changes plus every decode/reset/trim path that touches the shared
`llama_context`. Update the affected session methods around
`skippy_finish_model_open` and the later lane-management code referenced by the
review so they acquire the same lock before reading or mutating `lane_in_use` or
calling into `stage_model->ctx`.
In
`@third_party/llama.cpp/patches/0005-Add-resident-prefix-cache-and-session-refinements.patch`:
- Around line 1268-1274: Import path needs a pre-write synchronization before
mutating native KV tensors in the session import flow. In the function that
calls skippy_get_kv_cache and then kv->stage_import_kv_page, insert a sync/drain
of pending backend work immediately after the cache lookup succeeds and before
any KV page import or tensor writes happen. Use the same synchronization
mechanism already used by the export/read path so the import path is consistent
and safe.
- Around line 884-891: Update skippy_update_session_state_after_import so it
assigns session->n_past from imported_cells instead of taking the max with the
existing value; the restore path should reflect the imported state length
exactly, with the same clamping to int32_t bounds already present. Keep the fix
localized to this helper and ensure any callers relying on restored session
position now advance from the imported length rather than the prior session
state.
- Around line 520-526: The import path in seq_rm/find_slot is mutating the KV
cache too early: existing cells are removed before allocation is confirmed.
Update the logic around seq_rm, find_slot, and apply_ubatch so capacity is
checked first or the operation is rollback-safe, and only call seq_rm after a
slot has been successfully reserved to avoid losing the original page range on
failure.
In
`@third_party/llama.cpp/patches/0006-Expand-staged-execution-across-dense-and-recurrent-f.patch`:
- Around line 1264-1268: The deepstack injection guard in the staged execution
path is now skipping `deepstack_out` whenever `stage_filtered` is set and
`il_start > 0`, which omits required projector residuals in slices that still
cover early layers. Update the loop in the staged execution logic to preserve
deepstack additions for any `il < n_deepstack_layers`, or explicitly reject
stage splits that start inside the deepstack range; if the split must remain,
pass the needed sideband through the staging ABI so `deepstack_out` stays
consistent with full-model execution.
In
`@third_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patch`:
- Around line 946-960: The position handling in the external prefill path
silently falls back to n_past when positions is non-null but position_count is
invalid. Update the logic around the pos_storage fill branch to reject malformed
caller-supplied sidebands instead of entering the fallback path, and surface an
error from this decode/prefill function so external media/M-RoPE prefill cannot
proceed with incorrect positions.
- Around line 393-405: The selected-layer export path in
llama_kv_cache::stage_export_kv_page still enters the transposed-V handling even
when a layer has no V stream, which can dereference layer->v_stream[strm]. Add
an early skip or equivalent guard in the v_trans branch alongside the existing
v_row_bytes check so layers with no V stream are ignored in both transposed and
non-transposed paths, and make sure the guard applies to the loops that build V
payloads for selected layers.
- Around line 28-95: The staged-runtime ABI needs to be bumped to account for
the new public exports added here. Update the SKIPPY_ABI_VERSION constant(s)
alongside the new skippy_session_begin_external_decode,
skippy_session_end_external_decode, skippy_prefill_chunk_frame_sampled,
skippy_prefill_chunk_frame_with_positions, and
skippy_prefill_chunk_frame_sampled_with_positions declarations, and make sure
the Rust FFI-side ABI constants remain in sync with the C header.
In
`@third_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patch`:
- Around line 1953-1955: In the AltUp batch-input guard inside the Skippy
execution path, the early return from the branch that checks
SKIPPY_ACTIVATION_FLAG_GEMMA3N_ALTUP should also populate out_error before
returning SKIPPY_STATUS_UNSUPPORTED. Update the same conditional block near the
n_embd_inp/n_embd handling so it matches the surrounding unsupported-case
behavior by setting a clear error message first, then returning.
- Around line 1446-1454: The MTP draft bookkeeping in skippy_mtp_propose_next is
updating session state only inside the out_mtp_draft branch, which leaves
mtp_has_pending_draft and related sidecar state inconsistent when no draft
output is requested. Update skippy_mtp_propose_next so the pending-draft/session
state is recorded whenever a draft is actually produced, and avoid mutating the
MTP sidecar or leaving stale pending state when out_mtp_draft is null; make the
same adjustment in the duplicate location referenced by the patch.
- Line 330: The MTP MLA attention path now asserts that top_k is available, but
the MTP graph still passes nullptr, so GLM-DSA MTP can abort when this alias is
used. Update the MTP attention setup in the relevant draft/execution path to
compute and pass the DSA indexer top_k instead of nullptr, or disable the
GLM-DSA MTP path until that value exists. Make the same fix wherever this
MTP/GLM-DSA alias is wired so the GGML_ASSERT(top_k != nullptr) condition is
always satisfied.
- Around line 1735-1738: Add an unsupported guard in the batched token decode
path so a filtered first stage with include_output=false is rejected before
sampling. In the decode/sampling logic around the existing
skippy_is_filtered(first) check, ensure first->stage_model->config.layer_start >
0 or another non-output runtime slice is treated as unsupported and returns
SKIPPY_STATUS_UNSUPPORTED via skippy_set_error. Apply the same fix in both
affected decode entry points referenced by the diff so token-only batch decode
never proceeds when the graph does not produce logits.
- Around line 1771-1778: The batch decode loop in the skippy session path
advances token history but never refreshes or clears MTP state, unlike the
single-request flow. Update the batched decode handling around skippy_session,
skippy_record_tokens, and skippy_sample_token_ith to either call
skippy_mtp_sync_target_tokens for each request using the matching row/index or
explicitly clear mtp_pending_h and the sidecar KV after advancing n_past. Keep
the fix applied consistently to both affected batch loops so subsequent MTP
drafts use current state.
---
Nitpick comments:
In
`@third_party/llama.cpp/patches/0009-Add-chat-grammar-device-enumeration-and-runtime-even.patch`:
- Around line 512-570: The compaction copy path in
llama_kv_cache::copy_compacted_cells allocates a new std::vector<uint8_t> for
every tensor slice copy, and the transposed V branch does this per element.
Refactor llama_kv_cache_copy_tensor_bytes to take a reusable scratch buffer (or
otherwise reuse one buffer across the loop in copy_compacted_cells) so K/V row
copies and the v_trans path avoid repeated allocator churn. Keep the fix
localized to llama_kv_cache_copy_tensor_bytes and
llama_kv_cache::copy_compacted_cells.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a33c6fd5-ea88-45ec-a0ee-a79af157ee36
📒 Files selected for processing (11)
third_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patchthird_party/llama.cpp/patches/0002-Add-early-staged-model-family-and-chat-support.patchthird_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patchthird_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patchthird_party/llama.cpp/patches/0005-Add-resident-prefix-cache-and-session-refinements.patchthird_party/llama.cpp/patches/0006-Expand-staged-execution-across-dense-and-recurrent-f.patchthird_party/llama.cpp/patches/0007-Expand-staged-execution-across-VL-and-broad-model-fa.patchthird_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patchthird_party/llama.cpp/patches/0009-Add-chat-grammar-device-enumeration-and-runtime-even.patchthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patchthird_party/llama.cpp/upstream.txt
ndizazzo
approved these changes
Jun 30, 2026
|
|
||
| - name: SDK and API crate tests | ||
| if: ${{ matrix.group == 'sdk-api' && (needs.changes.outputs.all_rust == 'true' || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-client') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-api-client') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-api-server') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-config') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-commands') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-events') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-hardware-profile') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-runtime-install') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-sdk') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-cli') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-tui') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-embedded-runtime') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-console-server') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-ffi') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-nodejs')) }} | ||
| if: ${{ matrix.group == 'sdk-api' && (needs.changes.outputs.all_rust == 'true' || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-client') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-api-client') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-api-server') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-config') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-commands') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-events') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-hardware-profile') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-runtime-install') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-native-runtime') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-routing') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-types') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-sdk') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-cli') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-tui') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-embedded-runtime') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-console-server') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-ffi') || contains(fromJson(needs.changes.outputs.affected_crates || '[]'), 'mesh-llm-nodejs')) }} |
Collaborator
There was a problem hiding this comment.
Looks like this change is getting included multiple time across PRs
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
2 participants
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
86b94708f22478f900b76ca02e316f4f3418faffValidation
LLAMA_WORKDIR=$(mktemp -d /tmp/mesh-llm-llama.XXXXXX)+scripts/prepare-llama.sh pinnedpassed in a clean temporary checkoutjust buildcompiled patched llama.cpp, the UI, and the Rust debug binary; the wrapper then hung in local macOS keychain trust repair forMesh-LLM Local Codesign, so I interrupted only that signing/trust stepSummary by CodeRabbit
New Features
Bug Fixes
Chores