Add Inkling Q2 staged runtime and certify text split serving - #1017
Add Inkling Q2 staged runtime and certify text split serving#1017michaelneale wants to merge 67 commits into
Conversation
📝 WalkthroughWalkthroughThis change adds Inkling model conversion and native execution support, including multimodal projectors, MTP sidecars, activation payloads, split-runtime coordination, package inventory metadata, validation commands, server transport updates, capability records, and certification documentation. ChangesInkling packaging and conversion
Mesh runtime and server execution
Capability records and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
Placement dependency: this Inkling work should follow the final-stage native-MTP placement and exact-byte planning work in #938 rather than introduce a separate placement path. In particular, the external 15.5 GB Inkling sidecar should be charged to and loaded by the final/logits-owning stage, with Q2 layer allocation adjusted around that fixed cost. We will keep #1017 in draft and wait for #938 to settle before finalizing this integration. @i386 please flag any manifest, planner, or stage-load interfaces here that should align with the GLM work. |
Test plan (how I'll validate this once it's ready to try)Recording the intended plan of attack so it's clear how Inkling Q2 + MTP will be Topologies (box mixture + latency regimes)Inkling Q2 is large, so it must split. I'll validate across three latency
Mixed backends will be covered too (Metal coordinator + CUDA worker) since that Steps per topology
What "good" looks like
Happy to run this once the draft is far enough along; will report evidence |
…ex/inkling-q2-skippy Conflict resolution notes: - llama.cpp patch queue: renumbered Inkling patches 0017-0023 to 0022-0028 after main's 0018-0021 (ngram cache ABI, checkpoint removal, MTP re-prime, ngram confidence fix). SKIPPY_FEATURE_INKLING_MTP_MM moved 1<<26 -> 1<<27 (main took 1<<26 for FEATURE_NGRAM_CACHE_DRAFT); ABI patch bumped 31 -> 32 in patch 0027 and the skippy-ffi mirror. - skippy-topology: inkling family expectation moved into main's new family_capability.rs. - skippy-runtime: multimodal text-chunk token capture re-applied to main's extracted media.rs/types.rs. - model-artifact: per-layer kv_head_counts pricing ported into main's new gguf/kv_cache.rs (glm-dsa absorbed-MLA case preserved). - skippy-quantize: branch's tensor_stream.rs folded into gguf_writer.rs (AlternatingRows joined main's TensorTransform in glm_dsa.rs); Q2_0 now FFI-supported so the pinned-catalog test exemption is removed. - host-runtime: audio+vision projector evidence helper re-applied to main's restructured local.rs and local_split/loading.rs.
Rebased patches 0022-0028 onto main's 0018-0021 (ngram cache ABI, checkpoint removal, MTP re-prime, ngram confidence fix) so the full queue applies cleanly: - Replay-multi-depth-MTP hunk re-anchored around main's chain_heads sync path in skippy_mtp_sync_target_tokens. - SKIPPY_FEATURE_INKLING_MTP_MM moved to 1<<27 next to main's NGRAM_CACHE_DRAFT (1<<26); abi_features advertises both. - ABI patch 31 -> 32 (mirrored in skippy-ffi). Full just build (prepare + patch queue + native ABI + UI + binary) passes end-to-end.
- parser.rs test module: import Command and ModelsCommand (the merge left the new experimental-publication test without them). - CLI.md: document the models package --experimental flag so the cli-docs-sync gate sees the CLI surface change reflected in public docs.
Standalone N-gram plans (no native MTP) ran the serial propose->send->wait->commit loop, one verify window per round trip, regardless of verify_window_pipeline_depth. The composite verify-window pipeline machinery already handles pure-N-gram proposals (native_mtp_token_count == 0), so route standalone drafting through it. - NativeMtpDecodeOptions gains ngram_proposals_enabled (the provider should produce N-gram tokens for a standalone plan as well as an MTP composite), and ngram_max_proposal_tokens now falls back to the proposer's own limit when there is no extension policy. CompositeProposalProvider keys `enabled` off the new flag. - embedded_generation enables the pipeline for a standalone N-gram plan at verify_window_pipeline_depth > 1 (standalone_ngram_pipelining), while the single native-MTP verify-window path stays composite-only. The serial block is gated off when the pipeline is active so a request is drafted by exactly one path; depth 1 keeps the proven serial path. Opt-in: default depth is 1 (serial, unchanged). Pipelining trades extra speculative work for latency hiding, so it only pays off at high RTT; on loopback it is pure overhead. Local 2-stage split confirms the mechanism engages: at depth 3, verify_window_max_in_flight goes 0 -> 3 and occupancy_parallel_fraction 0 -> 0.81 on a standalone suffix re-emit, output correct, accept 0.92. WAN-lab RTT sweep is the remaining performance validation before this is worth turning on by default.
* origin/main: Add Ngram Suffix Proposer (#1037)
…n ceiling for long-haul WAN experiments
|
🤖 The streaming "empty deltas" issue is not a streaming bug and not from this branch. Reproduced locally on Qwen3-0.6B (different model family, no GPU needed):
60 tokens were generated and dropped on both paths, so the streaming layer is fine. The chat-output parser returns nothing when a thinking model hits Not introduced here: Reclassifying as a pre-existing parser bug affecting any thinking model that exhausts its budget mid-reason. Filing separately against main; not a blocker for this PR. |
* origin/main: Hand npm publishing to mesh-packaging feat(runtime): add daemon model lifecycle reconciliation (#1082) Accept bounded external token proposals in local Skippy generation (#1081) Expose target-authoritative local generation receipts (#1080) # Conflicts: # crates/mesh-llm-host-runtime/src/runtime/model_lifecycle.rs
Assisted-by: goose
* origin/main: fix: use published mesh-llm-hf-hub crate (#1096)
michaelneale
changed the title
|
Ready for review at The merge claim is deliberately narrow: Inkling Q2 text generation ran through a real three-stage M5 Metal + Australian Vast CUDA topology over Iroh/QUIC. The completed 12-request mixed-workload soak was 12/12 HTTP 200 with no stage failure. Deep suffix N-gram reached a 35.86 tok/s median on repeated/copy-shaped requests and 14.92 tok/s on novel controls; it is effective latency hiding for repetition, not a universal speedup. This does not claim production tool-call quality, multimodal generation, native Inkling MTP, the full 1,048,576-token context, multi-lane serving, arbitrary shared-prefix cache hits, or clean Metal shutdown. The generic hosted-stage teardown failure is tracked in #1103. The PR body contains the complete evidence and limits. @i386 @ndizazzo, review of the Inkling architecture/patch queue and staged recurrent-state handling would be particularly useful. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-host-runtime/src/models/capabilities.rs (1)
81-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDowngrade audio capability when the audio projector is absent.
audio_projector_loadedcomes from the same GGUF projector scan used for vision, but the verified-capability path only demotes vision when its projector is missing. Whenaudio_projector_loadedis false, a static name/config signal foraudio: Supportedis preserved, so advertise only the audio capability actually present in the loaded projector.🤖 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 `@crates/mesh-llm-host-runtime/src/models/capabilities.rs` around lines 81 - 98, Update runtime_verified_capabilities_from_static to explicitly set caps.audio to CapabilityLevel::None when evidence.audio_projector_loaded is false, while preserving the existing multimodal consistency behavior and promoting audio when the projector is loaded. Ensure static audio signals cannot survive verification without the corresponding projector.
🧹 Nitpick comments (7)
crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs (1)
240-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider importing
FamilyPolicyinstead of repeating thesuper::super::family_policy::FamilyPolicypath.Both
resolve_cache_type_kandresolve_cache_type_vspell out the full relative path for the type. Auseat the top of the file (or a local type alias) would be more readable and easier to keep in sync if the module ever moves.♻️ Suggested cleanup
-fn resolve_cache_type_k( - context: &ResolverContext<'_>, - kv: &KvDefaults, - kv_policy: KvCachePolicy, - family_policy: &super::super::family_policy::FamilyPolicy, -) -> String { +fn resolve_cache_type_k( + context: &ResolverContext<'_>, + kv: &KvDefaults, + kv_policy: KvCachePolicy, + family_policy: &FamilyPolicy, +) -> String {(with
use super::super::family_policy::FamilyPolicy;added near the top of the file, applying the same change toresolve_cache_type_vandresolve_model_fit_config.)Also applies to: 270-271
🤖 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 `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs` around lines 240 - 241, Import FamilyPolicy near the top of the module, then replace the repeated super::super::family_policy::FamilyPolicy paths in resolve_cache_type_k, resolve_cache_type_v, and resolve_model_fit_config with the imported type.crates/skippy-server/src/binary_transport/binary_messaging/connection.rs (1)
198-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBundle the parameters instead of silencing the lint.
The split into
handle_binary_connection_messagespushed the signature to 20 parameters and required a bare#[allow(clippy::too_many_arguments)]with no justification. Group the invariant transport/telemetry knobs (activation_width,wire_dtype,max_inflight,reply_credit_limit,async_prefill_forward,downstream_wire_condition,downstream_connect_timeout_secs,native_mtp_enabled) into a context struct, or at minimum document why the allow is needed.As per coding guidelines: "do not use
#[allow(...)]to silence warnings without a clear reason and developer approval".🤖 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 `@crates/skippy-server/src/binary_transport/binary_messaging/connection.rs` around lines 198 - 219, Remove the unjustified #[allow(clippy::too_many_arguments)] from handle_binary_connection_messages by grouping the invariant transport and telemetry parameters into a dedicated context struct and passing that struct to the function. Update the call sites and field accesses for activation_width, wire_dtype, max_inflight, reply_credit_limit, async_prefill_forward, downstream_wire_condition, downstream_connect_timeout_secs, and native_mtp_enabled, preserving their existing values and behavior.Source: Coding guidelines
crates/skippy-topology/src/tests.rs (1)
953-965: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new fallback inference path separately.
This identity is the newly added reviewed JSON record, so
infer_family_capabilitycan return before reaching the Inkling branch infamily_capability.rsat Lines [1109-1111]. Add a second identity absent from the reviewed catalog to verifyinkling_capabilityitself and catch regressions in the new fallback path.🤖 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 `@crates/skippy-topology/src/tests.rs` around lines 953 - 965, Extend the Inkling tests around infer_family_capability with a second, unreviewed identity that is absent from the reviewed catalog, ensuring it reaches and validates inkling_capability directly. Keep the existing reviewed JSON record assertions unchanged, and verify the fallback result’s family, wire dtype, validation, state mobility, and recurrent range behavior.crates/mesh-llm-host-runtime/src/runtime/local.rs (1)
520-520: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClarify
vram_bytesas local-fit capacity in the planner docs.
local_runtime_capacity_bytes()is a broader local budget that may include CPU offload memory, whilevram_bytesis documented as accelerator-resident capacity. Since this path can pass the former toRuntimeResourcePlanInput, update the planner docs or introduce a separate input for CPU-aware budgeting so future callers don’t misuse accelerator VRAM planning for CPU-inclusive capacity.**🤖 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 `@crates/mesh-llm-host-runtime/src/runtime/local.rs` at line 520, Clarify the planner contract around RuntimeResourcePlanInput.vram_bytes and the local.rs fallback using local_runtime_capacity_bytes(): document that vram_bytes must represent accelerator-resident capacity, or introduce a distinct CPU-aware capacity input for the broader local budget. Ensure callers cannot treat CPU-inclusive local capacity as VRAM capacity.crates/model-package/src/bin/queue-unsloth-layer-packages.rs (2)
792-799: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo implementations of source-total sizing with different overflow semantics. The planning path saturates while the job-spec path errors, so an overflow would produce a plan and a spec that disagree — and the duplication invites drift as projector accounting evolves.
crates/model-package/src/bin/queue-unsloth-layer-packages.rs#L792-L799: makecandidate_source_total_bytesthe single source of truth (returningResult<u64>with the checked arithmetic and overflow context).crates/model-package/src/bin/queue-unsloth-layer-packages.rs#L1100-L1111: replace the inlinetry_fold/checked_addblock with a call tocandidate_source_total_bytes(candidate)?.🤖 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 `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs` around lines 792 - 799, Update candidate_source_total_bytes to return Result<u64>, use checked arithmetic for quant and projector totals, and preserve the existing overflow context in its error. In crates/model-package/src/bin/queue-unsloth-layer-packages.rs:792-799, make this function the single sizing implementation; at crates/model-package/src/bin/queue-unsloth-layer-packages.rs:1100-1111, replace the inline try_fold/checked_add logic with candidate_source_total_bytes(candidate)?.
690-698: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
expecton a cross-function invariant.The pipeline tag is only guaranteed by
model_split_compatibility~180 lines away; a future compatibility branch that doesn't require a tag turns this into a panic in unattended queue automation. Prefer the same skip path used just above.♻️ Skip instead of panicking
- let source_pipeline_tag = - model_pipeline_tag(&source_info).expect("compatible model must have a pipeline tag"); + let Some(source_pipeline_tag) = model_pipeline_tag(&source_info) else { + eprintln!("skip {}: source repo info has no pipeline tag", model.repo_id); + return Ok(None); + };🤖 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 `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs` around lines 690 - 698, Replace the expect call on model_pipeline_tag in the package queue flow with an Option-aware skip path matching the source_revision branch above. When no pipeline tag is returned, log an appropriate skip message including model.repo_id and return Ok(None); preserve the existing tag-based processing for Some values.crates/skippy-quantize/src/gguf_writer.rs (1)
431-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the forced-F32 override.
The override silently ignores
output_typeforattn_rel_proj.weightand.shortconv_tensors. A one-line comment stating the runtime requirement would keep future dtype work from "fixing" this.🤖 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 `@crates/skippy-quantize/src/gguf_writer.rs` around lines 431 - 441, The forced F32 branches in target_dtype_for_mapped_tensor must be documented with a one-line comment explaining the runtime requirement and that they intentionally override output_type. Add the comment immediately before the mapped-name condition without changing the existing dtype behavior.
🤖 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 `@crates/mesh-llm-commands/src/model_package.rs`:
- Around line 197-199: The submit JSON payload in the model package command must
include the same experimental release-channel flag reported by the dry-run
payload. Update the payload construction near sourceRevision, sourceFile, and
projectors to add the existing experimental value under the "experimental"
field, preserving the current dry-run behavior.
In `@crates/mesh-llm-host-runtime/src/mesh/direct_path.rs`:
- Around line 223-241: Update record_draining_replaced_connection to reuse the
bounded drain-and-close helper used by remember_incoming_connection instead of
only logging the replacement. Expose that helper from connections.rs with crate-
or module-level visibility, then invoke it for the existing connection so
retained stream/dispatch clones receive an explicit terminal close after the
configured grace period; preserve the current logging context.
In `@crates/mesh-llm-host-runtime/src/runtime/local_package.rs`:
- Around line 445-472: Update collect_split_participant_membership to apply
split_peer_stage_path_exclusion_reason alongside
split_peer_preflight_exclusion_reason when evaluating each peer. Record
stage-path rejections in excluded and omit those peers from participants,
keeping the membership snapshot’s eligibility consistent with
collect_split_participants and topology planning.
In `@crates/mesh-llm-host-runtime/src/runtime/local.rs`:
- Around line 621-623: Update the startup paths in the async functions around
runtime_media_capability_evidence (including both direct-GGUF and layer-package
flows) to execute the projector metadata scan through
tokio::task::spawn_blocking. Await the blocking task and preserve the existing
capability evidence result and error handling while ensuring
scan_gguf_projector_meta does not run on Tokio worker threads.
In `@crates/skippy-correctness/src/runner/stage_fa_parity.rs`:
- Around line 9-48: Guard stage_fa_parity against invalid comparison ranges and
payloads before computing parity metrics. Validate that the decoded activation
range is non-empty and that enabled_values and disabled_values have equal,
nonzero lengths; return an error otherwise. Update the comparison flow around
decode_boundary, payload_f32, and the RMS calculation so tolerance checks only
run for valid matching data.
In `@crates/skippy-quantize/src/projector_validate.rs`:
- Around line 39-51: Update the projector validation flow around
mtmd_init_from_file to load and pass a valid llama_model pointer as its
text_model argument instead of std::ptr::null(). Ensure the model is initialized
before the call and properly released afterward, while preserving the existing
null-result validation and projector cleanup behavior.
In `@crates/skippy-quantize/src/tokenizer_metadata.rs`:
- Around line 287-296: Update the inkling_mm_model branch in tokenizer metadata
conversion to accept config.eos_token_id only when it meets the reference
converter’s minimum floor of 199998; otherwise fall back to 200006. Apply this
validated EOS value consistently to both tokenizer.ggml.eos_token_id and
tokenizer.ggml.bos_token_id while preserving the existing metadata flags and
return flow.
In `@crates/skippy-server/src/binary_transport/stage_execution.rs`:
- Around line 79-89: Update the stage execution retry loop and
take_warm_or_connect_downstream to pass the current remaining Duration into
connection establishment, then recompute remaining time after connecting before
calling complete_downstream_ready so all connect and handshake work shares the
absolute deadline. Preserve timeout and retry behavior, and add a regression
test covering a one-second configured timeout to verify the overall wait does
not exceed the deadline, including retry sleep.
In `@scripts/hf-skippy-mtp-certify-job.py`:
- Around line 22-36: Update ensure_build_tools to check shutil.which("apt-get")
before invoking apt-get, matching the guard in hf-skippy-convert-job.py. Raise
the same clear error when apt-get is unavailable, while preserving the existing
dependency detection and installation flow.
- Around line 73-83: Update projector_path to parse and validate
args.projector_url before calling urllib.request.urlopen, allowing only HTTP and
HTTPS schemes and rejecting all others before creating or writing the temporary
projector file.
In `@third_party/llama.cpp/patches/0046-Add-TML-Inkling-architecture.patch`:
- Around line 2032-2034: Remove the out-of-bounds memcpy writing rel_extent in
the operation setup near ggml_set_op_params; rely on rel->ne[0] or
rel_logits->ne[0] for the banded attention paths, preserving the existing scale
parameter initialization.
In
`@third_party/llama.cpp/patches/0053-Restore-recurrent-verify-window-state.patch`:
- Around line 194-201: Guard the accepted_count > 0 branch in the checkpoint
verification flow before calling skippy_decode_tokens: validate that
checkpoint.token_ids is non-empty, and explicitly fail using the existing
out_error/status mechanism when it is empty. Only pass
checkpoint.token_ids.data() with a positive count when the vector contains the
required tokens.
In `@tools/relay-fly-legacy/README.md`:
- Around line 10-13: Update the four relay URLs in the README table to remove
the trailing dot after “iroh.link”, changing each hostname from “iroh.link.” to
“iroh.link” while preserving the rest of every endpoint.
---
Outside diff comments:
In `@crates/mesh-llm-host-runtime/src/models/capabilities.rs`:
- Around line 81-98: Update runtime_verified_capabilities_from_static to
explicitly set caps.audio to CapabilityLevel::None when
evidence.audio_projector_loaded is false, while preserving the existing
multimodal consistency behavior and promoting audio when the projector is
loaded. Ensure static audio signals cannot survive verification without the
corresponding projector.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rs`:
- Around line 240-241: Import FamilyPolicy near the top of the module, then
replace the repeated super::super::family_policy::FamilyPolicy paths in
resolve_cache_type_k, resolve_cache_type_v, and resolve_model_fit_config with
the imported type.
In `@crates/mesh-llm-host-runtime/src/runtime/local.rs`:
- Line 520: Clarify the planner contract around
RuntimeResourcePlanInput.vram_bytes and the local.rs fallback using
local_runtime_capacity_bytes(): document that vram_bytes must represent
accelerator-resident capacity, or introduce a distinct CPU-aware capacity input
for the broader local budget. Ensure callers cannot treat CPU-inclusive local
capacity as VRAM capacity.
In `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs`:
- Around line 792-799: Update candidate_source_total_bytes to return
Result<u64>, use checked arithmetic for quant and projector totals, and preserve
the existing overflow context in its error. In
crates/model-package/src/bin/queue-unsloth-layer-packages.rs:792-799, make this
function the single sizing implementation; at
crates/model-package/src/bin/queue-unsloth-layer-packages.rs:1100-1111, replace
the inline try_fold/checked_add logic with
candidate_source_total_bytes(candidate)?.
- Around line 690-698: Replace the expect call on model_pipeline_tag in the
package queue flow with an Option-aware skip path matching the source_revision
branch above. When no pipeline tag is returned, log an appropriate skip message
including model.repo_id and return Ok(None); preserve the existing tag-based
processing for Some values.
In `@crates/skippy-quantize/src/gguf_writer.rs`:
- Around line 431-441: The forced F32 branches in target_dtype_for_mapped_tensor
must be documented with a one-line comment explaining the runtime requirement
and that they intentionally override output_type. Add the comment immediately
before the mapped-name condition without changing the existing dtype behavior.
In `@crates/skippy-server/src/binary_transport/binary_messaging/connection.rs`:
- Around line 198-219: Remove the unjustified
#[allow(clippy::too_many_arguments)] from handle_binary_connection_messages by
grouping the invariant transport and telemetry parameters into a dedicated
context struct and passing that struct to the function. Update the call sites
and field accesses for activation_width, wire_dtype, max_inflight,
reply_credit_limit, async_prefill_forward, downstream_wire_condition,
downstream_connect_timeout_secs, and native_mtp_enabled, preserving their
existing values and behavior.
In `@crates/skippy-topology/src/tests.rs`:
- Around line 953-965: Extend the Inkling tests around infer_family_capability
with a second, unreviewed identity that is absent from the reviewed catalog,
ensuring it reaches and validates inkling_capability directly. Keep the existing
reviewed JSON record assertions unchanged, and verify the fallback result’s
family, wire dtype, validation, state mobility, and recurrent range behavior.
🪄 Autofix (Beta)
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: 02055626-4bcf-4045-b0bc-f23de6a22719
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (114)
.github/workflows/queue-unsloth-layer-packages.ymlJustfilecrates/llama-quant-ffi/src/lib.rscrates/mesh-llm-cli/src/models.rscrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-commands/src/model_package.rscrates/mesh-llm-host-runtime/src/inference/skippy/family_policy.rscrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/mesh/capacity.rscrates/mesh-llm-host-runtime/src/mesh/connections.rscrates/mesh-llm-host-runtime/src/mesh/direct_path.rscrates/mesh-llm-host-runtime/src/mesh/mod.rscrates/mesh-llm-host-runtime/src/mesh/node.rscrates/mesh-llm-host-runtime/src/mesh/peer_state.rscrates/mesh-llm-host-runtime/src/mesh/stage_artifacts.rscrates/mesh-llm-host-runtime/src/mesh/stage_transport.rscrates/mesh-llm-host-runtime/src/mesh/tests/connections.rscrates/mesh-llm-host-runtime/src/mesh/tests/direct_path.rscrates/mesh-llm-host-runtime/src/models/capabilities.rscrates/mesh-llm-host-runtime/src/models/mod.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local_package.rscrates/mesh-llm-host-runtime/src/runtime/local_split.rscrates/mesh-llm-host-runtime/src/runtime/local_split/loading.rscrates/mesh-llm-host-runtime/src/runtime/local_split/tests.rscrates/mesh-llm-host-runtime/src/runtime/mod.rscrates/mesh-llm-host-runtime/src/runtime/model_lifecycle/load.rscrates/mesh-llm-host-runtime/src/runtime/model_lifecycle/reconciliation.rscrates/mesh-llm-host-runtime/src/runtime/split_participant_settle.rscrates/mesh-llm-host-runtime/src/runtime/split_planning.rscrates/mesh-llm-host-runtime/src/runtime/startup_handles.rscrates/mesh-llm-host-runtime/src/runtime/startup_retry.rscrates/mesh-llm-host-runtime/src/sdk.rscrates/mesh-llm-types/src/mesh/mod.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/commands/models/mod.rscrates/model-artifact/src/gguf.rscrates/model-artifact/src/gguf/kv_cache.rscrates/model-package/src/bin/queue-unsloth-layer-packages.rscrates/model-package/src/jobs.rscrates/model-package/src/prepare.rscrates/model-package/src/script.rscrates/model-package/src/scripts/split-model-job.shcrates/skippy-coordinator/src/topology.rscrates/skippy-correctness/src/cli.rscrates/skippy-correctness/src/main.rscrates/skippy-correctness/src/runner/mod.rscrates/skippy-correctness/src/runner/stage_fa_parity.rscrates/skippy-ffi/src/lib.rscrates/skippy-protocol/src/binary/activation.rscrates/skippy-protocol/src/binary/mod.rscrates/skippy-protocol/src/binary/types.rscrates/skippy-quantize/Cargo.tomlcrates/skippy-quantize/src/backend.rscrates/skippy-quantize/src/gguf_metadata.rscrates/skippy-quantize/src/gguf_template.rscrates/skippy-quantize/src/gguf_writer.rscrates/skippy-quantize/src/gguf_writer/glm_dsa.rscrates/skippy-quantize/src/gguf_writer_tests.rscrates/skippy-quantize/src/hf_checkpoint.rscrates/skippy-quantize/src/inkling_metadata.rscrates/skippy-quantize/src/main.rscrates/skippy-quantize/src/mtp_attach.rscrates/skippy-quantize/src/projector_validate.rscrates/skippy-quantize/src/tensor_map.rscrates/skippy-quantize/src/tokenizer_metadata.rscrates/skippy-quantize/src/types.rscrates/skippy-runtime/src/media.rscrates/skippy-runtime/src/runtime_events.rscrates/skippy-runtime/src/types.rscrates/skippy-server/src/binary_transport/binary_messaging.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/binary_messaging/reply.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/stage_execution.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend/decode_scheduler.rscrates/skippy-server/src/frontend/embedded_execution.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/embedded_generation/lifecycle.rscrates/skippy-server/src/frontend/generation.rscrates/skippy-server/src/frontend/generation/persistent_lanes.rscrates/skippy-server/src/frontend/generation/timeouts.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/request.rscrates/skippy-server/src/frontend/tests/prefill.rscrates/skippy-server/src/frontend/tests/request.rscrates/skippy-server/src/frontend/tests/wire_messages.rscrates/skippy-server/src/frontend/wire_messages.rscrates/skippy-server/src/runtime_state.rscrates/skippy-topology/capabilities/reviewed-family-capabilities.jsoncrates/skippy-topology/src/family_capability.rscrates/skippy-topology/src/tests.rsdocs/LAYER_PACKAGE_REPOS.mddocs/skippy/llama-parity-candidates.jsonscripts/hf-skippy-convert-job.pyscripts/hf-skippy-mtp-certify-job.pythird_party/llama.cpp/patches/0046-Add-TML-Inkling-architecture.patchthird_party/llama.cpp/patches/0047-Add-staged-execution-support-for-Inkling.patchthird_party/llama.cpp/patches/0048-Advance-native-MTP-depth-per-draft-step.patchthird_party/llama.cpp/patches/0049-Replay-multi-depth-MTP-caches-correctly.patchthird_party/llama.cpp/patches/0050-Add-Inkling-multi-depth-MTP-sidecars.patchthird_party/llama.cpp/patches/0051-Carry-multimodal-Inkling-embeddings-into-MTP.patchthird_party/llama.cpp/patches/0052-Size-recurrent-memory-for-appended-MTP-layers.patchthird_party/llama.cpp/patches/0053-Restore-recurrent-verify-window-state.patchthird_party/llama.cpp/patches/0054-skippy-preserve-verify-checkpoint-until-trim.patchthird_party/llama.cpp/patches/0055-skippy-retain-pipelined-verify-checkpoints.patchthird_party/llama.cpp/patches/0056-skippy-filter-Inkling-stage-memory-by-layer-range.patchthird_party/llama.cpp/patches/0057-Complete-Inkling-and-GLM-metadata-integration.patchthird_party/llama.cpp/patches/0058-skippy-balance-filtered-stages-across-devices.patchtools/relay-fly-legacy/README.mdwebsite/src/docs/pages/CLI.md
| record_draining_replaced_connection(remote, existing.as_ref(), &conn); | ||
| } | ||
| } | ||
|
|
||
| fn record_draining_replaced_connection( | ||
| remote: EndpointId, | ||
| existing: Option<&Connection>, | ||
| replacement: &Connection, | ||
| ) { | ||
| let Some(existing) = existing else { | ||
| return; | ||
| }; | ||
| tracing::debug!( | ||
| peer = %remote.fmt_short(), | ||
| replaced_stable_id = existing.stable_id(), | ||
| replacement_stable_id = replacement.stable_id(), | ||
| "Direct path connection replaced; allowing existing streams to drain" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replaced direct-path connection now never gets a terminal close.
The helper only logs, so once existing is dropped from state.connections the old QUIC connection is retained by whatever stream/dispatch tasks still hold clones and is only reclaimed by idle timeout. remember_incoming_connection in crates/mesh-llm-host-runtime/src/mesh/connections.rs (Lines 705-736) solves the identical problem with a bounded drain grace plus explicit close. Reuse that path here instead of an unbounded drain.
♻️ Suggested change
- record_draining_replaced_connection(remote, existing.as_ref(), &conn);
+ if let Some(existing) = existing.filter(|existing| existing.stable_id() != conn.stable_id())
+ {
+ record_draining_replaced_connection(remote, Some(&existing), &conn);
+ Self::spawn_replaced_connection_drain(remote, existing);
+ }Note the drain helper is currently private to connections.rs; it needs pub(crate)/pub(super) visibility.
🤖 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 `@crates/mesh-llm-host-runtime/src/mesh/direct_path.rs` around lines 223 - 241,
Update record_draining_replaced_connection to reuse the bounded drain-and-close
helper used by remember_incoming_connection instead of only logging the
replacement. Expose that helper from connections.rs with crate- or module-level
visibility, then invoke it for the existing connection so retained
stream/dispatch clones receive an explicit terminal close after the configured
grace period; preserve the current logging context.
| + float params[] = { scale, 0.0f, 0.0f }; | ||
| + ggml_set_op_params(result, params, sizeof(params)); | ||
| + memcpy(result->op_params + 16, &rel_extent, sizeof(rel_extent)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm GGML_MAX_OP_PARAMS and op_params declaration in the vendored tree,
# and check whether anything reads rel_extent back from op_params.
fd -t f 'ggml.h$' third_party --exec rg -n 'GGML_MAX_OP_PARAMS|int32_t op_params' {}
rg -n 'op_params \+ 16|rel_extent' third_party/llama.cpp/patches | head -50Repository: Mesh-LLM/mesh-llm
Length of output: 8032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ggml.h under third_party and print GGML_MAX_OP_PARAMS / op_params declarations.
printf 'ggml.h GGML_MAX_OP_PARAMS and op_params declarations:\n'
fd -t f 'ggml.h$' third_party --print0 | while IFS= read -r -d '' f; do
echo "--- $f"
rg -n -C 3 'GGML_MAX_OP_PARAMS|int32_t op_params' "$f" || true
done
printf '\nRelevant patch region:\n'
sed -n '2010,2045p' third_party/llama.cpp/patches/0046-Add-TML-Inkling-architecture.patch
printf '\nAll rel_extent occurrences in patch (no source code execution):\n'
rg -n 'rel_extent|REL_EXTENT|INKLING_REL_EXTENT' third_party/llama.cpp/patches/0046-Add-TML-Inkling-architecture.patch
printf '\nStruct/constructor declaration context for ggml_inkling_rel_logits:\n'
rg -n -C 5 'ggml_inkling_rel_logits|rel_logits' third_party/llama.cpp/patches/0046-Add-TML-Inkling-architecture.patchRepository: Mesh-LLM/mesh-llm
Length of output: 14720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Model the ggml_tensor layout from the vendored ggml.h and confirm that
# writing sizeof(int64_t) at offset GGML_MAX_OP_PARAMS from op_params is
# an out-of-bounds store into the ggml_tensor object for the common GGML_MAX_OP_PARAMS count.
python3 - <<'PY'
from pathlib import Path
import re
for f in Path('third_party').glob('**/ggml.h'):
s = f.read_text()
m = re.search(r'(?m)defn\|GGML_MAX_OP_PARAMS\s+=(\s*\d+)', s)
op = re.search(r'(?m)int32_t\s+op_params\s*\[\s*GGML_MAX_OP_PARAMS\s*/\s* sizeof\(int32_t\)\s*\]', s)
if m and op:
maxv = int(m.group(1))
ne_size = 8
print(f'{f}: GGML_MAX_OP_PARAMS={maxv} op_params_elements={maxv//4} layout_ok={op.group(0)}')
bytes_for_rel_extent = 8
offset = maxv + bytes_for_rel_extent
print(f' bytes from tensor start: flags={8}; ne={8*ne_size}; np=0; src=8; type={8}; padding={8*(4-ne_size//4)}; data=8; id=8; name=64; op_params={maxv}; total_before_op_params={sum([8,8*ne_size,0,8,8,8*(4-ne_size//4),8,8,64])}')
print(f' op_params starts at {sum([8,8*ne_size,0,8,8,8*(4-ne_size//4),8,8,64])}; out-of-bounds end is {sum([8,8*ne_size,0,8,8,8*(4-ne_size//4),8,8,64]) + maxv}')
print(f' memcpy(result->op_params + 16, &rel_extent, 8) writes through offset {sum([8,8*ne_size,0,8,8,8*(4-ne_size//4),8,8,64]) + maxv + 8}')
PYRepository: Mesh-LLM/mesh-llm
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked third_party files matching ggml:ggml:*.h:\n'
git ls-files third_party | rg 'ggml.*\.h$|ggml\.h$' || true
printf '\nTop-level files matching ggml:ggml:*.h:\n'
git ls-files | rg '(^|/)ggml.*\.h$' || true
printf '\nAll files containing GGML_MAX_OP_PARAMS:\n'
rg -n 'GGML_MAX_OP_PARAMS|int32_t\s+op_params' . --glob '*.h' --glob '*.hpp' --glob '*.c' --glob '*.cpp' --glob '*.cc' | head -80Repository: Mesh-LLM/mesh-llm
Length of output: 288
Remove the out-of-bounds rel_extent write.
ggml_tensor::op_params is a fixed-size 16-element int32_t array for GGML_MAX_OP_PARAMS == 64, so result->op_params + 16 is past-the-end and the 8-byte memcpy corrupts whichever ggml_tensor field follows the array. The banded attention paths read rel_extent from rel->ne[0], so the store is redundant; drop it or keep the value in rel_logits->ne[0].
🤖 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/0046-Add-TML-Inkling-architecture.patch` around
lines 2032 - 2034, Remove the out-of-bounds memcpy writing rel_extent in the
operation setup near ggml_set_op_params; rely on rel->ne[0] or rel_logits->ne[0]
for the banded attention paths, preserving the existing scale parameter
initialization.
| + } else if (accepted_count > 0) { | ||
| + status = skippy_decode_tokens( | ||
| + session, | ||
| + checkpoint.token_ids.data(), | ||
| + accepted_count, | ||
| + false, | ||
| + out_error); | ||
| + } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
checkpoint.token_ids.data() can be empty here.
skippy_checkpoint_verify_window only fills token_ids when the caller passes a non-null pointer (skippy_verify_tokens_frame_sampled passes token_ids, but a checkpoint taken without them leaves the vector empty). Reaching this branch with accepted_count > 0 and an empty vector hands skippy_decode_tokens a dangling pointer plus a positive count. Guard on emptiness and fail explicitly instead.
🛡️ Proposed guard
- } else if (accepted_count > 0) {
+ } else if (accepted_count > 0) {
+ if (checkpoint.token_ids.size() < accepted_count) {
+ skippy_set_error(out_error, SKIPPY_STATUS_RUNTIME_ERROR,
+ "verify checkpoint is missing the accepted token prefix");
+ return SKIPPY_STATUS_RUNTIME_ERROR;
+ }
status = skippy_decode_tokens(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + } else if (accepted_count > 0) { | |
| + status = skippy_decode_tokens( | |
| + session, | |
| + checkpoint.token_ids.data(), | |
| + accepted_count, | |
| + false, | |
| + out_error); | |
| + } | |
| } else if (accepted_count > 0) { | |
| if (checkpoint.token_ids.size() < accepted_count) { | |
| skippy_set_error(out_error, SKIPPY_STATUS_RUNTIME_ERROR, | |
| "verify checkpoint is missing the accepted token prefix"); | |
| return SKIPPY_STATUS_RUNTIME_ERROR; | |
| } | |
| status = skippy_decode_tokens( | |
| session, | |
| checkpoint.token_ids.data(), | |
| accepted_count, | |
| false, | |
| out_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/0053-Restore-recurrent-verify-window-state.patch`
around lines 194 - 201, Guard the accepted_count > 0 branch in the checkpoint
verification flow before calling skippy_decode_tokens: validate that
checkpoint.token_ids is non-empty, and explicitly fail using the existing
out_error/status mechanism when it is empty. Only pass
checkpoint.token_ids.data() with a positive count when the vector contains the
required tokens.
There was a problem hiding this comment.
There was a problem hiding this comment.
@michaelneale, acknowledged. I’ll leave this thread open and defer verification until the checkpoint-recovery work from #1100 is merged or incorporated into this PR.
i386
left a comment
i386
left a comment
There was a problem hiding this comment.
Re-review of changes since prior review head a3673cbb, at current head 19653da2.
The KV planning/runtime policy now resolves the Inkling family default consistently from GGUF metadata, and the replaced direct-path connection now uses the bounded drain-and-close path. I also reviewed the subsequent Inkling sidecar, wire-version, split-cache, and stage-loading hardening changes. No additional blocking finding in this pass; current required checks are green.
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)
crates/skippy-server/src/binary_transport/stage_execution.rs (1)
575-613: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConvert native-MTP proposals from the fixed array using
token_count.
NativeMtpDraft::token_idsis[i32; 8], whileStageReplyexpects aVec<i32>. Convertdraft.token_ids[0..draft.token_count.try_into()?].to_vec()(or return an error whentoken_countexceeds the array length) so the wire protocol carries only the actual 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 `@crates/skippy-server/src/binary_transport/stage_execution.rs` around lines 575 - 613, The stage_native_mtp_draft function currently forwards the entire fixed-size token_ids array instead of only the actual proposals. Use draft.token_count to bound the slice, validate that it does not exceed the array length, and return or propagate an error for an invalid count while preserving proposal_compute_us.third_party/llama.cpp/patches/0053-Carry-multimodal-Inkling-embeddings-into-MTP.patch (1)
47-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA failed MTP observe now reports the whole
llama_decodeas failed after KV was already mutated.
ctx->decode(batch)has already committed KV/state whenskippy_external_decode_observeruns. Returning its-1fromllama_decodemakes every caller treat the decode as failed and typically retry or abort the sequence, while the target context has in fact advanced — a silent position/cache divergence. The sidecar sync is auxiliary (drafting); a failure there should be logged and swallowed, or surfaced through a distinct channel, not folded into the decode return code.🛠️ Keep decode success authoritative
int ret = ctx->decode(batch); if (ret == 0) { - ret = skippy_external_decode_observe(ctx, batch); + // decode already committed KV; a sidecar/draft failure must not be + // reported as a decode failure + (void) skippy_external_decode_observe(ctx, batch); }🤖 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/0053-Carry-multimodal-Inkling-embeddings-into-MTP.patch` around lines 47 - 58, Keep ctx->decode(batch)'s return value authoritative in llama_decode; invoke skippy_external_decode_observe(ctx, batch) only as auxiliary sidecar work after a successful decode, and do not assign its failure result to ret. Log or propagate observer failures through a separate mechanism while preserving the successful decode return code.third_party/llama.cpp/patches/0048-Add-TML-Inkling-architecture.patch (2)
445-450: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent
unpadded_vocab_sizecontract.
get_vocab_basetreats it as optional (hp.get("unpadded_vocab_size") or n_vocab), butset_gguf_parameters(Line 502) and the log line (Line 505) indexhp["unpadded_vocab_size"]directly, so a checkpoint without the key passes vocab validation and then dies with aKeyErrorlate in conversion. Pick one contract — either validate/raise early in__init__, or fall back consistently.🤖 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/0048-Add-TML-Inkling-architecture.patch` around lines 445 - 450, Make the unpadded_vocab_size handling consistent across get_vocab_base, set_gguf_parameters, and the related log statement. Either validate the key during __init__ with an early, clear error or reuse the n_vocab fallback everywhere, ensuring checkpoints without unpadded_vocab_size do not reach direct hp[...] accesses and fail late.
4692-4695: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
<cstdlib>foratoi.
maincallsatoi, but the file does not include<cstdlib>. Add#include <cstdlib>and callstd::atoiso this does not depend on indirect or nonstandard headers.🤖 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/0048-Add-TML-Inkling-architecture.patch` around lines 4692 - 4695, Add the <cstdlib> header to the file containing main, and update the repeats initialization in main to call std::atoi explicitly instead of the unqualified atoi.third_party/llama.cpp/patches/0054-Size-recurrent-memory-for-appended-MTP-layers.patch (1)
26-40: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftVersion or gate the recurrent-state format change.
This patch makes exported MTP recurrent state include appended layers (
n_layer_all) but the reader rejects any saved blob whosen_layercount differs fromn_layer_all, including old MTP states that only containn_layer(). Add a format/version path or migration for cross-runtime state loads, or explicitly enforce/import-bound this format boundary.🤖 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/0054-Size-recurrent-memory-for-appended-MTP-layers.patch` around lines 26 - 40, Version or explicitly gate the recurrent-state format change in llama_memory_recurrent::state_write_data and state_read_data so readers can distinguish legacy n_layer() state blobs from the new n_layer_all format. Support migration or compatibility for old MTP states, or enforce an explicit import/export boundary that rejects incompatible formats before loading; preserve the existing layer-count validation for the selected format.
🧹 Nitpick comments (4)
third_party/llama.cpp/patches/0052-Add-Inkling-multi-depth-MTP-sidecars.patch (2)
444-467: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMTP logits skip the padded-vocab mask when the stage filter suppresses it.
vocab_maskis only allocated when!stage_filtered || stage_filter.include_output(0049), but the MTP branch adds it unconditionally-if-present. Today the sidecar only runs on aninclude_outputstage so it holds, but if that invariant ever slips, MTP drafts can sample padded ids while the trunk cannot. AGGML_ASSERTon the invariant here would make the coupling explicit.🤖 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/0052-Add-Inkling-multi-depth-MTP-sidecars.patch` around lines 444 - 467, Add a GGML_ASSERT in the MTP branch around vocab_mask handling to enforce that any MTP execution requiring output has vocab_mask available, matching the allocation condition from the stage-filter logic. Keep the existing ggml_add path unchanged when the mask is present.
472-492: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
output_normnow runs over every token, even whenembeddings_nextnis off.The norm was moved before the
inp_out_idsgather sot_h_nextncan see the full sequence, but the reorder is unconditional: on a large prefill the final RMS norm now processesn_tokens × n_embdinstead of just the output rows. Keep the old order when!cparams.embeddings_nextn.♻️ Gate the reorder on the nextn path
- cur = build_norm(cur, model.output_norm, NULL, LLM_NORM_RMS, -1); - cb(cur, "result_norm_all", -1); if (cparams.embeddings_nextn) { + cur = build_norm(cur, model.output_norm, NULL, LLM_NORM_RMS, -1); + cb(cur, "result_norm_all", -1); ggml_tensor * h_nextn = cur; if (cparams.embeddings_nextn_masked && inp_out_ids) { h_nextn = ggml_get_rows(ctx0, h_nextn, inp_out_ids); } cb(h_nextn, "h_nextn", -1); res->t_h_nextn = h_nextn; + if (inp_out_ids) { + cur = ggml_get_rows(ctx0, cur, inp_out_ids); + } + } else { + if (inp_out_ids) { + cur = ggml_get_rows(ctx0, cur, inp_out_ids); + } + cur = build_norm(cur, model.output_norm, NULL, LLM_NORM_RMS, -1); } - if (inp_out_ids) { - cur = ggml_get_rows(ctx0, cur, inp_out_ids); - }🤖 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/0052-Add-Inkling-multi-depth-MTP-sidecars.patch` around lines 472 - 492, Update llama_model_inkling::graph::graph so output_norm remains after the inp_out_ids gather when cparams.embeddings_nextn is disabled, preserving the existing output-only normalization path. Only normalize the full sequence before gathering when cparams.embeddings_nextn is enabled, then populate h_nextn and t_h_nextn from that normalized sequence while retaining the existing result outputs.third_party/llama.cpp/patches/0048-Add-TML-Inkling-architecture.patch (1)
4047-4053: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese tests bypass the
llama_build_and_testhelper.Every other entry in this file registers through
llama_build_and_test, which centralizes labels, working directory, and sanitizer handling. Rawadd_executable/add_testhere will drift (e.g. noLABEL, no consistent test env). Consider matching the surrounding convention unless the helper's llama linkage is the blocker.🤖 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/0048-Add-TML-Inkling-architecture.patch` around lines 4047 - 4053, Update the test registrations for test-flash-attn-bias and test-flash-attn-generic-hash to use the surrounding llama_build_and_test helper instead of raw add_executable/add_test calls, preserving their existing source files and linkage while inheriting centralized labels, working-directory, and sanitizer handling.third_party/llama.cpp/patches/0051-Replay-multi-depth-MTP-caches-correctly.patch (1)
220-247: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch is re-allocated every depth iteration.
llama_batch_init+std::malloc+llama_batch_freeinside the depth loop reallocates a max-draft_limit-row batch up todraft_limittimes per proposal, on the decode hot path. Allocating once fordraft_limit + 1rows and just settingbatch.n_tokens = n_rowsper iteration would remove the churn and the repeated failure-path bookkeeping.🤖 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/0051-Replay-multi-depth-MTP-caches-correctly.patch` around lines 220 - 247, Move llama_batch initialization and token allocation out of the depth loop in the chain_heads path, allocating capacity for draft_limit + 1 rows once and handling allocation failure once. Reuse that batch on each iteration by updating batch.n_tokens to n_rows and overwriting the active rows, then free it once after the loop while preserving the existing proposal-generation behavior.
🤖 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 `@crates/skippy-server/src/binary_transport/stage_execution.rs`:
- Around line 575-613: The stage_native_mtp_draft function currently forwards
the entire fixed-size token_ids array instead of only the actual proposals. Use
draft.token_count to bound the slice, validate that it does not exceed the array
length, and return or propagate an error for an invalid count while preserving
proposal_compute_us.
In `@third_party/llama.cpp/patches/0048-Add-TML-Inkling-architecture.patch`:
- Around line 445-450: Make the unpadded_vocab_size handling consistent across
get_vocab_base, set_gguf_parameters, and the related log statement. Either
validate the key during __init__ with an early, clear error or reuse the n_vocab
fallback everywhere, ensuring checkpoints without unpadded_vocab_size do not
reach direct hp[...] accesses and fail late.
- Around line 4692-4695: Add the <cstdlib> header to the file containing main,
and update the repeats initialization in main to call std::atoi explicitly
instead of the unqualified atoi.
In
`@third_party/llama.cpp/patches/0053-Carry-multimodal-Inkling-embeddings-into-MTP.patch`:
- Around line 47-58: Keep ctx->decode(batch)'s return value authoritative in
llama_decode; invoke skippy_external_decode_observe(ctx, batch) only as
auxiliary sidecar work after a successful decode, and do not assign its failure
result to ret. Log or propagate observer failures through a separate mechanism
while preserving the successful decode return code.
In
`@third_party/llama.cpp/patches/0054-Size-recurrent-memory-for-appended-MTP-layers.patch`:
- Around line 26-40: Version or explicitly gate the recurrent-state format
change in llama_memory_recurrent::state_write_data and state_read_data so
readers can distinguish legacy n_layer() state blobs from the new n_layer_all
format. Support migration or compatibility for old MTP states, or enforce an
explicit import/export boundary that rejects incompatible formats before
loading; preserve the existing layer-count validation for the selected format.
---
Nitpick comments:
In `@third_party/llama.cpp/patches/0048-Add-TML-Inkling-architecture.patch`:
- Around line 4047-4053: Update the test registrations for test-flash-attn-bias
and test-flash-attn-generic-hash to use the surrounding llama_build_and_test
helper instead of raw add_executable/add_test calls, preserving their existing
source files and linkage while inheriting centralized labels, working-directory,
and sanitizer handling.
In
`@third_party/llama.cpp/patches/0051-Replay-multi-depth-MTP-caches-correctly.patch`:
- Around line 220-247: Move llama_batch initialization and token allocation out
of the depth loop in the chain_heads path, allocating capacity for draft_limit +
1 rows once and handling allocation failure once. Reuse that batch on each
iteration by updating batch.n_tokens to n_rows and overwriting the active rows,
then free it once after the loop while preserving the existing
proposal-generation behavior.
In
`@third_party/llama.cpp/patches/0052-Add-Inkling-multi-depth-MTP-sidecars.patch`:
- Around line 444-467: Add a GGML_ASSERT in the MTP branch around vocab_mask
handling to enforce that any MTP execution requiring output has vocab_mask
available, matching the allocation condition from the stage-filter logic. Keep
the existing ggml_add path unchanged when the mask is present.
- Around line 472-492: Update llama_model_inkling::graph::graph so output_norm
remains after the inp_out_ids gather when cparams.embeddings_nextn is disabled,
preserving the existing output-only normalization path. Only normalize the full
sequence before gathering when cparams.embeddings_nextn is enabled, then
populate h_nextn and t_h_nextn from that normalized sequence while retaining the
existing result outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92245f70-47a0-4d16-ab38-c871adcfcf11
📒 Files selected for processing (29)
crates/mesh-llm-commands/src/model_package.rscrates/mesh-llm-host-runtime/src/mesh/connections.rscrates/mesh-llm-host-runtime/src/mesh/direct_path.rscrates/mesh-llm-host-runtime/src/models/capabilities.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local_package.rscrates/mesh-llm-host-runtime/src/runtime/local_split/loading.rscrates/skippy-correctness/src/runner/stage_fa_parity.rscrates/skippy-ffi/src/lib.rscrates/skippy-quantize/src/tokenizer_metadata.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/preconnect.rscrates/skippy-server/src/binary_transport/stage_execution.rscrates/skippy-server/src/frontend/generation/persistent_lanes.rscrates/skippy-server/src/runtime_state.rsscripts/hf-skippy-mtp-certify-job.pythird_party/llama.cpp/patches/0048-Add-TML-Inkling-architecture.patchthird_party/llama.cpp/patches/0049-Add-staged-execution-support-for-Inkling.patchthird_party/llama.cpp/patches/0050-Advance-native-MTP-depth-per-draft-step.patchthird_party/llama.cpp/patches/0051-Replay-multi-depth-MTP-caches-correctly.patchthird_party/llama.cpp/patches/0052-Add-Inkling-multi-depth-MTP-sidecars.patchthird_party/llama.cpp/patches/0053-Carry-multimodal-Inkling-embeddings-into-MTP.patchthird_party/llama.cpp/patches/0054-Size-recurrent-memory-for-appended-MTP-layers.patchthird_party/llama.cpp/patches/0055-Restore-recurrent-verify-window-state.patchthird_party/llama.cpp/patches/0056-skippy-preserve-verify-checkpoint-until-trim.patchthird_party/llama.cpp/patches/0057-skippy-retain-pipelined-verify-checkpoints.patchthird_party/llama.cpp/patches/0058-skippy-filter-Inkling-stage-memory-by-layer-range.patchthird_party/llama.cpp/patches/0059-Complete-Inkling-and-GLM-metadata-integration.patchthird_party/llama.cpp/patches/0060-skippy-balance-filtered-stages-across-devices.patch
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/skippy-server/src/runtime_state.rs
- crates/mesh-llm-host-runtime/src/mesh/direct_path.rs
- crates/mesh-llm-host-runtime/src/runtime/local.rs
- crates/mesh-llm-host-runtime/src/mesh/connections.rs
- crates/skippy-correctness/src/runner/stage_fa_parity.rs
- crates/mesh-llm-host-runtime/src/runtime/local_split/loading.rs
- crates/mesh-llm-host-runtime/src/models/capabilities.rs
- scripts/hf-skippy-mtp-certify-job.py
- crates/skippy-server/src/binary_transport/binary_messaging/connection.rs
- crates/skippy-server/src/frontend/generation/persistent_lanes.rs
- crates/skippy-quantize/src/tokenizer_metadata.rs
- crates/skippy-ffi/src/lib.rs
Inkling checkpoint before returning to PoolsideBranch Completed
Evidence boundaryThe earlier M5 + Vast model run showed that the Inkling implementation can load and execute across Metal/CUDA, but it used the pre-fix binary containing the now-corrected out-of-bounds write. It is implementation evidence only and must not be presented as final merge evidence. The corrected head has not yet completed:
Therefore this checkpoint does not yet claim production Inkling support, corrected-head Metal execution, or corrected-head 128K distributed inference. Next steps
Work is intentionally pausing here so Poolside #1095 can be finished first. |
|
Combined replacement branch is now open as #1118. It carries the current work from this PR onto current |
|
Superseded by #1118 |
4 participants
What this PR adds
Current support claim
This PR has now run real Inkling Q2 text inference across a three-stage mixed Metal/CUDA mesh. The certified live shape is:
0..3939..5959..66+ outputFixed test settings:
ctx=8192, one lane, F32 activation wire, F16 K/V, andmeshllm/inkling-UD-Q2_K_XL-layers@9b4b91a7ddd978dd7a01679bc977f6e53777f2c7.All stage traffic used the normal Iroh/QUIC mesh data path. Typical direct-path RTTs in the final runs were about 10–15 ms between the M5 and coordinator and 4–6 ms between the two Vast stages, with occasional larger spikes.
This is a text split-serving claim for this Q2 artifact and configuration. It is not yet a claim of tool-use, multimodal, live MTP, full native-context, multi-lane, Q8-wire, or DFlash certification.
Evidence on current head
19653da2End-to-end text and cache stability
KV-PIN-8842, reportedcached_tokens=3532, and completed in 2.718 s.hit_stage_mask=7) to token 3,538 after the lane had previously advanced to a longer state. This is the live regression proof for the recurrent-position fix: the old code retained the previous high-water position and failed the next divergentllama_decode; the current code moves the tracked position back to the imported state.llama_decode failed, binary-stage disconnect, or topology loss occurred in the fixed sequence.Exact replay is healthy. Arbitrary same-length shared-prefix reuse is not yet certified: the maintained same-prefix probe failed its semantic assertion because Q2 omitted
KV-PIN-8842, and the custom alpha/beta/gamma variants reportedcached_tokens=0because stage 0 missed the shared prefix. That is recorded as a remaining cache limitation, not described as a pass.N-gram latency hiding
The same three-stage topology was tested with standalone suffix N-gram speculation:
The reference deep run accepted 72 N-gram tail tokens, opened five verify windows, and reached three windows in flight. The repeated-Rust and tool-output cases accepted 64/90 and 32/90 proposals respectively; the novel control accepted 0/90 and stayed below 10 tok/s. N-gram is therefore a real latency-hiding win for repeated/copy/tool-output text, not a universal speed guarantee.
The performance result does not erase Q2 output quality limits. The repeated-Rust response reproduced the requested function, while the agent/tool-output response changed the iterator range and violated the supplied test contract. Throughput and semantic correctness are reported separately.
A subsequent 12-request uncached soak alternated unique repeated-JSON and novel ring-buffer prompts. All 12 returned HTTP 200 with no stage failure. Repeated prompts had median 35.86 tok/s (21.91–46.11); novel prompts had median 14.92 tok/s (8.55–19.21). Every request reported
cached_tokens=0, so this was generation/latency-hiding stability rather than exact replay. The soak did not score semantic quality.Build and test gates
MESH_LLM_DYNAMIC_NATIVE_RUNTIME=0 just release-buildMESH_LLM_DYNAMIC_NATIVE_RUNTIME=0 MESH_LLM_BUILD_PROFILE=release just build-runtime cuda 120cargo fmt --all --checkcargo test -p skippy-server --lib— 343 passedcargo test -p mesh-llm-host-runtime --lib— 1,860 passed, 8 ignoredskippy-server,mesh-llm-host-runtime, andmesh-llmcargo check -p skippy-serverandcargo check -p mesh-llmArtifact-side evidence remains available from the earlier jobs:
Explicitly not proven
tool_calls; the Q2 output was malformed tool-like text. Tool use fails this certification.ctx=8192with prompts up to roughly 3.5k tokens.model_unloaded, one M5 SIGINT shutdown ended in the upstreamggml_metal_device_freeresource-set assertion. Serving was already stopped. The generic hosted-stage drain work and required M5 proof are tracked explicitly in Drain hosted Skippy stages before Metal backend teardown #1103; this PR does not claim clean shutdown behavior.Review and merge posture
The branch is mergeable and CI is running. The intended merge claim is Inkling Q2 text split serving plus the architecture/package plumbing above. Tool use, multimodal generation, live MTP, full native context, multi-lane serving, shared-prefix variants, and the Metal teardown assertion remain visible follow-ups rather than implied successes.
Before leaving draft:
19653da2.Related latency-hiding evidence and follow-up are tracked in #1025.
@i386 @ndizazzo Please review the current, narrowed support claim and the generic fixes found by the live three-stage run.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation