skippy: land Laguna Q4 and experimental Inkling split serving - #1118
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds Inkling and Laguna staged-model support, immutable package revisions and projector metadata, stage-state v11 and protocol generation 4 behavior, split-runtime coordination and capacity handling, validation CLIs and job scripts, plus related tests and documentation. ChangesModel packaging and Inkling support
Runtime and mesh behavior
Protocol and speculative execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (17)
Justfile (1)
268-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate build-prep steps across standalone build/release recipes.
skippy-quantize-standalone-buildandskippy-quantize-standalone-release-buildrepeat the sameprepare-llama.sh pinned+build-llama.shsetup verbatim; only the finalcargo buildinvocation differs. A future change to the pinned-prep or backend/link-mode env vars in one recipe risks drifting from the other.♻️ Proposed refactor using a shared just dependency
+_skippy-quantize-standalone-prep backend="cpu": + scripts/prepare-llama.sh pinned + LLAMA_STAGE_BACKEND="{{ backend }}" LLAMA_STAGE_LINK_MODE=static scripts/build-llama.sh + -skippy-quantize-standalone-build backend="cpu": - scripts/prepare-llama.sh pinned - LLAMA_STAGE_BACKEND="{{ backend }}" LLAMA_STAGE_LINK_MODE=static scripts/build-llama.sh +skippy-quantize-standalone-build backend="cpu": (_skippy-quantize-standalone-prep backend) LLAMA_STAGE_BACKEND="{{ backend }}" LLAMA_STAGE_LINK_MODE=static just with-lld cargo build -p skippy-quantize --no-default-features [unix] -skippy-quantize-standalone-release-build backend="cpu": - scripts/prepare-llama.sh pinned - LLAMA_STAGE_BACKEND="{{ backend }}" LLAMA_STAGE_LINK_MODE=static scripts/build-llama.sh +skippy-quantize-standalone-release-build backend="cpu": (_skippy-quantize-standalone-prep backend) LLAMA_STAGE_BACKEND="{{ backend }}" LLAMA_STAGE_LINK_MODE=static just with-lld cargo build --release --locked -p skippy-quantize --no-default-features🤖 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 `@Justfile` around lines 268 - 277, Extract the shared `scripts/prepare-llama.sh pinned` and `build-llama.sh` setup from `skippy-quantize-standalone-build` and `skippy-quantize-standalone-release-build` into a shared just dependency or helper recipe, then make both recipes depend on it while retaining their distinct debug and release `cargo build` commands and existing backend/link-mode variables.crates/model-package/src/bin/queue-unsloth-layer-packages.rs (2)
792-799: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame total computed twice with different overflow semantics.
candidate_source_total_bytessaturates whilejob_spec_with_tokenre-derives the identical value withchecked_add+ error context. Job planning/bucket sizing and the container'sSOURCE_TOTAL_BYTEScan therefore disagree at the u64 boundary. Havejob_spec_with_tokencall the shared helper (or make the helper the checked one and use it in both places).♻️ Proposed consolidation
-fn candidate_source_total_bytes(candidate: &Candidate) -> u64 { - candidate - .projectors - .iter() - .fold(candidate.quant.total_bytes, |total, projector| { - total.saturating_add(projector.total_bytes) - }) -} +fn candidate_source_total_bytes(candidate: &Candidate) -> Result<u64> { + candidate + .projectors + .iter() + .try_fold(candidate.quant.total_bytes, |total, projector| { + total.checked_add(projector.total_bytes) + }) + .context("source GGUF and projector sizes overflowed u64") +}- let projector_bytes = candidate - .projectors - .iter() - .try_fold(0u64, |total, projector| { - total.checked_add(projector.total_bytes) - }) - .context("source GGUF and projector sizes overflowed u64")?; - let source_total_bytes = candidate - .quant - .total_bytes - .checked_add(projector_bytes) - .context("source GGUF and projector sizes overflowed u64")?; + let source_total_bytes = candidate_source_total_bytes(candidate)?;Also applies to: 1100-1111
🤖 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 job_spec_with_token to reuse candidate_source_total_bytes instead of independently summing quant and projector bytes with checked_add. Ensure job planning, bucket sizing, and SOURCE_TOTAL_BYTES all derive the total through the same helper and preserve consistent overflow semantics.
870-879: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
"inkling"substring as the multimodal allowlist.
info.id.to_ascii_lowercase().contains("inkling")will admit any repo whose id happens to contain that substring and needs a code change for the next supported multimodal family. A small named allowlist constant (or family-key match viamodel_family_key) would scale better and make the intent 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 `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs` around lines 870 - 879, The multimodal compatibility check in the queue compatibility logic uses an overly broad hardcoded “inkling” substring. Replace it with a named allowlist or model-family key match using the existing model-family mechanism, and update the condition to admit only explicitly supported multimodal families while preserving text-generation compatibility.crates/mesh-llm-host-runtime/src/runtime/local_split/tests.rs (1)
1615-1620: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnv-gated test silently passes in CI.
real_inkling_metadata_plans_family_kv_not_size_tieredreturnsOkwhenINKLING_METADATA_GGUFis unset, so it reports green without exercising anything. Prefer#[ignore = "requires INKLING_METADATA_GGUF"]so the skip is visible in test output.🤖 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_split/tests.rs` around lines 1615 - 1620, Update the real_inkling_metadata_plans_family_kv_not_size_tiered test to use an ignored-test attribute with the reason that INKLING_METADATA_GGUF is required, and remove the environment-variable early-return skip so missing configuration is visible in test output.crates/skippy-correctness/src/cli.rs (1)
329-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo doc comments means
stage-fa-parity --helprenders bare flags.Adding
///docs onmax_abs,layer_start/layer_end, and the two output paths would make the tolerance semantics and file outputs discoverable.🤖 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-correctness/src/cli.rs` around lines 329 - 349, Add doc comments to StageFaParityArgs fields max_abs, layer_start, layer_end, enabled_output, and disabled_output so stage-fa-parity --help describes tolerance semantics, layer range behavior, and the files written by each output path. Keep the existing argument names and defaults unchanged.crates/mesh-llm-host-runtime/src/runtime/local_split.rs (1)
152-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo independent hard-coded 30s timeouts now govern one start attempt.
elect_split_start_coordinatorusesDuration::from_secs(30)for membership settling andstart_runtime_split_modelpasses anotherDuration::from_secs(30)intoprepare_split_runtime_start, so a single split start can now block for up to ~60s before returning Standby/Started. Consider hoisting both into a named constant (and documenting the combined budget) so the startup retry loop cadence stays predictable.Also applies to: 378-395
🤖 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_split.rs` around lines 152 - 170, Define a shared named startup timeout constant near the split-start orchestration and reuse it for both elect_split_start_coordinator and prepare_split_runtime_start within start_runtime_split_model. Document that the two sequential phases share a combined startup budget, preserving the existing timeout duration and predictable retry cadence.crates/skippy-coordinator/src/topology.rs (1)
126-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStage-0 constraint failures surface as a generic
NoValidTopology.If the required stage-0 node isn't in
usable_nodes(or can't hold stage 0 at any shape), the caller only seesNoValidTopology { minimum_context }, which points at context sizing rather than the real cause. Consider a distinct error variant (or at least carrying the required node id) sosplit_topology_failure_reasoncan report it.Also applies to: 437-448
🤖 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-coordinator/src/topology.rs` around lines 126 - 130, Update plan_topology_with_required_stage0 and the related failure handling around split_topology_failure_reason so an unavailable or stage-0-incompatible required_stage0_node_id produces a distinct, identifiable TopologyPlanError (including the node id) instead of generic NoValidTopology. Ensure the caller reports this constraint failure while preserving existing behavior for ordinary topology failures.crates/skippy-correctness/src/runner/stage_fa_parity.rs (1)
64-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
model_iddiverges from the user-supplied--modelpath.
model_idis fixed to Inkling whilepackage_refis whateverargs.modelpoints to. If this harness is ever pointed at a non-Inkling package, family-specific behavior keyed offmodel_id(e.g. KV cache defaults) could silently apply the wrong policy instead of erroring.♻️ Suggested: parameterize model_id
- let selection = select_layer_package_parts(&PackageStageRequest { - model_id: "unsloth/inkling-GGUF:UD-Q2_K_XL".to_string(), + let selection = select_layer_package_parts(&PackageStageRequest { + model_id: args.model_id.clone(), topology_id: "stage-fa-parity".to_string(),🤖 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-correctness/src/runner/stage_fa_parity.rs` around lines 64 - 101, Update decode_boundary to derive PackageStageRequest.model_id from the user-supplied args.model instead of hardcoding the Inkling identifier, ensuring model-specific package selection and policies match the selected model.crates/mesh-llm-host-runtime/src/runtime/local_package.rs (2)
64-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent fallback hides a bad family default.
If
family_policy_for_compact_meta(...).default_kv_cache_typeever returns a stringGgufKvCacheQuant::from_llama_argscannot parse, planning silently reverts to the size-tiered policy and budgets a different KV size than the doc comment promises. Atracing::warn!on that path makes the mismatch diagnosable instead of showing up as an OOM at stage load.♻️ Proposed logging on the fallback path
models::gguf::GgufKvCacheQuant::from_llama_args(effective_k, effective_v).unwrap_or_else(|| { + tracing::warn!( + model_ref, + effective_k, + effective_v, + "unrecognized effective K/V cache types; falling back to size-tiered KV policy" + ); split_kv_cache_quant(&size_policy, cache_type_k_override, cache_type_v_override) })🤖 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_package.rs` around lines 64 - 111, Add a tracing::warn! call in split_effective_kv_cache_quant when GgufKvCacheQuant::from_llama_args returns None and split_kv_cache_quant is used as the fallback. Include the resolved family default and enough model context to diagnose the invalid value, while preserving the existing fallback behavior.
445-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPeer exclusion prelude is duplicated with
collect_split_participants.Lines 456-472 are byte-identical to lines 502-518; only the participant construction differs. Extracting the preflight/stage-path exclusion check into a small helper (returning
Result<(), SplitParticipantExclusion>per peer) keeps the two collectors from drifting as exclusion reasons are added.🤖 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_package.rs` around lines 445 - 486, Extract the duplicated peer exclusion logic from collect_split_participant_membership and collect_split_participants into a shared helper that returns Result<(), SplitParticipantExclusion> for each peer. Update both collectors to call the helper, recording the returned exclusion on Err and constructing the appropriate participant only on Ok, while preserving their existing participant-specific construction behavior.crates/mesh-llm-host-runtime/src/mesh/direct_path.rs (1)
223-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Optionparameter is dead at the only call site.The
if let Some(existing) = existing.filter(...)guard meansrecord_draining_replaced_connectionis never called withNone, so the early return is unreachable. Also the name suggests state mutation while the body only logs.♻️ Optional simplification
- record_draining_replaced_connection(remote, Some(&existing), &conn); + log_replaced_connection_drain(remote, &existing, &conn); Self::spawn_replaced_connection_drain(remote, existing); } } } -fn record_draining_replaced_connection( - remote: EndpointId, - existing: Option<&Connection>, - replacement: &Connection, -) { - let Some(existing) = existing else { - return; - }; +fn log_replaced_connection_drain( + remote: EndpointId, + existing: &Connection, + replacement: &Connection, +) { tracing::debug!(🤖 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 - 245, Update record_draining_replaced_connection to accept a direct &Connection instead of Option<&Connection>, remove the unreachable None early return, and rename the function to reflect that it only logs the replacement. Adjust its sole call site in the existing replacement branch to pass existing directly while preserving the current debug fields and message.third_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patch (2)
4194-4201: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThese banded cases add substantial cost to the default
test-backend-opsrun.Six cases at
n_q=512withn_kvfrom 8192 up to 32768 (128-dim, 8 heads) are appended tomake_test_cases_eval(), which everytest-backend-opsinvocation executes against the CPU reference. That is roughly 4 · 8 · 512 · 32768 · 128 FLOPs for the largest case alone, per backend. Gating the ≥16K cases behind the perf/large mode (as the comment about the "16.4-16.9K garbage threshold" suggests they are regression probes) would keep default CI runtime bounded while retaining coverage.🤖 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/0050-Add-TML-Inkling-architecture.patch` around lines 4194 - 4201, Reduce the default cost of make_test_cases_eval() by gating the banded test_flash_attn_ext_banded cases with n_kv at or above 16K behind the existing performance/large-test mode. Keep the smaller 8192 case in the default suite and preserve all regression probes when that mode is enabled.
440-451: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBare
except Exceptionmasks real tokenizer-config errors.If
tokenizer_config.jsonexists but is malformed, oradded_tokens_decoderhas a non-integer key, the conversion silently falls back to the positional "trailing 60 ids" convention and produces a GGUF with wrong CONTROL token types — a failure that only surfaces as bad generation much later. Narrowing toFileNotFoundError/KeyError(and logging the fallback) keeps the intended default while surfacing genuine corruption.🤖 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/0050-Add-TML-Inkling-architecture.patch` around lines 440 - 451, The tokenizer configuration loading around tc and special_ids must not mask malformed data or invalid token IDs. Restrict fallback handling to the expected missing-file or absent-key cases, log when the positional trailing-60 fallback is used, and allow JSON/parsing or integer-conversion errors to propagate instead of silently producing incorrect CONTROL token types.crates/skippy-correctness/src/runner/prediction_return.rs (1)
159-178: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePartial peek silently leaves the hello bytes in the stream.
peekunderSO_RCVTIMEOcan returnOk(n)with0 < n < 4. TheOk(_) => {}arm then skipsrecv_ready, leaving the READY magic in the receive buffer, andread_stage_messageat line 132 parses it as the open message. Practically unreachable on loopback with a 4-byte write, but a short retry loop (or treatingOk(n)withn>0 && n<4as "keep peeking") removes the ambiguity.🤖 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-correctness/src/runner/prediction_return.rs` around lines 159 - 178, Update the direct prediction return readiness handling around stream.peek and recv_ready so partial successful peeks do not fall through as an empty result. Retry peeking until all four READY_MAGIC bytes are available, or otherwise consume and validate the complete hello before read_stage_message can process the stream; preserve the existing timeout/error handling.third_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patch (1)
472-492: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFinal
output_normnow runs on the untrimmed sequence on every decode.Moving
build_norm(cur, model.output_norm, ...)above theinp_out_idsgather makes the norm costn_tokensrows instead ofn_outrows. During prefill with a single requested output row and a 6144-wide embedding, that is the full ubatch normalized just to be discarded.The reordering is only required to feed
t_h_nextn; whencparams.embeddings_nextnis false the old ordering is still correct and cheaper.♻️ Suggested: keep the trim-first ordering when nextn embeddings are not requested
ggml_tensor * inp_out_ids = build_inp_out_ids(); - 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); - } cb(cur, "result_norm", -1);🤖 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-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 false, preserving the trimmed-sequence cost; only normalize the untrimmed cur before gathering when embeddings_nextn is enabled so t_h_nextn receives the full normalized sequence.crates/skippy-quantize/src/gguf_writer.rs (1)
443-452: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnly
k_sconvshortconv path is exercised by tests.
mapped_tensor_dimsasserts a strict[channels, 1, kernel]shape for any mapped name containing.shortconv_, which also coversshortconv_v,shortconv_attn, andshortconv_mlp(per the newtensor_map.rsmappings forattn.v_sconv.weight,attn_sconv.weight,mlp_sconv.weight). Only thek_sconvcase is covered bywrites_inkling_mtp_streaming_transformsingguf_writer_tests.rs. If any of those other shortconv tensors have a different native shape in real Inkling checkpoints, the sharedensure!here would reject them at conversion time without any test catching it first.Consider adding test cases (or at least an assertion) for the other shortconv suffix variants.
🤖 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 443 - 452, Extend coverage for mapped_tensor_dims beyond the currently tested k_sconv case by adding assertions or test cases for shortconv_v, shortconv_attn, and shortconv_mlp mappings. Verify each variant’s expected native shape and converted dimensions, and ensure the existing strict validation remains limited to shapes those mappings actually require.crates/skippy-server/src/frontend/embedded_generation.rs (1)
1-2020: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile is very large and keeps absorbing new branching logic.
This file is already very large (~2,000 lines) and this PR adds more branching (effective-speculative bypass, verify-checkpoint retirement in two places) directly into
generate_embedded_stage_zero_tokens. Consider extracting the verify-window pipelining/retirement bookkeeping into a dedicated helper module in a future pass.As per coding guidelines, "Do not add Rust source files over 2,000 lines; split approaching oversized files by responsibility."
🤖 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/frontend/embedded_generation.rs` around lines 1 - 2020, The generate_embedded_stage_zero_tokens method has grown beyond the 2,000-line source-file guideline by accumulating verify-window pipelining and retirement branches. Extract that bookkeeping, including effective-speculative bypass and verify-checkpoint retirement logic, into a dedicated helper module or responsibility-focused module, then keep generate_embedded_stage_zero_tokens focused on orchestration while preserving existing behavior.Source: Coding guidelines
🤖 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-host-runtime/src/inference/skippy/resolver/resolution.rs`:
- Around line 248-250: Reorder the cache-type resolution logic so explicit
global_model_fit.cache_type_k/v values are checked before
family_policy.default_kv_cache_type. Preserve the family default as the fallback
before generic macro or policy defaults, ensuring global overrides remain
effective for both K and V cache resolution.
In `@crates/mesh-llm-host-runtime/src/runtime/local_split.rs`:
- Around line 189-194: Update the retry classification used by the split startup
loop to treat the canonical-coordinator stage-0 topology errors from the stage-0
validation checks as retryable. Include both the “split topology stage 0 … does
not match canonical coordinator …” and “split topology lock stage 0 must be
canonical coordinator …” failures, while preserving existing handling for
convergence, transport, and source-timeout cases.
In `@crates/mesh-llm-host-runtime/src/runtime/local_split/loading.rs`:
- Around line 23-27: Update wait_for_split_stage_source and the
STAGE_SOURCE_PREPARE timeout bounds so the timeout cannot be capped below the
calculated 16 MiB/s transfer budget. Make MAX_STAGE_SOURCE_PREPARE_TIMEOUT
configurable or allow calculated budgets above six hours, preserving the
existing minimum timeout and successful preparation for very large stages.
In `@crates/mesh-llm-host-runtime/src/runtime/split_participant_settle.rs`:
- Around line 12-18: Update the settle deadline logic used by is_ready and
finish_at_timeout so caller timeouts shorter than the required observation
barrier remain reachable, preferably by clamping the effective deadline to at
least the configured SPLIT_FIRST_QUORUM_OBSERVATION window. Preserve normal
timeout behavior for longer caller timeouts and avoid emitting the "settle timed
out" warning for a stable mesh solely because the requested timeout was below
the barrier duration.
In `@crates/skippy-model-package/src/preflight.rs`:
- Around line 1021-1030: Update the preflight validation for
window.pipeline_depth in the surrounding speculative strategy checks to reject
values above the shared supported maximum as well as zero. Reuse the existing
maximum-capacity constant or symbol used by the server/native
checkpoint-retention limit, preserve the current error reporting pattern, and
add a boundary test covering the maximum accepted value and the first
unsupported value.
In `@crates/skippy-protocol/src/lib.rs`:
- Around line 15-21: Retain the V3 generation feature constant and
generation-specific support in crates/skippy-protocol/src/lib.rs:15-21 while
adding V4. In crates/mesh-llm-host-runtime/src/protocol/convert.rs:17-21, update
the announcement conversion to advertise both supported generations; in
convert.rs:50-57, negotiate the highest mutually supported generation rather
than requiring V4, preserving V3 framing for V3-only peers. Add announcement
coverage in
crates/mesh-llm-host-runtime/src/protocol/tests/announcements.rs:82-85 for
V3-only interoperability and V4 preference when both peers support it.
In `@crates/skippy-quantize/src/mtp_attach.rs`:
- Around line 50-55: Update the ABI feature probing in the MTP attach validation
flow to handle panics from skippy_ffi::abi_features() when the loaded runtime
lacks the symbol. Catch the probe failure and convert it into the existing
unsupported-runtime validation error, preserving the current ensure! check for
runtimes that successfully report features.
In `@crates/skippy-quantize/src/tensor_map.rs`:
- Around line 113-134: Update normalize_inkling_mtp_source_name to compute the
layer index with layer_start.checked_add(depth), and propagate an error with the
context “Inkling MTP layer id overflow” when it overflows. Keep the existing
tensor-name normalization and formatting unchanged.
In `@crates/skippy-server/README.md`:
- Around line 83-87: Update the remaining “generation-3 prediction returns”
reference in the README compatibility section to “generation-4 prediction
returns,” preserving the surrounding generation-4 protocol documentation.
In `@crates/skippy-server/src/binary_transport/binary_messaging/connection.rs`:
- Around line 99-146: Split the control-message handling from
handle_binary_connection_messages into a semantically named module, including
the Stop, verify-retirement, session-control, generation-control, and
prefix-cache-control branches. Move the associated logic and relevant tests to
the new module, keeping it under 1,000 lines, and update
handle_binary_connection_messages to delegate while preserving session_tracker
behavior.
In `@crates/skippy-server/src/binary_transport/stage_execution.rs`:
- Around line 119-130: Update complete_downstream_ready to set a short write
timeout on the TcpStream before calling send_client_ready_hello_if_enabled, then
clear the write timeout after the handshake. Preserve the existing read-timeout
setup and result propagation, ensuring timeout cleanup occurs after both the
hello write and recv_ready.
In `@scripts/hf-skippy-mtp-certify-job.py`:
- Around line 79-89: Harden the projector download flow around the URL handling
and urllib.request.urlopen call: allow only trusted HTTPS hosts, validate every
redirect destination, reject private, loopback, link-local, reserved, and
otherwise non-public resolved addresses, and enforce bounded connection/read
timeouts and a maximum response/download size. Preserve the existing atomic
temporary-file replacement and require_gguf_magic validation after a successful
download.
In `@third_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patch`:
- Around line 3017-3060: Eliminate the O(n_kv) validation scan in
llama_kv_cache::get_n_kv_pos_contiguous by maintaining cached contiguity state
with a high-water mark on the cache, invalidating or updating it during sequence
removal and defragmentation. Use that state for an O(1) or incrementally bounded
check after the existing cheap ubatch validations, while preserving rejection
when any hole or non-contiguous cell is present.
In
`@third_party/llama.cpp/patches/0055-Carry-multimodal-Inkling-embeddings-into-MTP.patch`:
- Around line 22-40: Bump the appropriate SKIPPY_ABI_VERSION_* constant in
skippy/common.h for the new SKIPPY_ACTIVATION_FLAG_INKLING_MTP_EMBD and
SKIPPY_FEATURE_INKLING_MTP_MM staged-runtime ABI surface, then update the
matching ABI version constant in crates/skippy-ffi/src/lib.rs so both
representations remain synchronized.
- Around line 47-58: Update llama_decode so the return value remains solely the
result of ctx->decode(batch); do not propagate skippy_external_decode_observe
failures to callers after a successful target decode. Handle the observer
failure through the existing Skippy/MTP session state, marking it so
skippy_mtp_propose_next skips drafting until the session is re-primed, while
preserving the observer’s stderr logging.
In
`@third_party/llama.cpp/patches/0056-Size-recurrent-memory-for-appended-MTP-layers.patch`:
- Around line 24-42: Bump the staged-runtime ABI version in the common ABI
definition and update the mirrored constant in the Skippy FFI bindings. Modify
the relevant ABI version symbols in common.h and crates/skippy-ffi/src/lib.rs,
keeping both values identical so states using the new hparams.n_layer_all
recurrent layout cannot be mixed with the previous layout.
In
`@third_party/llama.cpp/patches/0059-Preserve-hybrid-verify-window-state-across-trim.patch`:
- Around line 164-203: Add a bounds validation at the start of
skippy_verify_prefix_activation_payload before either the ALTUP slice or memcpy
operations, ensuring all computed source and prefix ranges fit within
checkpoint.input_payload.size(). Return the established empty/failed result when
validation fails so skippy_fail_verify_restore can handle it; leave valid
payload reconstruction unchanged.
- Around line 133-141: The accepted verification flow must retire its checkpoint
after successful verification to prevent steady-state accumulation. Update the
patched skippy_verify_* success path to call
skippy_retire_verify_checkpoint_exact() for the accepted checkpoint, preserving
checkpoints during normal verified generation and leaving
trim/reset/import/reset-position cleanup behavior unchanged.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/mesh/direct_path.rs`:
- Around line 223-245: Update record_draining_replaced_connection to accept a
direct &Connection instead of Option<&Connection>, remove the unreachable None
early return, and rename the function to reflect that it only logs the
replacement. Adjust its sole call site in the existing replacement branch to
pass existing directly while preserving the current debug fields and message.
In `@crates/mesh-llm-host-runtime/src/runtime/local_package.rs`:
- Around line 64-111: Add a tracing::warn! call in
split_effective_kv_cache_quant when GgufKvCacheQuant::from_llama_args returns
None and split_kv_cache_quant is used as the fallback. Include the resolved
family default and enough model context to diagnose the invalid value, while
preserving the existing fallback behavior.
- Around line 445-486: Extract the duplicated peer exclusion logic from
collect_split_participant_membership and collect_split_participants into a
shared helper that returns Result<(), SplitParticipantExclusion> for each peer.
Update both collectors to call the helper, recording the returned exclusion on
Err and constructing the appropriate participant only on Ok, while preserving
their existing participant-specific construction behavior.
In `@crates/mesh-llm-host-runtime/src/runtime/local_split.rs`:
- Around line 152-170: Define a shared named startup timeout constant near the
split-start orchestration and reuse it for both elect_split_start_coordinator
and prepare_split_runtime_start within start_runtime_split_model. Document that
the two sequential phases share a combined startup budget, preserving the
existing timeout duration and predictable retry cadence.
In `@crates/mesh-llm-host-runtime/src/runtime/local_split/tests.rs`:
- Around line 1615-1620: Update the
real_inkling_metadata_plans_family_kv_not_size_tiered test to use an
ignored-test attribute with the reason that INKLING_METADATA_GGUF is required,
and remove the environment-variable early-return skip so missing configuration
is visible in test output.
In `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs`:
- Around line 792-799: Update job_spec_with_token to reuse
candidate_source_total_bytes instead of independently summing quant and
projector bytes with checked_add. Ensure job planning, bucket sizing, and
SOURCE_TOTAL_BYTES all derive the total through the same helper and preserve
consistent overflow semantics.
- Around line 870-879: The multimodal compatibility check in the queue
compatibility logic uses an overly broad hardcoded “inkling” substring. Replace
it with a named allowlist or model-family key match using the existing
model-family mechanism, and update the condition to admit only explicitly
supported multimodal families while preserving text-generation compatibility.
In `@crates/skippy-coordinator/src/topology.rs`:
- Around line 126-130: Update plan_topology_with_required_stage0 and the related
failure handling around split_topology_failure_reason so an unavailable or
stage-0-incompatible required_stage0_node_id produces a distinct, identifiable
TopologyPlanError (including the node id) instead of generic NoValidTopology.
Ensure the caller reports this constraint failure while preserving existing
behavior for ordinary topology failures.
In `@crates/skippy-correctness/src/cli.rs`:
- Around line 329-349: Add doc comments to StageFaParityArgs fields max_abs,
layer_start, layer_end, enabled_output, and disabled_output so stage-fa-parity
--help describes tolerance semantics, layer range behavior, and the files
written by each output path. Keep the existing argument names and defaults
unchanged.
In `@crates/skippy-correctness/src/runner/prediction_return.rs`:
- Around line 159-178: Update the direct prediction return readiness handling
around stream.peek and recv_ready so partial successful peeks do not fall
through as an empty result. Retry peeking until all four READY_MAGIC bytes are
available, or otherwise consume and validate the complete hello before
read_stage_message can process the stream; preserve the existing timeout/error
handling.
In `@crates/skippy-correctness/src/runner/stage_fa_parity.rs`:
- Around line 64-101: Update decode_boundary to derive
PackageStageRequest.model_id from the user-supplied args.model instead of
hardcoding the Inkling identifier, ensuring model-specific package selection and
policies match the selected model.
In `@crates/skippy-quantize/src/gguf_writer.rs`:
- Around line 443-452: Extend coverage for mapped_tensor_dims beyond the
currently tested k_sconv case by adding assertions or test cases for
shortconv_v, shortconv_attn, and shortconv_mlp mappings. Verify each variant’s
expected native shape and converted dimensions, and ensure the existing strict
validation remains limited to shapes those mappings actually require.
In `@crates/skippy-server/src/frontend/embedded_generation.rs`:
- Around line 1-2020: The generate_embedded_stage_zero_tokens method has grown
beyond the 2,000-line source-file guideline by accumulating verify-window
pipelining and retirement branches. Extract that bookkeeping, including
effective-speculative bypass and verify-checkpoint retirement logic, into a
dedicated helper module or responsibility-focused module, then keep
generate_embedded_stage_zero_tokens focused on orchestration while preserving
existing behavior.
In `@Justfile`:
- Around line 268-277: Extract the shared `scripts/prepare-llama.sh pinned` and
`build-llama.sh` setup from `skippy-quantize-standalone-build` and
`skippy-quantize-standalone-release-build` into a shared just dependency or
helper recipe, then make both recipes depend on it while retaining their
distinct debug and release `cargo build` commands and existing backend/link-mode
variables.
In `@third_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patch`:
- Around line 4194-4201: Reduce the default cost of make_test_cases_eval() by
gating the banded test_flash_attn_ext_banded cases with n_kv at or above 16K
behind the existing performance/large-test mode. Keep the smaller 8192 case in
the default suite and preserve all regression probes when that mode is enabled.
- Around line 440-451: The tokenizer configuration loading around tc and
special_ids must not mask malformed data or invalid token IDs. Restrict fallback
handling to the expected missing-file or absent-key cases, log when the
positional trailing-60 fallback is used, and allow JSON/parsing or
integer-conversion errors to propagate instead of silently producing incorrect
CONTROL token types.
In
`@third_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patch`:
- 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 false,
preserving the trimmed-sequence cost; only normalize the untrimmed cur before
gathering when embeddings_nextn is enabled so t_h_nextn receives the full
normalized sequence.
🪄 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: 000c7644-433c-4a7e-aad0-34f8fe1794f4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (151)
.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-config/src/model_validation.rscrates/mesh-llm-host-runtime/src/inference/skippy/deployment.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/native_mtp_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.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/protocol/convert.rscrates/mesh-llm-host-runtime/src/protocol/tests/announcements.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/prediction_return.rscrates/skippy-correctness/src/runner/split_chain.rscrates/skippy-correctness/src/runner/stage_fa_parity.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/src/package.rscrates/skippy-model-package/src/preflight.rscrates/skippy-protocol/src/binary/activation.rscrates/skippy-protocol/src/binary/mod.rscrates/skippy-protocol/src/binary/types.rscrates/skippy-protocol/src/lib.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/activation.rscrates/skippy-runtime/src/media.rscrates/skippy-runtime/src/package.rscrates/skippy-runtime/src/runtime_events.rscrates/skippy-runtime/src/session.rscrates/skippy-runtime/src/types.rscrates/skippy-server/README.mdcrates/skippy-server/src/binary_transport/binary_messaging.rscrates/skippy-server/src/binary_transport/binary_messaging/async_forwarder.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/binary_messaging/reply.rscrates/skippy-server/src/binary_transport/binary_messaging/session_tracker.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/preconnect.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/server.rscrates/skippy-server/src/frontend/generation/timeouts.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rscrates/skippy-server/src/frontend/request.rscrates/skippy-server/src/frontend/speculative.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/lib.rscrates/skippy-topology/src/tests.rsdocs/LAYER_PACKAGE_REPOS.mddocs/design/TESTING.mddocs/design/message_protocol.mddocs/skippy/DATA_FLOW.mddocs/skippy/FAMILY_CERTIFY.mddocs/skippy/FAMILY_STATUS.mddocs/skippy/LLAMA_PARITY.mddocs/skippy/NEW_MODEL_ONBOARDING.mddocs/skippy/PIPELINED_VERIFY_WINDOW.mddocs/skippy/SUFFIX_NGRAM_PROPOSER.mddocs/skippy/WAN_SPLIT_PERF.mddocs/skippy/llama-parity-candidates.jsondocs/specs/layer-package-repos.mdscripts/hf-skippy-convert-job.pyscripts/hf-skippy-mtp-certify-job.pyscripts/tests/test_windows_native_runtime_deps.pythird_party/llama.cpp/patches/0048-Support-Laguna-staged-execution.patchthird_party/llama.cpp/patches/0049-Filter-staged-runtime-memory-to-layer-range.patchthird_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patchthird_party/llama.cpp/patches/0051-Add-staged-execution-support-for-Inkling.patchthird_party/llama.cpp/patches/0052-Advance-native-MTP-depth-per-draft-step.patchthird_party/llama.cpp/patches/0053-Replay-multi-depth-MTP-caches-correctly.patchthird_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patchthird_party/llama.cpp/patches/0055-Carry-multimodal-Inkling-embeddings-into-MTP.patchthird_party/llama.cpp/patches/0056-Size-recurrent-memory-for-appended-MTP-layers.patchthird_party/llama.cpp/patches/0057-Complete-Inkling-and-GLM-metadata-integration.patchthird_party/llama.cpp/patches/0058-skippy-balance-filtered-stages-across-devices.patchthird_party/llama.cpp/patches/0059-Preserve-hybrid-verify-window-state-across-trim.patchthird_party/llama.cpp/patches/0060-skippy-retire-accepted-verify-checkpoints-exactly.patchthird_party/llama.cpp/patches/0061-Reject-trims-after-failed-verify-recovery.patchtools/relay-fly-legacy/README.mdwebsite/src/docs/pages/CLI.md
9722a91 to
f4b1dd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
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/frontend/native_mtp/verify_window.rs (1)
184-229: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winRetirement check uses
commit_countinstead of the actualcommitted_positions, risking premature checkpoint retirement.
verify_checkpoint_no_longer_needed's first parameter iscommitted_positions(positions actually committed), but the call here passesnative_mtp_verify_decision.commit_count— the full intended commit length before the loop's early-stop/max_tokenstruncation. The loop above already computes the correctcommitted_positions(used correctly infully_accepted_windowat Line 210), so when generation stops early (EOG ormax_tokens) mid-window,commit_count >= consumed_positionscan be true while the actualcommitted_positions < consumed_positions, causingretire_verify_windowto fire and discard a recovery checkpoint that still covers unconsumed positions.🐛 Proposed fix
let checkpoint_no_longer_needed = verify_checkpoint_no_longer_needed( - native_mtp_verify_decision.commit_count, + committed_positions, consumed_positions, );🤖 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/frontend/native_mtp/verify_window.rs` around lines 184 - 229, Pass the loop’s actual committed_positions value to verify_checkpoint_no_longer_needed instead of native_mtp_verify_decision.commit_count. Keep the existing retirement flow unchanged so checkpoints are retired only when all consumed positions were actually committed, including early-stop and max_tokens truncation cases.crates/skippy-server/src/runtime_state.rs (1)
887-902: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame backward-restore bug left unfixed in the KV/full-state import paths.
This diff fixes
import_recurrent_state_for_token_countbecause a prefix restore can move a lane's tracked position backwards, and retaining the previous high-water mark makes the next decode submit at the wrong position (per the new comment onrecord_restored_session_token_count).import_state_for_token_countandimport_full_state_for_token_countare the same "restore session to an exact token_count" operation, but still use the old.and_modify(|current| *current = (*current).max(token_count)).or_insert(token_count)pattern here — so the identical bug (stale high-water mark → wrong decode position →llama_decodefailure) can still reproduce through these two paths.🐛 Proposed fix — reuse the new helper for both methods
pub fn import_state_for_token_count( &mut self, session_id: &str, bytes: &[u8], token_count: u64, ) -> Result<()> { let layer_start = i32::try_from(self.model_layer_start())?; let layer_end = i32::try_from(self.model_layer_end())?; let session = self.session(session_id)?; session.import_state_for_token_count(layer_start, layer_end, bytes, token_count)?; - self.session_token_counts - .entry(session_id.to_string()) - .and_modify(|current| *current = (*current).max(token_count)) - .or_insert(token_count); + record_restored_session_token_count(&mut self.session_token_counts, session_id, token_count); Ok(()) }pub fn import_full_state_for_token_count( &mut self, session_id: &str, bytes: &[u8], token_count: u64, ) -> Result<()> { let layer_start = i32::try_from(self.model_layer_start())?; let layer_end = i32::try_from(self.model_layer_end())?; let session = self.session(session_id)?; session.import_full_state_for_token_count(layer_start, layer_end, bytes, token_count)?; - self.session_token_counts - .entry(session_id.to_string()) - .and_modify(|current| *current = (*current).max(token_count)) - .or_insert(token_count); + record_restored_session_token_count(&mut self.session_token_counts, session_id, token_count); Ok(()) }Also applies to: 918-933
🤖 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/runtime_state.rs` around lines 887 - 902, Update import_state_for_token_count and import_full_state_for_token_count to use the new record_restored_session_token_count helper after restoring state, replacing the max-based session_token_counts updates. Ensure exact token_count restores can move the tracked position backward while preserving the existing import behavior and error propagation.crates/mesh-llm-host-runtime/src/mesh/node.rs (1)
132-165: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the touched oversized Rust modules before merge. All three files exceed the 1,000-line limit and contain separable responsibilities.
crates/mesh-llm-host-runtime/src/mesh/node.rs#L132-L165: move startup and hardware-snapshot construction into a semantically named mesh startup module.crates/mesh-llm-host-runtime/src/mesh/peer_state.rs#L854-L862: move connection acquisition and subprotocol-stream access into a responsibility-specific mesh module.crates/skippy-server/src/frontend/generation_flow.rs#L743-L743: extract split multimodal generation/forwarding into the existingfrontend/generation/module tree.As per coding guidelines, “When modifying a Rust source file over 1,000 lines, extract any separable responsibility into a semantically named module.”
🤖 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/node.rs` around lines 132 - 165, Split the oversized modules by extracting each separable responsibility: in crates/mesh-llm-host-runtime/src/mesh/node.rs lines 132-165, move startup and hardware-snapshot construction into a semantically named mesh startup module; in crates/mesh-llm-host-runtime/src/mesh/peer_state.rs lines 854-862, move connection acquisition and subprotocol-stream access into a responsibility-specific mesh module; and in crates/skippy-server/src/frontend/generation_flow.rs line 743, extract split multimodal generation and forwarding into the existing frontend/generation/ module tree.Source: Coding guidelines
crates/skippy-quantize/src/backend.rs (1)
172-174: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale symbol name in error message.
The reason string still says
"skippy_abi_features", but the probe now callsabi_features. This is confusing for anyone debugging via the reported reason.📝 Proposed fix
if feature_mask.is_none() { - return "loaded Skippy runtime does not expose skippy_abi_features".to_string(); + return "loaded Skippy runtime does not expose abi_features".to_string(); }🤖 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/backend.rs` around lines 172 - 174, Update the error string in the feature-mask probe to reference the current abi_features symbol instead of the stale skippy_abi_features name, while preserving the existing return behavior..github/workflows/queue-unsloth-layer-packages.yml (1)
63-69: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseonactions/checkout.Static analysis flags this checkout step for persisting the git credential unnecessarily. This job never pushes/tags, so the credential isn't needed after checkout.
🔒 Proposed fix
- uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5.1.0 + with: + persist-credentials: false🤖 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 @.github/workflows/queue-unsloth-layer-packages.yml around lines 63 - 69, Update the actions/checkout step in the workflow to set persist-credentials to false. Leave the existing checkout version and subsequent rust-toolchain step unchanged.Source: Linters/SAST tools
♻️ Duplicate comments (1)
third_party/llama.cpp/patches/0056-Size-recurrent-memory-for-appended-MTP-layers.patch (1)
14-42: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBump the staged-runtime ABI for the recurrent state layout change (duplicate of prior review).
state_write_data/state_read_datanow key onhparams.n_layer_allinstead ofhparams.n_layer(), changing the per-layer record count for MTP-capable models. This diff is unchanged from the version flagged in a previous review round; without an ABI version bump, cross-node state produced by pre/post-patch builds can be silently corrupted or spuriously rejected.As per coding guidelines: "When changing the staged-runtime ABI, bump the appropriate
SKIPPY_ABI_VERSION_PATCH,MINOR, orMAJORvalue inskippy/common.hand update the Rust mirror incrates/skippy-ffi/src/lib.rsin the same change."#!/bin/bash rg -n 'SKIPPY_ABI_VERSION_(MAJOR|MINOR|PATCH)' skippy/common.h crates/skippy-ffi/src/lib.rs 2>/dev/null🤖 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/0056-Size-recurrent-memory-for-appended-MTP-layers.patch` around lines 14 - 42, Bump the appropriate SKIPPY_ABI_VERSION_* value in skippy/common.h for the recurrent state layout change introduced by llama_memory_recurrent::state_write_data and state_read_data, then apply the identical version update to the Rust mirror in crates/skippy-ffi/src/lib.rs. Preserve all existing ABI components and change only the required version level.Source: Coding guidelines
🧹 Nitpick comments (13)
tools/relay-fly-legacy/README.md (1)
8-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit region mappings to the source of truth.
The documented relay URLs/IDs/trailing dots match
effective_relay_urls, butconnections.rsencodes them only as an ordered URL list, not as explicitUSW1-2 => US West/APS1-1 => Asia-Pacific South, etc. Add structured relay metadata or comments alongside the defaults so future changes don’t drift or silently reorder regions.🤖 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 `@tools/relay-fly-legacy/README.md` around lines 8 - 16, Add explicit relay ID-to-region mappings alongside the default relay definitions in effective_relay_urls within connections.rs, preserving the existing URLs and ordering while documenting USW1-2, APS1-1, EUC1-1, and USE1-1 with their corresponding regions. Use structured metadata or clear adjacent comments as appropriate to keep the source of truth aligned with the README.crates/model-package/src/prepare.rs (1)
182-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting revision resolution into a helper.
The new commit-pinning logic (fetch
info(), extractsha/pipeline_tag, thenlist_inventoryat that revision) is a self-contained unit bolted into the already-largeresolvefunction. Extracting it (e.g.resolve_source_revision(client, repo, requested_revision) -> Result<(String, String)>) would make this new behavior independently testable and keepresolvemore readable.🤖 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/prepare.rs` around lines 182 - 203, The revision-resolution logic in resolve is too large and should be isolated. Extract the requested-revision lookup, source SHA and pipeline-tag extraction, and revision-pinned list_inventory call into a helper such as resolve_source_revision, returning the resolved revision data needed by resolve; then replace the inline block with that helper call while preserving existing error contexts and defaults.crates/mesh-llm-host-runtime/src/runtime/local_package.rs (1)
445-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared peer-exclusion loop.
Lines 456-472 are byte-identical to lines 502-518 in
collect_split_participants; only the participant construction differs. A small helper returningResult<PeerInfo, SplitParticipantExclusionReason>(or an iterator of classified peers) would keep the two exclusion policies from drifting apart.🤖 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_package.rs` around lines 445 - 478, Extract the duplicated peer classification logic from collect_split_participant_membership and collect_split_participants into a shared helper that applies split_peer_preflight_exclusion_reason and split_peer_stage_path_exclusion_reason, returning either peer information or its exclusion reason. Update both callers to use this helper while preserving their differing participant construction and existing exclusion collection behavior.crates/skippy-correctness/src/runner/stage_fa_parity.rs (1)
91-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded Inkling
model_idin a generic parity runner.
select_layer_package_partsreceives a fixed"unsloth/inkling-GGUF:UD-Q2_K_XL"while the actual package comes fromargs.model, so running this against any other family silently mislabels the stage request. Plumb the model id throughStageFaParityArgs(defaulting to the current value if convenient).🤖 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-correctness/src/runner/stage_fa_parity.rs` around lines 91 - 100, Plumb a configurable model ID through StageFaParityArgs and use it when constructing the PackageStageRequest in the parity runner instead of the hardcoded Inkling value. Preserve the current Inkling identifier as the default if appropriate, while ensuring args.model remains the package reference.crates/mesh-llm-host-runtime/src/runtime/local_split/tests.rs (2)
81-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify: the
package_planning_calledflag is unreachable bookkeeping.The
Coordinator(_)arm only sets a bool that is asserted afterwards;panic!/unreachable!in that arm expresses the same intent more directly.♻️ Suggested simplification
- let mut package_planning_called = false; - let gate = canonical_coordinator_gate(local.node_id, vec![local, coordinator]) .expect("canonical coordinator gate"); match gate { CanonicalCoordinatorGate::Standby { coordinator: selected, } => assert_eq!(selected, coordinator.node_id), - CanonicalCoordinatorGate::Coordinator(_) => package_planning_called = true, + CanonicalCoordinatorGate::Coordinator(_) => { + panic!("local node must not be elected coordinator") + } } - - assert!(!package_planning_called);🤖 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_split/tests.rs` around lines 81 - 97, In the test noncanonical_gate_returns_standby_without_invoking_package_planning, remove the package_planning_called flag and replace the CanonicalCoordinatorGate::Coordinator(_) match arm with unreachable!() or an equivalent panic. Keep the standby coordinator assertion unchanged.
1644-1670: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion couples the family default to the size tier.
assert_eq!(planned, size_tiered)passes only because the hardcoded 318 GiBsource_model_byteshappens to land on a q4_0 tier; a futureKvCachePolicytier change would fail this test for reasons unrelated to family resolution. Assertingplanned == GgufKvCacheQuant::from_llama_args("q4_0", "q4_0")…(as the hermetic test at line 1593 does) states the intent directly.🤖 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_split/tests.rs` around lines 1644 - 1670, Replace the size-tier comparison in the test with a direct assertion that the planned quantization matches the expected Q4_0 family result produced by GgufKvCacheQuant::from_llama_args("q4_0", "q4_0"), as done by the hermetic test. Keep the policy.default_kv_cache_type assertion and diagnostic output, but remove the planned == size_tiered assertion so the test no longer depends on the model-size tier.crates/model-package/src/bin/queue-unsloth-layer-packages.rs (1)
792-799: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo divergent overflow policies for the same total.
candidate_source_total_bytessaturates whilejob_spec_with_tokenrecomputes the same sum withchecked_addand errors. Planning (line 256) and the job env (line 1116) can therefore disagree on a pathological input. Reuse one helper and pick one policy.♻️ Suggested consolidation
- let projector_bytes = candidate - .projectors - .iter() - .try_fold(0u64, |total, projector| { - total.checked_add(projector.total_bytes) - }) - .context("source GGUF and projector sizes overflowed u64")?; - let source_total_bytes = candidate - .quant - .total_bytes - .checked_add(projector_bytes) - .context("source GGUF and projector sizes overflowed u64")?; + let source_total_bytes = candidate_source_total_bytes(candidate);Also applies to: 1100-1111
🤖 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, Unify total-byte calculation between candidate_source_total_bytes and job_spec_with_token by reusing the same helper and overflow policy. Update the job-spec construction path around job_spec_with_token to call candidate_source_total_bytes (or the shared calculation helper) instead of recomputing with checked_add, ensuring planning and job environment values always agree.crates/mesh-llm-host-runtime/src/runtime/local_split.rs (1)
195-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSecond
tracing::info!is redundant.Lines 208-216 restate
model_ref,topology_id,run_id,local_node,context_length, andparallel_lanesalready emitted at lines 195-207, which also logs the elected coordinator. Dropping it (or reducing todebug!) keeps startup logs readable.🤖 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_split.rs` around lines 195 - 216, Remove the second redundant tracing::info! call in the split topology planning flow, preserving the preceding log with the elected coordinator and planning details. Do not alter the first log or surrounding topology behavior.third_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patch (1)
472-492: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
output_normnow runs over every token instead of only the output rows.Moving
build_norm(..., output_norm, ...)above theinp_out_idstrim is required to deriveh_nextn, but it also normalizes the full ubatch on every prefill even whenembeddings_nextnis off. Consider keeping the trimmed path when!cparams.embeddings_nextn.🤖 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-Add-Inkling-multi-depth-MTP-sidecars.patch` around lines 472 - 492, Update llama_model_inkling::graph::graph so output_norm is applied after inp_out_ids trimming when cparams.embeddings_nextn is disabled, while retaining the full-sequence normalization needed to populate h_nextn when it is enabled. Preserve the existing result_norm and result_norm_all callbacks for their respective paths.crates/skippy-quantize/src/gguf_writer.rs (1)
454-511: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffFused w13 is streamed twice (once per output tensor).
Both
ffn_gateandffn_upsources carrysource_byte_len = tensor.byte_len()and the samesource_name, sostream_tensor_datareads the entire fused tensor once per projection. That is correct but doubles source I/O for the largest MTP tensors. If conversion throughput matters here, consider a single pass writing both halves into their respective GGUF offsets.🤖 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 454 - 511, The fused w13 tensor is currently streamed twice because both sources in inkling_w13_tensor_sources reference the full source tensor. Update the conversion/streaming path to read each fused tensor once, split alternating rows during that pass, and write the gate and up halves to their respective GGUF offsets while preserving the existing names, shapes, dtypes, and parity mapping.third_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patch (1)
4983-4985: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTODO: Inkling is excluded from
test-llama-archsfixtures.Arch-specific hparams (
d_rel,rel_extent,shortconv_kernel,logit_scale_denom) have no fixture, so the new architecture has no load-path coverage in that suite. Want me to open an issue to track adding the fixture params?🤖 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/0050-Add-TML-Inkling-architecture.patch` around lines 4983 - 4985, Remove the unconditional LLM_ARCH_INKLING exclusion from the test-llama-archs fixture-selection logic and add fixture values for its architecture-specific hparams—d_rel, rel_extent, shortconv_kernel, and logit_scale_denom—so Inkling is included in load-path coverage.crates/skippy-server/src/binary_transport/binary_messaging/control_messages.rs (1)
33-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFive new
#[allow(clippy::too_many_arguments)]handlers instead of a shared context struct.The handlers all thread the same cluster of values (
config,topology,runtime,kv,telemetry,upstream,downstream,wire_dtype,downstream_wire_condition,downstream_connect_timeout_secs, forwarder, prediction-return maps). Extracting aControlMessageContext<'_>(borrowed collaborators) plus a small mutable-state struct would drop the lint suppressions and shrink the call sites inconnection.rs.As per coding guidelines, "Do not leave compiler or lint warnings in touched Rust code; fix warnings rather than using
#[allow(...)]unless there is a clear reason and explicit developer approval."Also applies to: 183-195, 233-250, 304-326
🤖 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/control_messages.rs` around lines 33 - 54, Replace the #[allow(clippy::too_many_arguments)] handler signatures, including handle_stop and the other three affected control-message handlers, with a shared borrowed ControlMessageContext<'_> plus a small mutable-state struct for per-request state. Move the common collaborators (config, topology, runtime, kv, telemetry, streams, wire settings, timeout, forwarder, and prediction-return resources) into the context, update handlers and connection.rs call sites to use it, and remove the lint suppressions.Source: Coding guidelines
crates/skippy-correctness/src/runner/split_chain.rs (1)
533-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReused
startup_timeout_secsas the prediction-return wait budget.
prediction_return.receive(Duration::from_secs(args.startup_timeout_secs))repurposes a server-startup readiness timeout as the bound for waiting on the actual decode/prediction round-trip. For large models this could time out prematurely (or be generously long) relative to what was tuned for process startup. Sincerecv_replypreviously had no explicit timeout here, this is a net improvement (bounds an otherwise-unbounded wait), but a dedicated timeout knob would better reflect intent.Also applies to: 556-558
🤖 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-correctness/src/runner/split_chain.rs` around lines 533 - 535, Replace the use of args.startup_timeout_secs in the prediction_return.receive calls within the split-chain prediction flow with a dedicated prediction/decode round-trip timeout setting. Add or reuse the appropriate configuration knob for this wait budget, and apply it consistently at both reported receive sites while preserving the existing error context.
🤖 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-host-runtime/src/protocol/convert.rs`:
- Around line 17-21: Update skippy_stage_subprotocols() and
supports_skippy_stage_generation() in
crates/mesh-llm-host-runtime/src/protocol/convert.rs at lines 17-21 and 50-54 to
advertise and recognize both V3 and V4 generation features. Preserve
stage_protocol_generation_supported for either supported generation, and select
V3-compatible encoding when negotiating with a V3 peer while retaining V4
behavior for V4 peers.
In `@crates/mesh-llm-host-runtime/src/runtime/startup_retry.rs`:
- Around line 7-13: Update split_participants_are_still_converging to classify
canonical-coordinator stage-0 mismatch errors as converging, matching both
“split topology stage 0” messages that indicate a mismatch with the canonical
coordinator and “split topology lock stage 0” messages requiring the canonical
coordinator, while preserving the existing classifiers.
In `@crates/skippy-quantize/src/tensor_map.rs`:
- Around line 242-264: Update the tensor mapping around the “mlp.w13_dn.weight”
arm to handle trunk Inkling fused w13 tensors consistently with native
conversion: route them through the supported conversion path when
TensorSelection::ExcludeMtp or All can select them, otherwise explicitly gate
them out before HfLayerTensor::map reaches the existing bail. Preserve the
current behavior for unsupported fused tensors.
In `@crates/skippy-quantize/src/tokenizer_metadata.rs`:
- Line 11: Update the TOKEN_TYPE_UNUSED constant to 5 so padded [PAD{i}]
vocabulary entries are emitted with the GGUF reserved/unused tokenizer type
instead of UNKNOWN.
In
`@crates/skippy-server/src/binary_transport/binary_messaging/session_tracker.rs`:
- Around line 47-49: Update the runtime.lock() failure branch in the
orphan-session reclamation flow to log the poisoned-lock error with eprintln!
before returning, matching the existing per-session Err handling and preserving
the current early-return behavior.
In `@crates/skippy-server/src/frontend/embedded_generation/lifecycle.rs`:
- Around line 123-125: The should_open_upstream_prediction_return function must
support standalone N-gram pipelined verify-window modes, not only native MTP, so
direct-return receivers are always opened before completion waits on them.
Update crates/skippy-server/src/frontend/embedded_generation/lifecycle.rs lines
123-125 to gate setup on all applicable pipelined verify-window modes;
alternatively, update crates/skippy-server/src/frontend/embedded_generation.rs
lines 934-945 to disable standalone N-gram pipelining unless its direct-return
receiver and stream are configured.
In `@scripts/hf-skippy-convert-job.py`:
- Around line 131-135: Ensure artifact_dir is created before the manifest and
status copy operations in the conversion flow, using recursive directory
creation with existing directories allowed. Update the function containing
write_beta_card so shutil.copy2 always receives a valid destination directory.
In `@third_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patch`:
- Around line 2932-2944: Update the padding condition in the fold_kq_b_into_mask
block to call ggml_pad only when kq_mask->ne[1] is greater than bias->ne[1],
preventing a negative padding extent; preserve the existing bias construction
and casting flow for equal or valid mask-larger dimensions.
- Around line 700-708: Update the RPC protocol definition alongside the
GGML_OP_COUNT assertion: increase RPC_PROTO_PATCH_VERSION from 4 to the next
version because the inserted GGML_OP_FLASH_ATTN_EXT_BANDED renumbers serialized
operations. Keep the GGML_OP_COUNT static_assert at 108.
In
`@third_party/llama.cpp/patches/0053-Replay-multi-depth-MTP-caches-correctly.patch`:
- Around line 117-159: Invalidate the MTP sidecar prefix whenever the depth loop
in skippy_mtp_sync_target_tokens performs seq_rm for chain_heads but exits with
rc != 0. Clear or mark false the session’s mtp_prefix_valid state before
returning the runtime error, while preserving the existing successful
synchronization behavior.
In
`@third_party/llama.cpp/patches/0055-Carry-multimodal-Inkling-embeddings-into-MTP.patch`:
- Around line 103-124: Update llm_graph_input_embd_h::set_input to handle
ubatches smaller than the full skippy_graph_get_mtp_embeddings() sidecar batch:
use the ubatch offset to select the corresponding slice of
mtp_embeddings.values, or validate that ubatch->n_tokens is no greater than
mtp_embeddings.token_count before copying. Remove the equality assertion while
preserving bounds validation and correct embedding alignment for split ubatches.
- Around line 534-564: Update the session destruction path to clear
g_skippy_external_decode_session when it points to the session being freed,
preventing external-decode observation from accessing a destroyed
skippy_session. Preserve the existing cleanup in
skippy_session_end_external_decode and only clear the thread-local for the
matching session.
In
`@third_party/llama.cpp/patches/0060-skippy-retire-accepted-verify-checkpoints-exactly.patch`:
- Around line 40-43: Update the Rust ABI mirror in crates/skippy-ffi/src/lib.rs
by changing ABI_VERSION_PATCH from 35 to 34, matching the
SKIPPY_ABI_VERSION_PATCH bump in the patch and preserving exact runtime ABI
consistency.
In
`@third_party/llama.cpp/patches/0062-Harden-Inkling-MTP-and-KV-contiguity-state.patch`:
- Around line 191-216: Update the caller of
skippy_verify_prefix_activation_payload() to distinguish an empty returned
vector as verification failure and propagate an error before assigning
input_desc.payload_bytes or calling skippy_decode_activation_frame. Preserve
valid non-empty prefix handling, and ensure oversized or otherwise corrupted
checkpoints cannot continue as zero-length prefixes.
---
Outside diff comments:
In @.github/workflows/queue-unsloth-layer-packages.yml:
- Around line 63-69: Update the actions/checkout step in the workflow to set
persist-credentials to false. Leave the existing checkout version and subsequent
rust-toolchain step unchanged.
In `@crates/mesh-llm-host-runtime/src/mesh/node.rs`:
- Around line 132-165: Split the oversized modules by extracting each separable
responsibility: in crates/mesh-llm-host-runtime/src/mesh/node.rs lines 132-165,
move startup and hardware-snapshot construction into a semantically named mesh
startup module; in crates/mesh-llm-host-runtime/src/mesh/peer_state.rs lines
854-862, move connection acquisition and subprotocol-stream access into a
responsibility-specific mesh module; and in
crates/skippy-server/src/frontend/generation_flow.rs line 743, extract split
multimodal generation and forwarding into the existing frontend/generation/
module tree.
In `@crates/skippy-quantize/src/backend.rs`:
- Around line 172-174: Update the error string in the feature-mask probe to
reference the current abi_features symbol instead of the stale
skippy_abi_features name, while preserving the existing return behavior.
In `@crates/skippy-server/src/frontend/native_mtp/verify_window.rs`:
- Around line 184-229: Pass the loop’s actual committed_positions value to
verify_checkpoint_no_longer_needed instead of
native_mtp_verify_decision.commit_count. Keep the existing retirement flow
unchanged so checkpoints are retired only when all consumed positions were
actually committed, including early-stop and max_tokens truncation cases.
In `@crates/skippy-server/src/runtime_state.rs`:
- Around line 887-902: Update import_state_for_token_count and
import_full_state_for_token_count to use the new
record_restored_session_token_count helper after restoring state, replacing the
max-based session_token_counts updates. Ensure exact token_count restores can
move the tracked position backward while preserving the existing import behavior
and error propagation.
---
Duplicate comments:
In
`@third_party/llama.cpp/patches/0056-Size-recurrent-memory-for-appended-MTP-layers.patch`:
- Around line 14-42: Bump the appropriate SKIPPY_ABI_VERSION_* value in
skippy/common.h for the recurrent state layout change introduced by
llama_memory_recurrent::state_write_data and state_read_data, then apply the
identical version update to the Rust mirror in crates/skippy-ffi/src/lib.rs.
Preserve all existing ABI components and change only the required version level.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/runtime/local_package.rs`:
- Around line 445-478: Extract the duplicated peer classification logic from
collect_split_participant_membership and collect_split_participants into a
shared helper that applies split_peer_preflight_exclusion_reason and
split_peer_stage_path_exclusion_reason, returning either peer information or its
exclusion reason. Update both callers to use this helper while preserving their
differing participant construction and existing exclusion collection behavior.
In `@crates/mesh-llm-host-runtime/src/runtime/local_split.rs`:
- Around line 195-216: Remove the second redundant tracing::info! call in the
split topology planning flow, preserving the preceding log with the elected
coordinator and planning details. Do not alter the first log or surrounding
topology behavior.
In `@crates/mesh-llm-host-runtime/src/runtime/local_split/tests.rs`:
- Around line 81-97: In the test
noncanonical_gate_returns_standby_without_invoking_package_planning, remove the
package_planning_called flag and replace the
CanonicalCoordinatorGate::Coordinator(_) match arm with unreachable!() or an
equivalent panic. Keep the standby coordinator assertion unchanged.
- Around line 1644-1670: Replace the size-tier comparison in the test with a
direct assertion that the planned quantization matches the expected Q4_0 family
result produced by GgufKvCacheQuant::from_llama_args("q4_0", "q4_0"), as done by
the hermetic test. Keep the policy.default_kv_cache_type assertion and
diagnostic output, but remove the planned == size_tiered assertion so the test
no longer depends on the model-size tier.
In `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs`:
- Around line 792-799: Unify total-byte calculation between
candidate_source_total_bytes and job_spec_with_token by reusing the same helper
and overflow policy. Update the job-spec construction path around
job_spec_with_token to call candidate_source_total_bytes (or the shared
calculation helper) instead of recomputing with checked_add, ensuring planning
and job environment values always agree.
In `@crates/model-package/src/prepare.rs`:
- Around line 182-203: The revision-resolution logic in resolve is too large and
should be isolated. Extract the requested-revision lookup, source SHA and
pipeline-tag extraction, and revision-pinned list_inventory call into a helper
such as resolve_source_revision, returning the resolved revision data needed by
resolve; then replace the inline block with that helper call while preserving
existing error contexts and defaults.
In `@crates/skippy-correctness/src/runner/split_chain.rs`:
- Around line 533-535: Replace the use of args.startup_timeout_secs in the
prediction_return.receive calls within the split-chain prediction flow with a
dedicated prediction/decode round-trip timeout setting. Add or reuse the
appropriate configuration knob for this wait budget, and apply it consistently
at both reported receive sites while preserving the existing error context.
In `@crates/skippy-correctness/src/runner/stage_fa_parity.rs`:
- Around line 91-100: Plumb a configurable model ID through StageFaParityArgs
and use it when constructing the PackageStageRequest in the parity runner
instead of the hardcoded Inkling value. Preserve the current Inkling identifier
as the default if appropriate, while ensuring args.model remains the package
reference.
In `@crates/skippy-quantize/src/gguf_writer.rs`:
- Around line 454-511: The fused w13 tensor is currently streamed twice because
both sources in inkling_w13_tensor_sources reference the full source tensor.
Update the conversion/streaming path to read each fused tensor once, split
alternating rows during that pass, and write the gate and up halves to their
respective GGUF offsets while preserving the existing names, shapes, dtypes, and
parity mapping.
In
`@crates/skippy-server/src/binary_transport/binary_messaging/control_messages.rs`:
- Around line 33-54: Replace the #[allow(clippy::too_many_arguments)] handler
signatures, including handle_stop and the other three affected control-message
handlers, with a shared borrowed ControlMessageContext<'_> plus a small
mutable-state struct for per-request state. Move the common collaborators
(config, topology, runtime, kv, telemetry, streams, wire settings, timeout,
forwarder, and prediction-return resources) into the context, update handlers
and connection.rs call sites to use it, and remove the lint suppressions.
In `@third_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patch`:
- Around line 4983-4985: Remove the unconditional LLM_ARCH_INKLING exclusion
from the test-llama-archs fixture-selection logic and add fixture values for its
architecture-specific hparams—d_rel, rel_extent, shortconv_kernel, and
logit_scale_denom—so Inkling is included in load-path coverage.
In
`@third_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patch`:
- Around line 472-492: Update llama_model_inkling::graph::graph so output_norm
is applied after inp_out_ids trimming when cparams.embeddings_nextn is disabled,
while retaining the full-sequence normalization needed to populate h_nextn when
it is enabled. Preserve the existing result_norm and result_norm_all callbacks
for their respective paths.
In `@tools/relay-fly-legacy/README.md`:
- Around line 8-16: Add explicit relay ID-to-region mappings alongside the
default relay definitions in effective_relay_urls within connections.rs,
preserving the existing URLs and ordering while documenting USW1-2, APS1-1,
EUC1-1, and USE1-1 with their corresponding regions. Use structured metadata or
clear adjacent comments as appropriate to keep the source of truth aligned with
the README.
🪄 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: 4bae7c41-82f0-4368-b273-6d7d313ef289
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (158)
.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-config/src/model_validation.rscrates/mesh-llm-host-runtime/src/inference/skippy/deployment.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/native_mtp_tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/resolution.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.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/protocol/convert.rscrates/mesh-llm-host-runtime/src/protocol/tests/announcements.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/prediction_return.rscrates/skippy-correctness/src/runner/split_chain.rscrates/skippy-correctness/src/runner/stage_fa_parity.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/Cargo.tomlcrates/skippy-model-package/src/package.rscrates/skippy-model-package/src/preflight.rscrates/skippy-protocol/src/binary/activation.rscrates/skippy-protocol/src/binary/mod.rscrates/skippy-protocol/src/binary/types.rscrates/skippy-protocol/src/lib.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/activation.rscrates/skippy-runtime/src/media.rscrates/skippy-runtime/src/package.rscrates/skippy-runtime/src/runtime_events.rscrates/skippy-runtime/src/session.rscrates/skippy-runtime/src/types.rscrates/skippy-server/README.mdcrates/skippy-server/src/binary_transport/binary_messaging.rscrates/skippy-server/src/binary_transport/binary_messaging/async_forwarder.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/binary_messaging/control_messages.rscrates/skippy-server/src/binary_transport/binary_messaging/message_receive.rscrates/skippy-server/src/binary_transport/binary_messaging/reply.rscrates/skippy-server/src/binary_transport/binary_messaging/session_lifecycle.rscrates/skippy-server/src/binary_transport/binary_messaging/session_tracker.rscrates/skippy-server/src/binary_transport/binary_messaging/telemetry.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/preconnect.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/server.rscrates/skippy-server/src/frontend/generation/timeouts.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rscrates/skippy-server/src/frontend/request.rscrates/skippy-server/src/frontend/speculative.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/lib.rscrates/skippy-topology/src/tests.rsdocs/LAYER_PACKAGE_REPOS.mddocs/design/TESTING.mddocs/design/message_protocol.mddocs/skippy/DATA_FLOW.mddocs/skippy/FAMILY_CERTIFY.mddocs/skippy/FAMILY_STATUS.mddocs/skippy/LLAMA_PARITY.mddocs/skippy/NEW_MODEL_ONBOARDING.mddocs/skippy/PIPELINED_VERIFY_WINDOW.mddocs/skippy/SUFFIX_NGRAM_PROPOSER.mddocs/skippy/WAN_SPLIT_PERF.mddocs/skippy/llama-parity-candidates.jsondocs/specs/layer-package-repos.mdscripts/hf-skippy-convert-job.pyscripts/hf-skippy-mtp-certify-job.pyscripts/tests/test_hf_skippy_mtp_certify_job.pyscripts/tests/test_windows_native_runtime_deps.pythird_party/llama.cpp/patches/0048-Support-Laguna-staged-execution.patchthird_party/llama.cpp/patches/0049-Filter-staged-runtime-memory-to-layer-range.patchthird_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patchthird_party/llama.cpp/patches/0051-Add-staged-execution-support-for-Inkling.patchthird_party/llama.cpp/patches/0052-Advance-native-MTP-depth-per-draft-step.patchthird_party/llama.cpp/patches/0053-Replay-multi-depth-MTP-caches-correctly.patchthird_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patchthird_party/llama.cpp/patches/0055-Carry-multimodal-Inkling-embeddings-into-MTP.patchthird_party/llama.cpp/patches/0056-Size-recurrent-memory-for-appended-MTP-layers.patchthird_party/llama.cpp/patches/0057-Complete-Inkling-and-GLM-metadata-integration.patchthird_party/llama.cpp/patches/0058-skippy-balance-filtered-stages-across-devices.patchthird_party/llama.cpp/patches/0059-Preserve-hybrid-verify-window-state-across-trim.patchthird_party/llama.cpp/patches/0060-skippy-retire-accepted-verify-checkpoints-exactly.patchthird_party/llama.cpp/patches/0061-Reject-trims-after-failed-verify-recovery.patchthird_party/llama.cpp/patches/0062-Harden-Inkling-MTP-and-KV-contiguity-state.patchtools/relay-fly-legacy/README.mdwebsite/src/docs/pages/CLI.md
🚧 Files skipped from review as they are similar to previous changes (19)
- docs/skippy/PIPELINED_VERIFY_WINDOW.md
- docs/skippy/WAN_SPLIT_PERF.md
- docs/design/TESTING.md
- crates/skippy-runtime/src/activation.rs
- crates/skippy-server/README.md
- crates/mesh-llm-host-runtime/src/protocol/tests/announcements.rs
- docs/skippy/FAMILY_STATUS.md
- docs/skippy/DATA_FLOW.md
- website/src/docs/pages/CLI.md
- docs/skippy/SUFFIX_NGRAM_PROPOSER.md
- docs/skippy/FAMILY_CERTIFY.md
- crates/skippy-server/src/frontend/generation.rs
- docs/skippy/LLAMA_PARITY.md
- docs/design/message_protocol.md
- crates/mesh-llm-host-runtime/src/mesh/tests/direct_path.rs
- docs/specs/layer-package-repos.md
- docs/LAYER_PACKAGE_REPOS.md
- crates/skippy-server/src/binary_transport/preconnect.rs
- crates/mesh-llm/src/commands/models/mod.rs
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 (1)
crates/skippy-server/src/frontend/embedded_generation.rs (1)
1267-1270: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRetire pipelined checkpoints using the actual commit count.
This check runs before
pipelined_target_commit_count(...)applies active-window/lookahead limits. A window can therefore be retired as fully consumed usingnative_mtp_verify_decision.commit_counteven though the later pipeline logic commits fewer positions, allowing downstream state needed by an in-flight window to be discarded prematurely. Move this retirement check after the finalcommit_countcalculation and use that value.🤖 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/frontend/embedded_generation.rs` around lines 1267 - 1270, Move the verify_checkpoint_no_longer_needed call in the native MTP pipeline flow until after pipelined_target_commit_count computes the final commit_count, and pass that final value instead of native_mtp_verify_decision.commit_count. Preserve the existing window.input_tokens.len() argument and use the resulting check for checkpoint retirement only after active-window/lookahead limits are applied.
🧹 Nitpick comments (2)
crates/skippy-server/src/runtime_state.rs (1)
1400-1405: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the public restore APIs in this regression test.
The test only calls
record_restored_session_token_countdirectly, so it does not verify thatimport_state_for_token_count,import_full_state_for_token_count, andimport_recurrent_state_for_token_countupdate the tracked position. Add coverage through those wrappers.🤖 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/runtime_state.rs` around lines 1400 - 1405, Update the regression test prefix_restore_moves_tracked_position_backwards to invoke the public wrappers import_state_for_token_count, import_full_state_for_token_count, and import_recurrent_state_for_token_count instead of calling record_restored_session_token_count directly, while preserving the tracked-position assertion and covering each wrapper’s backwards-update behavior.crates/skippy-quantize/src/gguf_writer_tests.rs (1)
374-377: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert transformed payloads, not only tensor shape.
This test can pass with incorrect W13 deinterleave or BF16 conversion because it checks only names and dimensions. Add byte-level assertions for both generated gate/up tensors, matching the neighboring MTP test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-quantize/src/gguf_writer_tests.rs` around lines 374 - 377, Extend the test around parse_test_gguf to assert the byte-level payloads of both blk.3.ffn_gate.weight and blk.3.ffn_up.weight, not just their dimensions. Match the neighboring MTP test’s payload assertion approach and expected transformed bytes so W13 deinterleaving and BF16 conversion are validated.
🤖 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/frontend/embedded_generation.rs`:
- Around line 1267-1270: Move the verify_checkpoint_no_longer_needed call in the
native MTP pipeline flow until after pipelined_target_commit_count computes the
final commit_count, and pass that final value instead of
native_mtp_verify_decision.commit_count. Preserve the existing
window.input_tokens.len() argument and use the resulting check for checkpoint
retirement only after active-window/lookahead limits are applied.
---
Nitpick comments:
In `@crates/skippy-quantize/src/gguf_writer_tests.rs`:
- Around line 374-377: Extend the test around parse_test_gguf to assert the
byte-level payloads of both blk.3.ffn_gate.weight and blk.3.ffn_up.weight, not
just their dimensions. Match the neighboring MTP test’s payload assertion
approach and expected transformed bytes so W13 deinterleaving and BF16
conversion are validated.
In `@crates/skippy-server/src/runtime_state.rs`:
- Around line 1400-1405: Update the regression test
prefix_restore_moves_tracked_position_backwards to invoke the public wrappers
import_state_for_token_count, import_full_state_for_token_count, and
import_recurrent_state_for_token_count instead of calling
record_restored_session_token_count directly, while preserving the
tracked-position assertion and covering each wrapper’s backwards-update
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7ad5de6-3b9b-44b7-981b-9bd9924e5caa
📒 Files selected for processing (18)
.github/workflows/queue-unsloth-layer-packages.ymlcrates/mesh-llm-host-runtime/src/runtime/local_split.rscrates/mesh-llm-host-runtime/src/runtime/local_split/tests.rscrates/mesh-llm-host-runtime/src/runtime/startup_retry.rscrates/skippy-correctness/src/cli.rscrates/skippy-correctness/src/runner/stage_fa_parity.rscrates/skippy-quantize/src/backend.rscrates/skippy-quantize/src/gguf_writer.rscrates/skippy-quantize/src/gguf_writer_tests.rscrates/skippy-quantize/src/tensor_map.rscrates/skippy-quantize/src/tokenizer_metadata.rscrates/skippy-server/src/binary_transport/binary_messaging/session_tracker.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/embedded_generation/lifecycle.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rscrates/skippy-server/src/runtime_state.rsscripts/hf-skippy-convert-job.pythird_party/llama.cpp/patches/0063-Harden-Inkling-MTP-sidecar-and-verify-cleanup.patch
💤 Files with no reviewable changes (1)
- crates/mesh-llm-host-runtime/src/runtime/local_split.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-server/src/frontend/embedded_generation.rs`:
- Around line 1366-1382: Extract the separable pipelined and serial
verify-window handling from the oversized source into a responsibility-named
module such as verify_windows, including the related implementation and tests.
Update references and module wiring so behavior remains unchanged, keep the
extracted module under 1,000 lines, and reduce the original file below 1,000
lines.
🪄 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: f7ae28fa-ef74-4642-8b89-13daefd10c95
📒 Files selected for processing (2)
crates/skippy-quantize/src/gguf_writer_tests.rscrates/skippy-server/src/frontend/embedded_generation.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/embedded_generation/lifecycle.rs (1)
490-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
extensionreset in the restore test.The implementation clears both
ngramandextension, but the test only asserts thatngramis cleared. Populateextensionin the fixture and asserteffective.extension.is_none()so this part of the contract is protected.🤖 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/frontend/embedded_generation/lifecycle.rs` around lines 490 - 497, Update restored_prefix_bypasses_history_ngram_for_that_request to populate config.extension in the fixture, then assert effective.extension.is_none() alongside the existing ngram assertion while confirming the original config remains unchanged.
🤖 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.
Nitpick comments:
In `@crates/skippy-server/src/frontend/embedded_generation/lifecycle.rs`:
- Around line 490-497: Update
restored_prefix_bypasses_history_ngram_for_that_request to populate
config.extension in the fixture, then assert effective.extension.is_none()
alongside the existing ngram assertion while confirming the original config
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e142f4bd-1f78-44ad-80c0-dcc57bb80613
📒 Files selected for processing (1)
crates/skippy-server/src/frontend/embedded_generation/lifecycle.rs
michaelneale
changed the title
|
@coderabbitai Can we close #1017 #1095 and #110 in favour of this single PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Mesh-LLM/mesh-llm"
echo "== Repository permissions =="
gh api "repos/${repo}" --jq '{full_name, owner: .owner.login, permissions}'
echo
for number in 1017 1095 110; do
echo "== Issue #${number} =="
gh issue view "$number" --repo "$repo" --json number,title,state,author,body,url,labels,assignees
done
echo
echo "== Requester permission =="
gh api "repos/${repo}/collaborators/ndizazzo/permission" --jq '{user: "ndizazzo", permission}'Length of output: 21074
Please explicitly confirm that you want me to close You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Mesh-LLM/mesh-llm"
echo "=== Repository ==="
gh api "repos/${repo}" --jq '{owner: .owner.login, owner_type: .owner.type, default_branch: .default_branch, archived: .archived}'
for number in 1017 1095 110; do
echo
echo "=== Issue #${number} ==="
gh issue view "$number" --repo "$repo" --json number,title,state,author,assignees,labels,body,url,comments \
--jq '{number, title, state, author: .author.login, assignees: [.assignees[].login], labels: [.labels[].name], body, url, comments: [.comments[] | {author: .author.login, body: .body, createdAt: .createdAt}]}'
done
echo
echo "=== Pull request `#1118` metadata ==="
gh pr view 1118 --repo "$repo" --json number,title,state,author,body,url,closingIssuesReferences \
--jq '{number, title, state, author: .author.login, url, closingIssuesReferences: [.closingIssuesReferences[] | {number, title, url}], body}'Length of output: 106726
I would not change Because this is a multi-item closure, please have a repository maintainer explicitly confirm: “Close You are interacting with an AI system. |
e871eeb to
a3a60e2
Compare
ndizazzo
left a comment
ndizazzo
left a comment
There was a problem hiding this comment.
Let's split mesh/node.rs and skippy-model-package/preflight.rs before merging...
They were already over the repo’s 2,000-line limit on the base branch, and this PR grows them even larger. This seems like a good point to extract the responsibilities into named modules and bring both files back under the limit.
|
Nick review follow-up is pushed in
Local validation: |
|
@ndizazzo ready for another look |
|
The remaining actionable Rust lifecycle concern carried from superseded #1100 is fixed in Validation: 351 |
|
@ndizazzo @i386 the review follow-up and fresh Inkling checkpoint are pushed at Nick's three requested changes are included: conversion split sizing, exact manifest-shard validation, and readiness diagnostics using the configured RTT threshold. The large-file extractions are also retained. Fresh Inkling evidence is deliberately bounded: real all-CUDA split generation, recurrent depth-2 verification recovery, 2/2 sequential native tool loops, exact replay 3,531/3,531, and a clean native fatal-pattern scan. Both overlap phases still miss the formal bar, changed-tail prefix reuse is still zero, and the 480,000-character streaming probe ended without an SSE data event when both SSH-launched Mesh processes stopped together; it is not claimed as a long-context pass. The PR body and operator docs now say this explicitly. Final local/cloud gates: clean 64-patch apply, 351/351 |
2 participants
Why
Poolside Laguna and TML Inkling were developed on separate model branches while the hybrid/recurrent verification recovery they share remained in #1100. This PR gives those changes one linear patch queue on current
mainand removes the overlapping recovery implementations.It consolidates #1017, #1095, and #1100. The support claims are intentionally asymmetric: the pinned Poolside Q4 package is promoted with current three-stage CUDA and 128K evidence; the pinned Inkling Q2 package is documented as an experimental text candidate with explicit remaining gaps.
What changes
Stopargumentsspelling0048..0064There is no Mesh wire-format break. Stage generation 4 is advertised and required explicitly, so older nodes fail closed before exchanging incompatible stage-control frames.
Poolside Laguna Q4: supported package
Artifact:
meshllm/laguna-s-2.1-Q4_K_M-layers@0c467ad441ee94cb5a76f626294d963c4048507dThe branch-local Linux release host was exercised on three temporary Australian Vast nodes using ordinary Mesh planning and direct Iroh/QUIC paths (normally about 8–9 ms during the run):
0..2525..3939..48131072; one lane; Q4_0 K/V; F16 activation wireObserved results:
tool_callsentry; the tool-result follow-up returned a normal final answersession already exists, all-lanes-busy, unsupported-trim, fatal, decode, reset, proactive-eviction, orphan, or error-level eventThis satisfies the repository context-capacity contract for the pinned Q4_K_M package and promotes that package—not every Laguna quant—to the support matrix.
Poolside limits
Inkling Q2: experimental text candidate
Artifact:
meshllm/inkling-UD-Q2_K_XL-layers@9b4b91a7ddd978dd7a01679bc977f6e53777f2c7A fresh all-CUDA run used one 4 x 96 GB node and one 48 GB node on a direct Iroh/QUIC path whose unloaded RTT was about 5 ms. Ordinary automatic placement produced
0..65 / 65..66, four lanes, F32 activation wire, Q4_0 K/V, and a 131,072-token allocation.What is proven on the current branch:
This topology is evidence that the package runs; it is not a deployment recommendation. The 65-layer head took roughly 14 minutes to become ready, reserved about 589 GiB of CUDA host compute workspace, and briefly starved membership processing during startup. Load also inflated coordination RTT far beyond the approximately 5 ms physical path.
Remaining gaps are concrete:
The operator path and these limits are documented in
docs/SKIPPY_SPLITS.md; Inkling remains outside the promoted support matrix indocs/skippy/FAMILY_STATUS.md.Nick review follow-up
mesh/node.rs(now 1,840 lines) and artifact validation/reporting fromskippy-model-package/preflight.rs(now 1,892 lines)--split-max-sizeby increasing the byte-balanced GGUF split count when required, including MTP-only tensor selectionValidation
just release-host-buildcargo fmt --all --checkcargo test -p skippy-server --libtest-chatInkling parser regressionFresh GitHub CI for the final head must finish green before merge. Nick's prior change request is addressed in the branch but still requires re-review.
Tracks #1090 and #1025. Consolidates #1017, #1095, and #1100.