Add Ngram Suffix Proposer - #1037
Conversation
The adaptive verify window was never enabled on the split-serving path: to_embedded_openai_args hardcoded adaptive_speculative_window = false. With a fixed window, an early reject never shrank the window, so a sustained reject storm kept proposing at full depth and paying the full 2-round-trip recovery cost per token. On a WAN split this measured as ~40% throughput loss with N-gram speculation ON versus OFF, despite high per-token acceptance. Enable the adaptive window whenever speculation actually proposes a window (ngram or draft mode). The existing shrink_adaptive_window logic then narrows the window toward the observed accept depth after an early reject, cutting recovery frequency. Adds a regression test asserting ngram speculation turns the adaptive window on.
Adds a third speculative n-gram proposer kind, `suffix`: a pure-Rust longest-suffix matcher that is not bound by llama.cpp's 4-token match window (NGRAM_CACHE_MAX_NGRAM). It indexes committed history by a hashed seed n-gram, finds the longest verbatim earlier occurrence of the query suffix (up to SUFFIX_NGRAM_MAX_WINDOW = 64), and copies the tokens that followed it, scaling draft length with match length. This targets input-grounded, repetitive workloads — re-emitting a file with a small edit, echoed tool output, repeated identifiers — where a long match is unambiguous and justifies a long, high-confidence draft. It stays silent below `ngram_min`, so it is roughly neutral on freeform prose. Implementation is additive and default-off: - SuffixNgramProposer + a HistoryNgramProposer enum dispatching Cache/Suffix behind the existing propose(committed, prefix, max) contract; call sites change mechanically from CachedNgramProposer to the enum. - No FFI/llama.cpp changes; no changes to transport, verify, or recovery paths. Reuses NgramProposalConfig fields (min_ngram/max_ngram/ max_proposal_tokens). - Config selectable via `ngram_proposer = "suffix"` (mesh config + CLI).
Documents the suffix proposer in USAGE.md and CONFIGURATION.md with a config example. Adds evals/skippy-suffix-proposer-bench.py: attaches to running endpoints, compares off/simple/cache/suffix arms across edit, tool-loop and chat workloads, reading decode tok/s and acceptance from the server timings. Requires a >=2-stage split.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesSuffix N-gram speculation is added across configuration, package validation, CLI parsing, standalone serving, native-MTP verification, telemetry, benchmarks, and documentation. Legacy proposer and extension controls are removed, while split-topology lock parsing and explicit disabled KV-cache materialization are added. Suffix speculation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
57-2000: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
generate_embedded_stage_zero_tokens/ this file substantially exceed the repo's Rust file-size guideline.This PR adds new logic (lines 842, 853-855, 1364-1371, 1399, 1419-1428, 1964-1966) inside an already ~2000-line file and a single function spanning the bulk of it. As per coding guidelines, "When modifying a Rust file already over 1,000 lines, extract any separable responsibility into a named module, keep the new file under 1,000 lines, and move or add its tests with the extracted behavior," and "Do not add Rust source files over 2,000 lines; split approaching oversized files by responsibility." Extracting the decode-loop's native-MTP/verify-window/speculative-proposal orchestration (much of which is already split into
native_mtp/*submodules) further out of this file/function would help bring it into compliance.🤖 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 57 - 2000, Extract the separable native-MTP, verify-window, and speculative-proposal orchestration from generate_embedded_stage_zero_tokens into a named module or helper responsible for decode-loop coordination. Move the associated implementation and tests with that behavior, keep the extracted Rust file under 1,000 lines, and leave generate_embedded_stage_zero_tokens focused on request/session and stage orchestration while preserving existing behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/speculative/suffix.rs (1)
176-198: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded candidate scan in
lookup()could degrade on repetitive workloads.
bucket.iter().rev()examines every prior occurrence of the seed with no cap, and each candidate runs an O(max_window) backward comparison. For highly repetitive content (e.g., many prior occurrences of a common 3–8 token seed, plausible in code completion), the per-decode-step proposal cost can grow with total occurrence count for the life of a long request. The existing testretains_useful_matches_beyond_eight_seed_occurrencesintentionally exercises this unbounded path, so this is likely a known tradeoff, but a bound on the number of most-recent candidates examined (or a time budget) would cap worst-case latency without materially hurting match quality.♻️ Sketch: cap candidates examined
- for &end in bucket.iter().rev() { + const MAX_CANDIDATES: usize = 32; + for &end in bucket.iter().rev().take(MAX_CANDIDATES) { let end = end as usize; if end + 1 >= committed_len { continue; }🤖 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/speculative/suffix.rs` around lines 176 - 198, Bound the candidate scan in lookup by examining only a fixed number of the most-recent entries from bucket.iter().rev(), while preserving the existing filtering and match selection behavior. Define or reuse an appropriate candidate-limit constant near the lookup logic, increment candidates_examined only for candidates actually evaluated, and ensure the useful-match behavior covered by retains_useful_matches_beyond_eight_seed_occurrences remains intact.
🤖 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/speculative.rs`:
- Around line 207-208: Remove the #[allow(clippy::too_many_lines)] attribute and
split resolve_decode_config into semantically named helpers for its distinct
responsibilities, including ngram-kind resolution, extension-controls
application, and verify-window resolution. Keep resolve_decode_config focused on
orchestration while preserving the existing configuration behavior.
In `@crates/skippy-server/src/frontend/embedded_generation.rs`:
- Around line 1419-1428: Update the speculative request construction in
crates/skippy-server/src/binary_transport/binary_messaging.rs and
crates/skippy-server/src/frontend/generation/server.rs:379-380 so ngram_max
carries speculative.ngram.max_proposal_tokens for every proposer kind, including
Cache and Suffix, or remove the redundant field. In
crates/skippy-server/src/frontend/embedded_generation.rs#L1419-L1428, update the
propose_configured_ngram_tokens call to pass proposal_limit directly without
min(request.ngram_max), preserving the proposer’s internal max_proposal_tokens
enforcement.
---
Outside diff comments:
In `@crates/skippy-server/src/frontend/embedded_generation.rs`:
- Around line 57-2000: Extract the separable native-MTP, verify-window, and
speculative-proposal orchestration from generate_embedded_stage_zero_tokens into
a named module or helper responsible for decode-loop coordination. Move the
associated implementation and tests with that behavior, keep the extracted Rust
file under 1,000 lines, and leave generate_embedded_stage_zero_tokens focused on
request/session and stage orchestration while preserving existing behavior.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/speculative/suffix.rs`:
- Around line 176-198: Bound the candidate scan in lookup by examining only a
fixed number of the most-recent entries from bucket.iter().rev(), while
preserving the existing filtering and match selection behavior. Define or reuse
an appropriate candidate-limit constant near the lookup logic, increment
candidates_examined only for candidates actually evaluated, and ensure the
useful-match behavior covered by
retains_useful_matches_beyond_eight_seed_occurrences remains intact.
🪄 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: 4852109b-584a-4961-afd6-8f326208730c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.tomlcrates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/model_validation.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.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/resolver/translation.rscrates/skippy-model-package/src/preflight.rscrates/skippy-server/Cargo.tomlcrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation/server.rscrates/skippy-server/src/frontend/generation/types.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/native_mtp/decode.rscrates/skippy-server/src/frontend/native_mtp/hybrid.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/speculative/standalone.rscrates/skippy-server/src/frontend/speculative/suffix.rscrates/skippy-server/src/frontend/tests/multimodal.rscrates/skippy-server/src/frontend/tests/prompting.rsdocs/README.mddocs/USAGE.mddocs/skippy/CONFIGURATION.mddocs/skippy/PIPELINED_VERIFY_WINDOW.mddocs/skippy/SUFFIX_NGRAM_PROPOSER.mddocs/specs/layer-package-repos.mdevals/skippy-suffix-proposer-bench.py
💤 Files with no reviewable changes (2)
- crates/skippy-server/src/frontend/generation_flow.rs
- crates/skippy-server/src/frontend/tests/multimodal.rs
|
Integrated PR #1026 ( Scenario
Results versus the previous runsValues are median server decode TPS / end-to-end wall TPS.
Against the current MTP-only row, MTP+Suffix is 4.57x server TPS and @michaelneale: are the stages actually busy in parallel?For Cache and Suffix, yes; for MTP-only and Simple, no.
Suffix contributes 2,135 refill tokens across the five measured requests, The standalone regression is also revealing: Cache and Suffix accept 96.4% This rerun proves the pipeline mechanism, but not the controlled-WAN claim: Correctness caveat: every arm was deterministic within-arm. Target-only, all |
Suffix lookup now early-exits once a full-window match is found, bounding scan cost on repetitive content. Keeps the full scan otherwise so older, longer matches are still found (retains_useful_matches_beyond_eight_seed_occurrences). Adds docstrings across the N-gram proposer surface (suffix, standalone, and the speculative config types) for the docstring-coverage gate.
Capture the draft-model speculative-decode pipelining findings for branch wip/wan-direct-prediction-return so the work can be picked up: what is proven over WAN, the draft-vs-ngram acceptance-survival result, a Cohere/SWA trim limitation, the 2-node bringup config trap, and pointers to the related ngram-widening PRs (#1037, #1026, #875, #887). Assisted-by: goose
Transport-core status/handoff for PR #1028: what is proven over WAN, the landed keep-set, why fixed pipeline depth is diagnostic-only, the go-forward suffix-ngram + adaptive-depth direction (#1037), and the 2-node bringup config trap. Draft-model work is deferred on wip/wan-draft-ahead. Assisted-by: goose
The binary transport only filled ngram_min/ngram_max when the proposer kind was Simple, so embedded stage-0 requests with cache or suffix configs arrived with ngram_max=0 and the standalone fallback proposed nothing. Mirror the host runtime translation and pass the configured limits through for every proposer kind.
…treams Ported from the WAN lab branch (wip/wan-direct-prediction-return, c340f74), where it was validated live on a ~26ms WAN split. open_stage_transport_stream re-applied the formation-time MAX_SPLIT_RTT_MS ceiling to every fresh operational stream, so per-request direct-return sinks were rejected under normal WAN RTT jitter while pooled forward lanes stayed healthy - surfacing as ready-handshake timeouts and 502s on an already-admitted split. Split admission still gates eligibility via gossiped, hysteresis-smoothed RTT plus re-election; operational streams now warn and proceed.
…etup Ported from the WAN lab branch (46108cf). Over a WAN mesh the return sink connects to a local bridge alias, but the remote ready byte only arrives after the bridge cold-establishes a fresh stage QUIC connection (~10s budget) and the remote handler dials its local server. 5s timed out during that cold setup on a healthy ~26ms split; forward lanes already use a 20s budget. Match it.
…g the model task Observed live on a real WAN split (Sydney M5 <-> AU 4090): one transient direct-return 502 led periodic_check to mark the remote stage unavailable; after the 75s grace the coordinator withdrew the topology. The Withdraw event returned StartupLoopControl::Break, so startup_local_model_loop tore down and the task ended permanently - while the remote worker sat healthy, logging 'standing by for stage assignment' forever. Only recovery was manually restarting both nodes with a fresh token. Make withdraw non-terminal: a new RelaunchSplit control/outcome runs the full existing teardown, then loops back to the launch phase and re-enters wait_for_split_participants, relaunching the split when an eligible peer returns. The stop channel is checked before relaunch so explicit shutdown still wins. LocalFallback (model fits locally) is unchanged. The participant-wait loop's 30s cadence and stable-participant gating act as the natural retry throttle; no extra backoff added.
…oser Reconciles the standalone suffix N-gram proposer with Mesh-LLM#1026's positional-MTP n-gram pipelining rework, which had diverged the config foundation. Key decisions: - Standalone N-gram stays allowed: validate() permits a request-local ngram proposer without native MTP, and the resolver produces a disabled native-MTP config plus "ngram" mode for standalone plans. - Unify the decode loop on HistoryNgramProposer (cache + suffix superset) so the composite pipeline, verify-window path, and standalone path share one proposer type; drop the now-unused CachedNgramProposer::from_config. - Adopt Mesh-LLM#1026's simplified NgramExtensionConfig ({max_tokens}) and the top-level arg cleanup (ngram bounds derive from speculative config). - Drop the "simple" proposer kind: Mesh-LLM#1026 removed its skippy-ffi backing (skippy_ngram_simple_draft), leaving cache and suffix. Enum, validation, resolver, CLI, preflight, docs, and tests updated accordingly. Gate the cache max-window (<=4) check on the cache kind in both the frontend validate() and package preflight so suffix windows (<=64) are not rejected. Build and lib tests pass across the affected crates.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/skippy-server/src/kv_integration/config.rs (1)
79-92: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftFail closed when recurrent-state detection is inconclusive.
model_requires_recurrent_statereturnsfalsewhen model inspection cannot open the model or enumerate tensors. Incrates/skippy-server/src/binary_transport/binary_messaging/connection.rs, Line 106 negates this result and therefore treats an unknown model as safe forVerifyWindow. Use a fallible or tri-state helper for this protocol gate and reject when inspection is inconclusive; retain the permissive fallback only where it cannot enable positional speculation.🤖 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/kv_integration/config.rs` around lines 79 - 92, Update model_requires_recurrent_state and its VerifyWindow caller in the binary messaging connection flow to distinguish confirmed non-recurrent models from inconclusive inspection. Propagate inspection failures as an unknown/error state, and make the protocol gate reject unknown models instead of negating the result to allow them; preserve permissive behavior only for callers that cannot enable positional speculation.crates/mesh-llm-config/src/model_validation.rs (1)
419-427: 🎯 Functional Correctness | 🟠 MajorRemoving
"ngram"frommode's accepted values — same compatibility concern asbuilt_in_schema.rs.See the companion comment on
crates/mesh-llm-config/src/model/built_in_schema.rs(lines 604-610) — this is the enforcement half of the same schema tightening and carries the same backward-compatibility risk for existingspeculative.mode = "ngram"configs.🤖 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-config/src/model_validation.rs` around lines 419 - 427, Update validate_speculative to preserve “ngram” as an accepted value for config.mode, matching the compatibility behavior required by the companion built-in schema definition. Keep the existing validation for “auto”, “disabled”, and “draft” unchanged.
♻️ Duplicate comments (1)
crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs (1)
196-207: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
#[allow(clippy::too_many_lines)]onresolve_decode_config— previously flagged, still unresolved.
resolve_decode_configgrew further with the suffix-proposer handling added in this PR; the#[allow(...)]still bypasses the length/complexity guardrail instead of extracting the already-distinct sub-blocks (ngram-kind resolution, extension-controls application, verify-window resolution) into named helpers.As per coding guidelines: "Do not add Rust methods or functions exceeding the configured Clippy line-count or cognitive-complexity limits; split them into semantically named helpers" and "do not use
#[allow(...)]to silence them without a clear reason and developer approval."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs` around lines 196 - 207, Remove the #[allow(clippy::too_many_lines)] attribute from resolve_decode_config and split its distinct logic into semantically named helpers. Extract ngram-kind resolution, extension-controls application, and verify-window resolution while preserving the existing configuration precedence and behavior.Source: Coding guidelines
🧹 Nitpick comments (6)
crates/mesh-llm-host-runtime/src/runtime/startup_handles.rs (1)
1242-1412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the relaunch loop/outcome handling into a named submodule. This file is already well over 1,000 lines and this change adds the launch loop plus
startup_resolve_loop_outcome/StartupLoopOutcomerelaunch logic to it. As per coding guidelines, "When modifying a Rust file already over 1,000 lines, extract any separable responsibility into a named module, keep the new file under 1,000 lines, and move or add its tests with the extracted behavior." The split-relaunch lifecycle is a natural separable 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/mesh-llm-host-runtime/src/runtime/startup_handles.rs` around lines 1242 - 1412, Extract the split-relaunch lifecycle from the large startup_handles module into a named Rust submodule, including the launch loop, startup_resolve_loop_outcome, StartupLoopOutcome, and their related state/helpers. Update call sites and visibility/imports so behavior remains unchanged, and move or add tests for the extracted relaunch behavior in the new module, keeping it under 1,000 lines.Source: Coding guidelines
crates/mesh-llm-host-runtime/src/runtime/split_planning.rs (1)
208-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating the shared post-planning tail.
plan_locked_runtime_slice_topology_with_resourcesandplan_runtime_slice_topology_with_resourcesdiffer only in the planning call (plan_locked_topologyvsplan_runtime_slice_topology_result); theparticipant_by_id/map_runtime_slice_stages/sort_by_key/validate_split_capacity/tracingtail is identical. Extracting a helper that takes the resolvedTopologyPlan(or stages) would keep the two paths from drifting.🤖 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/split_planning.rs` around lines 208 - 256, The post-planning logic in plan_locked_runtime_slice_topology_with_resources and plan_runtime_slice_topology_with_resources is duplicated and should be centralized. Extract a helper that accepts the resolved TopologyPlan (or mapped stages plus required metadata) and performs participant indexing, stage mapping and sorting, validate_split_capacity, validation tracing, and PlannedRuntimeSliceTopology construction; have both functions invoke it after their distinct planning calls while preserving existing inputs and behavior.crates/skippy-bench/src/telemetry_report.rs (1)
143-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider an explicit timeout on the finalize call.
finalize_onlybuilds a barereqwest::blocking::Client::new()with no explicit timeout configured. Depending on the exact reqwest 0.12 default forblocking::Client, this may already carry an implicit ~30s timeout, but that's not obvious from the call site and isn't tuned for this endpoint (which may need longer if the metrics server is still flushing spans under load). An explicit.timeout(...)on the client/request would make the behavior self-documenting and tunable independent of the library default.🤖 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-bench/src/telemetry_report.rs` around lines 143 - 166, Update finalize_only’s reqwest blocking client configuration to apply an explicit, self-documenting timeout suitable for metrics-server finalization, using the project’s existing timeout constant or configuration if available. Preserve the current finalize request flow and error handling while ensuring the client construction propagates any configuration error correctly.third_party/llama.cpp/patches/0018-Expose-stateful-N-gram-cache-ABI.patch (1)
31-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
cache->dynamic/cache->static_cacheare allocated but never populated.
skippy_ngram_cache_update(called from bothresetandappend) only feedscache->context.cache->dynamicandcache->static_cachestay empty for the lifetime of the cache, yetskippy_ngram_cache_draftstill passes them intocommon_ngram_cache_draft, which (per upstream lookup-decoding design) treatsnc_dynamicas a second, lower-confidence fallback tier. Withdynamicalways empty, that fallback tier can never contribute a draft, and the strict/lax threshold split reinforced by patch 0021 has no effect for this path — the cache degrades to context-only lookup.If this single-tier behavior is intentional for the stateful per-session cache, consider documenting it explicitly (and dropping the unused fields/params to avoid confusion); if not,
dynamicshould also be updated onappend.Also applies to: 44-84, 118-163
🤖 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/0018-Expose-stateful-N-gram-cache-ABI.patch` around lines 31 - 38, Update skippy_ngram_cache_update, including its reset and append paths, so cache->dynamic is populated when tokens are appended and available to skippy_ngram_cache_draft as the lower-confidence fallback tier. Keep cache->context behavior unchanged, and either populate cache->static_cache according to its intended tier semantics or remove/document it if single-tier behavior is deliberate.crates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rs (1)
398-406: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestore the removed proposer-kind assertion.
The assertion that the resolved cache proposer's
ngram.kindequalsCachewas dropped frompackage_composite_strategy_resolves_native_mtp_with_cache_extension. This test is specifically about composite (mtp+cache) resolution, where verifying the correct proposer kind was actually selected is a meaningful regression guard distinct from just checkingmin_ngram/max_ngram/max_proposal_tokens.✅ Suggested restoration
let ngram = resolved .speculative .decode .ngram .as_ref() .expect("cache proposer should resolve"); + assert_eq!(ngram.kind, skippy_server::NgramProposerKind::Cache); assert_eq!(ngram.min_ngram, 2); assert_eq!(ngram.max_ngram, 4); assert_eq!(ngram.max_proposal_tokens, 9);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rs` around lines 398 - 406, Restore the proposer-kind assertion in package_composite_strategy_resolves_native_mtp_with_cache_extension by verifying the resolved ngram.kind is Cache alongside the existing ngram configuration assertions. Keep the current min_ngram, max_ngram, and max_proposal_tokens checks unchanged.crates/mesh-llm-config/src/model_validation.rs (1)
541-575: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuffix window limits (
min >= 3,max <= 64) are hardcoded here and duplicated inskippy-model-package/preflight.rs.Both
validate_speculative_proposer_controlshere andvalidate_ngram_proposerincrates/skippy-model-package/src/preflight.rs(lines 837-867, per the emittedunsupported_ngram_suffix_windowcheck) independently hardcode the same3/64bounds. Extracting a shared constant (e.g., re-exported from the crate that owns the actual suffix-proposer runtime limit) would prevent the two validators from silently drifting apart if the limit changes.🤖 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-config/src/model_validation.rs` around lines 541 - 575, Extract the suffix proposer window bounds used by validate_speculative_proposer_controls into shared constants owned by the crate defining the runtime limit, then reuse those constants in both this validation and validate_ngram_proposer in preflight.rs. Remove the duplicated literal 3/64 bounds while preserving the existing min/max validation behavior and diagnostics.
🤖 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-cli/src/lib.rs`:
- Around line 15-16: Restore the crate-root re-export for
SpeculativeNgramProposerCli in the public exports of lib.rs so downstream
mesh_llm_cli::SpeculativeNgramProposerCli imports continue to compile;
alternatively, bump the crate’s major version if the removal is intentional.
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs`:
- Around line 143-145: Update the n-gram mode override in the speculative
resolver and the n-gram-building gate in resolve_decode_config to honor
requested_strategy. When strategy is explicitly "disabled", do not build n-gram
configuration, derive n-gram effective strategies, or force mode to "ngram";
preserve the existing behavior for other strategies.
In `@crates/mesh-llm-host-runtime/src/runtime/model_lifecycle.rs`:
- Line 407: Update the runtime-loaded model lifecycle path where
split_topology_lock is set to None so it forwards
ctx.options.split_topology_lock to the planner. Preserve fail-closed behavior
when a topology lock is configured, ensuring local_split.rs selects the locked
planner; alternatively, explicitly reject runtime loads under a configured lock.
In `@crates/mesh-llm-host-runtime/src/runtime/run_auto.rs`:
- Around line 601-604: Extract the node/plugin startup orchestration around
run_auto in crates/mesh-llm-host-runtime/src/runtime/run_auto.rs (anchor lines
601-604) into a semantic run_auto submodule, keeping the new module under 1,000
lines and preserving behavior. Also extract the additional-model startup
orchestration around crates/mesh-llm-host-runtime/src/runtime/serving_surface.rs
line 1128 into a semantic module, likewise keeping it under 1,000 lines; both
sites require refactoring.
In `@docs/SKIPPY_SPLITS.md`:
- Around line 94-102: Replace the private .local hostnames in the topology-lock
example at docs/SKIPPY_SPLITS.md lines 94-102 with neutral placeholders such as
<node-a> and <node-b>. Make the same replacement in
website/src/docs/pages/CLI.md lines 225-234 and explain there that users must
provide their own node selectors.
In `@SKIPPY_PROTOCOL_TODO.md`:
- Around line 158-165: Update the completed “Make speculative positions
authoritative in stage-state” checklist entry in SKIPPY_PROTOCOL_TODO.md from
stage-state v9 to v10, preserving the existing authoritative-position semantics
and compatibility guidance.
---
Outside diff comments:
In `@crates/mesh-llm-config/src/model_validation.rs`:
- Around line 419-427: Update validate_speculative to preserve “ngram” as an
accepted value for config.mode, matching the compatibility behavior required by
the companion built-in schema definition. Keep the existing validation for
“auto”, “disabled”, and “draft” unchanged.
In `@crates/skippy-server/src/kv_integration/config.rs`:
- Around line 79-92: Update model_requires_recurrent_state and its VerifyWindow
caller in the binary messaging connection flow to distinguish confirmed
non-recurrent models from inconclusive inspection. Propagate inspection failures
as an unknown/error state, and make the protocol gate reject unknown models
instead of negating the result to allow them; preserve permissive behavior only
for callers that cannot enable positional speculation.
---
Duplicate comments:
In `@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs`:
- Around line 196-207: Remove the #[allow(clippy::too_many_lines)] attribute
from resolve_decode_config and split its distinct logic into semantically named
helpers. Extract ngram-kind resolution, extension-controls application, and
verify-window resolution while preserving the existing configuration precedence
and behavior.
---
Nitpick comments:
In `@crates/mesh-llm-config/src/model_validation.rs`:
- Around line 541-575: Extract the suffix proposer window bounds used by
validate_speculative_proposer_controls into shared constants owned by the crate
defining the runtime limit, then reuse those constants in both this validation
and validate_ngram_proposer in preflight.rs. Remove the duplicated literal 3/64
bounds while preserving the existing min/max validation behavior and
diagnostics.
In
`@crates/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rs`:
- Around line 398-406: Restore the proposer-kind assertion in
package_composite_strategy_resolves_native_mtp_with_cache_extension by verifying
the resolved ngram.kind is Cache alongside the existing ngram configuration
assertions. Keep the current min_ngram, max_ngram, and max_proposal_tokens
checks unchanged.
In `@crates/mesh-llm-host-runtime/src/runtime/split_planning.rs`:
- Around line 208-256: The post-planning logic in
plan_locked_runtime_slice_topology_with_resources and
plan_runtime_slice_topology_with_resources is duplicated and should be
centralized. Extract a helper that accepts the resolved TopologyPlan (or mapped
stages plus required metadata) and performs participant indexing, stage mapping
and sorting, validate_split_capacity, validation tracing, and
PlannedRuntimeSliceTopology construction; have both functions invoke it after
their distinct planning calls while preserving existing inputs and behavior.
In `@crates/mesh-llm-host-runtime/src/runtime/startup_handles.rs`:
- Around line 1242-1412: Extract the split-relaunch lifecycle from the large
startup_handles module into a named Rust submodule, including the launch loop,
startup_resolve_loop_outcome, StartupLoopOutcome, and their related
state/helpers. Update call sites and visibility/imports so behavior remains
unchanged, and move or add tests for the extracted relaunch behavior in the new
module, keeping it under 1,000 lines.
In `@crates/skippy-bench/src/telemetry_report.rs`:
- Around line 143-166: Update finalize_only’s reqwest blocking client
configuration to apply an explicit, self-documenting timeout suitable for
metrics-server finalization, using the project’s existing timeout constant or
configuration if available. Preserve the current finalize request flow and error
handling while ensuring the client construction propagates any configuration
error correctly.
In `@third_party/llama.cpp/patches/0018-Expose-stateful-N-gram-cache-ABI.patch`:
- Around line 31-38: Update skippy_ngram_cache_update, including its reset and
append paths, so cache->dynamic is populated when tokens are appended and
available to skippy_ngram_cache_draft as the lower-confidence fallback tier.
Keep cache->context behavior unchanged, and either populate cache->static_cache
according to its intended tier semantics or remove/document it if single-tier
behavior is deliberate.
🪄 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: 6d692003-5687-455b-a283-f174574273db
📒 Files selected for processing (137)
SKIPPY_PROTOCOL_TODO.mdcrates/llama-spec-bench/README.mdcrates/llama-spec-bench/src/main.rscrates/mesh-llm-cli/src/benchmark.rscrates/mesh-llm-cli/src/lib.rscrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/candidates.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/tests.rscrates/mesh-llm-commands/src/gpus/tune/benchmark/trial_config.rscrates/mesh-llm-commands/src/gpus/tune/output_types.rscrates/mesh-llm-commands/src/gpus/tune/output_values.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/model/built_in_schema/control_behavior/speculative.rscrates/mesh-llm-config/src/model_validation.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/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rscrates/mesh-llm-host-runtime/src/mesh/stage_transport.rscrates/mesh-llm-host-runtime/src/plugin/config.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local/native_runtime_events/tests.rscrates/mesh-llm-host-runtime/src/runtime/local_split.rscrates/mesh-llm-host-runtime/src/runtime/local_split/coordinator.rscrates/mesh-llm-host-runtime/src/runtime/local_split/recovery.rscrates/mesh-llm-host-runtime/src/runtime/local_split/test_support.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.rscrates/mesh-llm-host-runtime/src/runtime/options.rscrates/mesh-llm-host-runtime/src/runtime/run_auto.rscrates/mesh-llm-host-runtime/src/runtime/serving_surface.rscrates/mesh-llm-host-runtime/src/runtime/split_planning.rscrates/mesh-llm-host-runtime/src/runtime/split_topology_lock.rscrates/mesh-llm-host-runtime/src/runtime/startup_handles.rscrates/mesh-llm-host-runtime/src/runtime/startup_models.rscrates/mesh-llm-host-runtime/src/runtime/survey.rscrates/mesh-llm-host-runtime/tests/fixtures/config_schema_defaults_ui_reference.jsoncrates/mesh-llm-host-runtime/tests/fixtures/schema_driven_controls_valid.tomlcrates/mesh-llm-system/src/autoupdate.rscrates/mesh-llm-ui/src/features/configuration/api/config-adapter.test.tscrates/mesh-llm-ui/src/features/configuration/components/DefaultsTab.test.tsxcrates/mesh-llm-ui/src/features/configuration/lib/build-toml.test.tscrates/mesh-llm-ui/src/features/configuration/pages/ConfigurationPage.test.tsxcrates/mesh-llm/src/lib.rscrates/skippy-bench/src/cli.rscrates/skippy-bench/src/evals.rscrates/skippy-bench/src/evals/adapters/speed_bench.rscrates/skippy-bench/src/evals/run.rscrates/skippy-bench/src/telemetry_report.rscrates/skippy-coordinator/src/topology.rscrates/skippy-coordinator/src/topology/locked.rscrates/skippy-ffi/README.mdcrates/skippy-ffi/src/lib.rscrates/skippy-metrics/src/lib.rscrates/skippy-model-package/src/package.rscrates/skippy-model-package/src/preflight.rscrates/skippy-prompt/src/prompt_cli/args.rscrates/skippy-prompt/src/prompt_cli/binary_repl.rscrates/skippy-prompt/src/prompt_cli/draft.rscrates/skippy-prompt/src/prompt_cli/generation.rscrates/skippy-prompt/src/prompt_cli/launch.rscrates/skippy-prompt/src/prompt_cli/mod.rscrates/skippy-prompt/src/prompt_cli/speculative.rscrates/skippy-prompt/src/prompt_cli/tests.rscrates/skippy-prompt/src/prompt_cli/topology.rscrates/skippy-prompt/src/prompt_cli/wire_messages.rscrates/skippy-protocol/src/binary/codec.rscrates/skippy-protocol/src/binary/mod.rscrates/skippy-protocol/src/binary/types.rscrates/skippy-runtime/src/lib.rscrates/skippy-runtime/src/ngram.rscrates/skippy-runtime/src/package.rscrates/skippy-runtime/src/session.rscrates/skippy-server/README.mdcrates/skippy-server/src/binary_transport.rscrates/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/telemetry.rscrates/skippy-server/src/binary_transport/decode_batcher.rscrates/skippy-server/src/binary_transport/direct_return.rscrates/skippy-server/src/binary_transport/kv_eviction.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/restore_prefill_decode.rscrates/skippy-server/src/binary_transport/stage_execution.rscrates/skippy-server/src/binary_transport/wire.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend/backend.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/persistent_lanes.rscrates/skippy-server/src/frontend/generation/server.rscrates/skippy-server/src/frontend/generation/types.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/native_mtp/decode.rscrates/skippy-server/src/frontend/native_mtp/hybrid.rscrates/skippy-server/src/frontend/native_mtp/mod.rscrates/skippy-server/src/frontend/native_mtp/pipeline.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rscrates/skippy-server/src/frontend/prefix_cache.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/speculative/standalone.rscrates/skippy-server/src/frontend/tests/prefill.rscrates/skippy-server/src/frontend/tests/prompting.rscrates/skippy-server/src/frontend/wire_messages.rscrates/skippy-server/src/kv_integration/config.rscrates/skippy-server/src/kv_integration/mod.rscrates/skippy-server/src/kv_integration/resident_prefix.rscrates/skippy-server/src/runtime_state.rsdocs/CLI.mddocs/SKIPPY_SPLITS.mddocs/USAGE.mddocs/design/TESTING.mddocs/plugins/telemetry.mddocs/skippy/CONFIGURATION.mddocs/skippy/PIPELINED_VERIFY_WINDOW.mddocs/skippy/WAN_SPLIT_PERF.mddocs/skippy/speculative_decoding.mddocs/specs/layer-package-repos.mddocs/specs/speculative-decoding-wiring-plan.mdscripts/family-certify.shthird_party/llama.cpp/patches/0017-Expose-upstream-ngram-simple-draft-ABI.patchthird_party/llama.cpp/patches/0018-Expose-stateful-N-gram-cache-ABI.patchthird_party/llama.cpp/patches/0019-Remove-legacy-session-checkpoint-ABI.patchthird_party/llama.cpp/patches/0020-Re-prime-native-MTP-after-state-restoration.patchthird_party/llama.cpp/patches/0021-Fix-N-gram-confidence-threshold-indexing.patchwebsite/src/docs/pages/CLI.mdwebsite/src/docs/pages/config-defaults.md
💤 Files with no reviewable changes (17)
- docs/specs/speculative-decoding-wiring-plan.md
- crates/skippy-model-package/src/package.rs
- crates/skippy-prompt/src/prompt_cli/topology.rs
- crates/mesh-llm-host-runtime/tests/fixtures/schema_driven_controls_valid.toml
- docs/skippy/speculative_decoding.md
- crates/mesh-llm-host-runtime/tests/fixtures/config_schema_defaults_ui_reference.json
- crates/skippy-prompt/src/prompt_cli/draft.rs
- crates/skippy-server/src/frontend/backend.rs
- third_party/llama.cpp/patches/0017-Expose-upstream-ngram-simple-draft-ABI.patch
- crates/skippy-runtime/src/package.rs
- crates/skippy-prompt/src/prompt_cli/launch.rs
- crates/skippy-prompt/src/prompt_cli/binary_repl.rs
- crates/skippy-server/src/frontend/wire_messages.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rs
- crates/skippy-server/src/binary_transport/binary_messaging/telemetry.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/tests.rs
- crates/skippy-prompt/src/prompt_cli/args.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/specs/layer-package-repos.md
- docs/USAGE.md
| RuntimeSurface, SkillAgentArg, SkillCommand, TrustCommand, TrustPolicy, | ||
| legacy_runtime_surface_warning, normalize_runtime_surface_args, validate_discovery_mode_args, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== crates/mesh-llm-cli/src/lib.rs ==\n'
sed -n '1,120p' crates/mesh-llm-cli/src/lib.rs
printf '\n== Search for SpeculativeNgramProposerCli ==\n'
rg -n "SpeculativeNgramProposerCli" crates/mesh-llm-cli -S
printf '\n== Search for crate-root imports of mesh_llm_cli::SpeculativeNgramProposerCli ==\n'
rg -n "mesh_llm_cli::SpeculativeNgramProposerCli|use\s+mesh_llm_cli::\{[^}]*SpeculativeNgramProposerCli|use\s+mesh_llm_cli::SpeculativeNgramProposerCli" -S .Repository: Mesh-LLM/mesh-llm
Length of output: 1253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' crates/mesh-llm-cli/src/lib.rs
printf '\n---\n'
rg -n "SpeculativeNgramProposerCli" crates/mesh-llm-cli -S
printf '\n---\n'
rg -n "mesh_llm_cli::SpeculativeNgramProposerCli|use\s+mesh_llm_cli::\{[^}]*SpeculativeNgramProposerCli|use\s+mesh_llm_cli::SpeculativeNgramProposerCli" -S .Repository: Mesh-LLM/mesh-llm
Length of output: 1097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/mesh-llm-cli/src/lib.rs')
print(p.read_text().splitlines()[:40])
PYRepository: Mesh-LLM/mesh-llm
Length of output: 749
Keep the crate-root re-export or bump the major version for this API break. Removing mesh_llm_cli::SpeculativeNgramProposerCli breaks downstream imports from the crate root.
🤖 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-cli/src/lib.rs` around lines 15 - 16, Restore the crate-root
re-export for SpeculativeNgramProposerCli in the public exports of lib.rs so
downstream mesh_llm_cli::SpeculativeNgramProposerCli imports continue to
compile; alternatively, bump the crate’s major version if the removal is
intentional.
| .and_then(|m| m.flash_attention) | ||
| .unwrap_or(FlashAttentionType::Auto), | ||
| parallel_override, | ||
| split_topology_lock: None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not bypass the configured topology lock for runtime-loaded models.
Line 407 forces unlocked planning even when ctx.options.split_topology_lock is set; local_split.rs:307-333 consequently takes the non-locked planner. Forward the configured lock (or explicitly reject runtime loads under a lock) so a lock remains fail-closed.
Proposed fix
- split_topology_lock: None,
+ split_topology_lock: ctx.options.split_topology_lock.as_deref(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| split_topology_lock: None, | |
| split_topology_lock: ctx.options.split_topology_lock.as_deref(), |
🤖 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/model_lifecycle.rs` at line 407,
Update the runtime-loaded model lifecycle path where split_topology_lock is set
to None so it forwards ctx.options.split_topology_lock to the planner. Preserve
fail-closed behavior when a topology lock is configured, ensuring local_split.rs
selects the locked planner; alternatively, explicitly reject runtime loads under
a configured lock.
| node.set_stage_control_sender(skippy::spawn_stage_control_loop( | ||
| Some(Arc::new(node.clone())), | ||
| skippy_telemetry_options(options), | ||
| )) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Extract responsibilities from the modified oversized Rust modules. Both files exceed 1,000 lines after modification and contain separable orchestration responsibilities.
crates/mesh-llm-host-runtime/src/runtime/run_auto.rs#L601-L604: move node/plugin startup orchestration into a semanticrun_autosubmodule.crates/mesh-llm-host-runtime/src/runtime/serving_surface.rs#L1128-L1128: move additional-model startup orchestration into a semantic module.
As per coding guidelines, when modifying a Rust file already over 1,000 lines, extract any separable responsibility into a named module and keep the new file under 1,000 lines.
📍 Affects 2 files
crates/mesh-llm-host-runtime/src/runtime/run_auto.rs#L601-L604(this comment)crates/mesh-llm-host-runtime/src/runtime/serving_surface.rs#L1128-L1128
🤖 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/run_auto.rs` around lines 601 - 604,
Extract the node/plugin startup orchestration around run_auto in
crates/mesh-llm-host-runtime/src/runtime/run_auto.rs (anchor lines 601-604) into
a semantic run_auto submodule, keeping the new module under 1,000 lines and
preserving behavior. Also extract the additional-model startup orchestration
around crates/mesh-llm-host-runtime/src/runtime/serving_surface.rs line 1128
into a semantic module, likewise keeping it under 1,000 lines; both sites
require refactoring.
Source: Coding guidelines
| { | ||
| "node": "micstudio.local", | ||
| "layer_start": 0, | ||
| "layer_end": 31 | ||
| }, | ||
| { | ||
| "node": "studio54-3.local", | ||
| "layer_start": 31, | ||
| "layer_end": 47 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove private hostnames from both tracked examples.
The topology-lock examples use machine-specific .local hostnames instead of neutral placeholders, disclosing private connection details in repository documentation.
docs/SKIPPY_SPLITS.md#L94-L102: replacemicstudio.localandstudio54-3.localwith placeholders such as<node-a>and<node-b>.website/src/docs/pages/CLI.md#L225-L234: make the same replacement and explain that users must provide their own selectors.
As per coding guidelines, **/*: Never commit credentials or private machine connection details to tracked files; keep them outside the repository.
📍 Affects 2 files
docs/SKIPPY_SPLITS.md#L94-L102(this comment)website/src/docs/pages/CLI.md#L225-L234
🤖 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 `@docs/SKIPPY_SPLITS.md` around lines 94 - 102, Replace the private .local
hostnames in the topology-lock example at docs/SKIPPY_SPLITS.md lines 94-102
with neutral placeholders such as <node-a> and <node-b>. Make the same
replacement in website/src/docs/pages/CLI.md lines 225-234 and explain there
that users must provide their own node selectors.
Source: Coding guidelines
| - [x] Make speculative positions authoritative in stage-state v9. | ||
| - Decode and `VerifyWindow` messages carry the absolute position each stage | ||
| must have before execution. | ||
| - Stages ahead of that position rewind attention KV locally. | ||
| - Speculation never checkpoints, restores, trims by control message, or | ||
| replays a rejected prefix. | ||
| - Recurrent-state stages reject positional speculation instead of falling | ||
| back to the removed checkpoint protocol. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the stage-state generation.
This section says v9, but the PR’s serving-pipeline contract is stage-state v10. Labeling the authoritative-position semantics as v9 makes the protocol rollout and compatibility guidance ambiguous.
🤖 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 `@SKIPPY_PROTOCOL_TODO.md` around lines 158 - 165, Update the completed “Make
speculative positions authoritative in stage-state” checklist entry in
SKIPPY_PROTOCOL_TODO.md from stage-state v9 to v10, preserving the existing
authoritative-position semantics and compatibility guidance.
Retargets the suffix N-gram proposer PR onto main, which now contains Mesh-LLM#1026's positional-MTP n-gram rework (squash-merged). Builds on the earlier reconcile of Mesh-LLM#1026; this merge folds in main's other changes. - Keep the standalone-suffix reconciliation at every conflict (validate() allows a request-local ngram proposer without native MTP; resolver emits "ngram" mode; decode path unified on HistoryNgramProposer; simple proposer stays dropped since its skippy-ffi backing was removed upstream). - Take main's non-suffix additions where they don't overlap: skippy-ffi dynamic_library module, the expanded preflight suite, and the rewritten layer-package-repos spec. - Re-apply fixes the line-merge silently dropped where main touched the same regions: NgramProposerKind re-export (frontend.rs, lib.rs), the cache-only gating of the preflight ngram_max<=4 / history_scope checks, and the ngram_proposer path in the defaults UI schema fixture. - Scrub stale ngram-simple references from the docs. Workspace builds clean; lib tests green across skippy-server, mesh-llm-config, mesh-llm-cli, mesh-llm-host-runtime, and skippy-model-package.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/USAGE.md (1)
754-763: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove unsupported confidence and scaling claims.
The proposer contract supports exact matching with an independent
max_proposal_tokenscap; it does not establish “high-confidence” drafts or that draft length scales with match length. This also conflicts with the preceding statement that match length and continuation length are separate controls. Reword this to describe potential long drafts without implying guaranteed quality or sizing behavior.🤖 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 `@docs/USAGE.md` around lines 754 - 763, Revise the `suffix` proposer documentation to remove claims that drafts are “high-confidence” or that draft length scales with match length. Describe only that exact long suffix matches may produce long drafts, subject to the independent `max_proposal_tokens` cap, while preserving the documented `ngram_min` and `ngram_max` match controls.
🤖 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 `@docs/USAGE.md`:
- Around line 749-752: Update the N-gram settings documentation near the
request-local cache description to qualify the output-correctness statement:
describe target verification as the intended safeguard, and avoid claiming that
tuning these values is proven not to affect correctness until exact-greedy
equivalence is validated.
---
Outside diff comments:
In `@docs/USAGE.md`:
- Around line 754-763: Revise the `suffix` proposer documentation to remove
claims that drafts are “high-confidence” or that draft length scales with match
length. Describe only that exact long suffix matches may produce long drafts,
subject to the independent `max_proposal_tokens` cap, while preserving the
documented `ngram_min` and `ngram_max` match controls.
🪄 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: 647a354c-ba0c-4a95-b72a-4a40ef5a3ff8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/skippy-model-package/src/preflight.rsdocs/USAGE.mddocs/skippy/CONFIGURATION.mddocs/skippy/SUFFIX_NGRAM_PROPOSER.mddocs/specs/layer-package-repos.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/skippy/SUFFIX_NGRAM_PROPOSER.md
- crates/skippy-model-package/src/preflight.rs
| The request-local cache is limited to `ngram_max <= 4`. N-gram settings may run | ||
| standalone or, with native MTP, form one composite proposal. All combinations | ||
| are verified together by the target, so tuning these values changes speculative | ||
| work, not output correctness. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Qualify the output-correctness guarantee.
This states that tuning N-gram settings cannot affect output correctness, but the PR objectives explicitly say exact-greedy equivalence has not been proven. Document target verification as the intended safeguard, not as an established guarantee, until equivalence is validated.
🤖 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 `@docs/USAGE.md` around lines 749 - 752, Update the N-gram settings
documentation near the request-local cache description to qualify the
output-correctness statement: describe target verification as the intended
safeguard, and avoid claiming that tuning these values is proven not to affect
correctness until exact-greedy equivalence is validated.
|
Minor: a couple of stale doc comments still reference
|
i386
left a comment
i386
left a comment
There was a problem hiding this comment.
Requesting changes for the following issues:
-
P1 — Standalone N-gram is silently ignored for single-stage/direct serving.
speculative_mode_for_embeddedenables N-gram regardless ofstaged, but a stage withoutdownstreamimmediately delegates togenerate_local_tokens, which has no N-gram path. Direct GGUF and one-stage package requests therefore run target-only despite reportingngram-cache/ngram-suffix. Please implement local verification or reject this configuration outside multi-stage serving. -
P1 —
strategy = "disabled"can be overridden by inherited N-gram settings.resolve_decode_configbuilds an N-gram plan from defaults without checking the requested strategy, after which the mode override re-enables speculation. A model-level disable must clear or ignore inherited proposer state. This corroborates the existing unresolved inline thread. -
P2 — Suffix lookup remains unbounded on ambiguous repetitive input. Every indexed occurrence is examined unless one reaches the full
max_window. Repeated seeds whose preceding tokens differ scan the entire bucket on every decode step, yieldingO(generated_tokens × candidate_occurrences × max_window)work for request-controlled prompts. Please add a hard candidate budget or another bounded index strategy. -
P2 — Existing speculative-plan JSON is no longer readable.
NgramProposalConfig.kindis newly required, while plans produced by previous releases contain onlymin_ngram,max_ngram, andmax_proposal_tokens.--openai-speculative-configwill fail deserialization. A serde default ofCachewould preserve prior behavior. -
P2 — Explicit standalone strategies can silently become disabled. With
strategy = "ngram-cache"or"ngram-suffix", no package metadata, and either bound omitted, N-gram construction is skipped and resolution succeeds with no proposer. An explicitly requested strategy should fail with the documented “both bounds required” error. -
P2 — Two copy-paste configuration examples contain rejected keys.
extension_initial_tokensandextension_tail_backoff_proposalsappear indocs/skippy/SUFFIX_NGRAM_PROPOSER.mdanddocs/USAGE.md, but do not exist inSpeculativeConfigRaw, which denies unknown fields.
Reviewed head 165db9320e170a1334c376f6857cb8daa85a324f.
P1 fixes: - Reject standalone N-gram on single-stage/direct serving. The no-downstream path (generate_local_tokens) has no N-gram verification, so ensure_embedded_ openai_safe now errors for a standalone proposer when !staged instead of silently running target-only; speculative_mode_for_embedded is also gated on staged. - strategy = "disabled" no longer inherits proposer state. resolve_decode_config short-circuits for a disabled request, clearing ngram/extension/native-MTP so the mode override cannot re-enable speculation from [defaults] or package metadata. P2 fixes: - Explicit ngram-cache / ngram-suffix strategies now force proposer construction, so omitting a bound fails with the both-bounds-required error instead of resolving to no proposer. - NgramProposalConfig.kind gets a serde default of Cache, so speculative plans written before the field existed still deserialize (--openai-speculative-config). - Bound the suffix candidate scan at 64 occurrences per lookup (most-recent-first) so ambiguous repetitive input can't scan the whole bucket every decode step. - Remove the rejected extension_initial_tokens / extension_tail_backoff_proposals keys from the USAGE and suffix-proposer doc examples. Adds tests for each: single-stage rejection, disabled-clears-inherited-ngram, explicit-without-bounds error, legacy-JSON kind default, and the suffix scan cap.
* origin/main: Add Ngram Suffix Proposer (#1037)
5 participants
suffixN-gram draft proposer (prompt-lookup decoding)Summary
Adds a third N-gram draft proposer,
suffix: a pure-Rust longest-suffix matcher (prompt-lookup decoding).simpleandcacheare bound by llama.cpp's 4-token match window.suffixis not, so it matches verbatim spans up to 64 tokens and copies them as the speculative draft. Additive and default-off.Motivation
The existing
simple/cacheproposers wrap llama.cpp's N-gram proposer, capped at a 4-token match window (NGRAM_CACHE_MAX_NGRAM). In long agent transcripts a 4-token match is ambiguous: it occurs in many places with different continuations, so drafts stay short and get rejected.Agent-coding workloads are dominated by re-emission: read a file, write it back with a small change; echo tool output; repeat identifiers. The output is largely a copy of text already in context. A 16 to 32 token suffix match is usually unique, so it can copy whole spans the model would otherwise decode one token at a time.
What's in the PR
SuffixNgramProposer(speculative/suffix.rs): indexes committed history by an exact seed key (no hash collisions), finds the longest verbatim earlier occurrence of the query suffix (up toSUFFIX_NGRAM_MAX_WINDOW = 64), and copies what followed. Draft length scales with match length. Stays silent belowngram_min.speculative/standalone.rs): Cache and Suffix now work as standalone (non-MTP) proposers, not just Simple. A plainngram_proposer = "suffix"config activates it. No MTP model required.prefix_cache.enabled = falsenow survives model-family defaults, and standalone timing fields are populated. Neither changes the lookup algorithm.ngram_proposer = "suffix"(mesh config and CLI). ReusesNgramProposalConfig(min_ngram/max_ngram/max_proposal_tokens).docs/skippy/SUFFIX_NGRAM_PROPOSER.md, USAGE.md, CONFIGURATION.md.Benchmarks
Standalone N-gram matrix on
GLM-4.7-Flash-MTP-GGUF:Q4_K_M, two-stage split, deterministic re-emit-a-file prompt (rename one function, generate 384 tokens). Medians of 5 measured requests after 2 warmups. Prefix caching disabled so a cross-request restore cannot skew the comparison.Suffix was the fastest arm: +427% over target-only, +11% over Simple. It had the highest acceptance while proposing the fewest tokens. Lookup cost was about 12.6 microseconds per request.
Simple beat Cache on throughput despite lower acceptance, because it proposed more tokens in absolute terms. Acceptance percentage alone does not rank a proposer.
Reproduced on a local 8B two-stage split (base M1 Pro): about 3.6x over baseline on a realistic file re-emit, neutral on freeform chat.
Not in scope / follow-ups
chain_restore_hit. Disabling the cache made sequential requests reliable. Speculative checkpoint state appears to interact with cross-request KV restore. This should be fixed before a cache-enabled rerun.Testing
Config example
toml [models.speculative] strategy = "auto" ngram_proposer = "suffix" ngram_min = 5 ngram_max = 32 ngram_max_proposal_tokens = 48 Summary by CodeRabbit
New Features
ngram-suffixspeculative decoding for standalone and native-MTP flows (asngram-suffix/native-mtp+ngram-suffix).--split-topology-lockoption.Bug Fixes
Documentation
Telemetry