Pipeline MTP-anchored n-gram verify windows - #938
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
📝 WalkthroughWalkthroughChangesThis PR adds typed speculative-decoding configuration and CLI overrides, N-gram simple/cache proposers, package proposer schemas, VerifyWindow protocol messages, pipelined native MTP execution, response telemetry, benchmark timing capture, and runtime resilience updates. It also renames local verification tooling from VerifySpan to VerifyWindow. Speculative configuration and package resolution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant RuntimeResolver
participant StageOpenAiBackend
participant VerifyWindowScheduler
participant BinaryTransport
participant NativeMtpVerifier
CLI->>RuntimeResolver: provide speculative overrides
RuntimeResolver->>StageOpenAiBackend: resolve typed speculative plan
StageOpenAiBackend->>VerifyWindowScheduler: open pipeline window
StageOpenAiBackend->>BinaryTransport: send VerifyWindow
BinaryTransport->>NativeMtpVerifier: verify proposal
NativeMtpVerifier-->>BinaryTransport: typed draft and reply window
BinaryTransport-->>StageOpenAiBackend: return verification reply
StageOpenAiBackend->>VerifyWindowScheduler: complete window in FIFO order
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
Very nice |
…elined-decode # Conflicts: # crates/skippy-server/src/frontend/embedded_generation.rs
i386
changed the title
i386
changed the title
…de' into experiment/skippy-pipelined-decode
|
Added the two requested standalone proposer rows. These are three-repetition SPEED-Bench long-coding results on the same
Standalone simple N-gram beats no speculation by 4.79%. Cached N-gram is functioning, but is only 1.74% over no speculation and trails simple N-gram on this workload. Both hybrid variants beat native MTP against their direct parent; MTP + simple remains the winner. For the two standalone rows, Draft/Accepted comes from correlated Artifacts and complete reproduction identity: https://github.com/Mesh-LLM/lab-experiments/blob/6ee91ad/skippy-pipelined-decode/phase-6/20260717T043625Z/RESULTS.md |
|
Single-node SPEED-Bench parity is complete for GLM 4.7 Flash Q4_K_M on studio54 (Metal, ctx 4096, one slot, three repetitions per row).
This confirms a small base serving-path deficit and a severe Mesh native-MTP hot-path regression. Direct llama MTP improves 15.78% over its baseline; Mesh MTP regresses 39.48% from its baseline. Mesh simple N-gram does beat direct llama simple N-gram by 4.73%. Telemetry also has two remaining diagnostic gaps: pure N-gram/extender proposal and acceptance counts are unavailable, and the Mesh response-level MTP draft denominator reports 29.07% while native verification telemetry reports 87.29%. Actually unpatched llama.cpp at the exact base pin cannot load this GGUF (expected 868 tensors, got 862). The direct control is canonical llama-server from the same patched llama checkout, which holds model support and inference core constant while removing the Mesh/Skippy serving path. Full configs, repetitions, compact telemetry, and interpretation: https://github.com/Mesh-LLM/lab-experiments/tree/2208edf/skippy-pipelined-decode/phase-6/20260717T143804Z |
Bring in #1011 (resilient to memory pressure & network jitter) which adds bounded timeouts on the prediction-return and lane-open paths. Resolved conflicts in binary_transport/direct_return.rs and frontend.rs by taking main's bounded single-attempt connection logic while keeping the branch's speculative decode code. Removed now-unused retry helpers/constants.
WAN split validation: Sydney ↔ Melbourne (2-node), real GPURan this branch as a genuine 2-node split over the public internet: M5 Max (Sydney, Metal, stage-0) ↔ RTX A5000 (Melbourne vast.ai, CUDA, stage-1), ~20 ms RTT, direct iroh hole-punch. Both nodes built from the same commit. Model Headline: it works well over a real WAN link 🎉Once Latency contribution (the interesting bit)
Split overhead 44.9 ms/token ≈ 2×RTT — latency (the per-token round-trips) is essentially the entire cost of splitting here, not compute. That's precisely why prediction matters on WAN. Speculative A/B (RTT ~20 ms)
+13–19% over MTP-off, and the gain grows on longer/coding output — speculation commits multiple tokens per round-trip, hiding the 44.9 ms latency tax. Stability / recovery (killed the remote stage mid-generation)
Note on
|
…ition Adds docs/skippy/WAN_SPLIT_PERF.md: the single-stream per-token cost model (TPOT ~= C_total + (S-1)*2*RTT + (S-1)*P), compute-bound vs latency-bound criteria, when adding a stage helps (memory, concurrency/pipeline overlap, dense compute-bound models), and speculation as the WAN amortization lever. Backed by 2026-07-18 Sydney<->Melbourne 2-node measurements: solo 12.9 ms/tok compute, split 57.8 ms/tok, decomposing to 12.9 compute + 40 (2xRTT) + 4.9 protocol. Workload was latency-bound (~78% network).
Documents the measured ~30s hang when a new request routes to a killed split stage, the confirmed root cause (60s heartbeat / lenient failure threshold + slow lane-open timeouts), and a two-layer fix (short steady-state lane-open deadline; feed lane failures into target_health cooldown) plus an explicit validation gate. Mesh-timing changes are out of scope pending live multi-node validation.
|
This is starting to work well |
…stage When a downstream split stage dies, a new request would open a fresh lane and wait the full ~20s warmup ready-deadline before erroring (observed as a ~30s hang in the Sydney<->Melbourne kill test). The 20s deadline is only needed during pool warmup, when the downstream may still be loading its model. Split the deadline: pool warmup keeps LANE_READY_READ_TIMEOUT (20s); mid-life reconnects from checkout()/replace_lane() on an already-serving mesh use a new LANE_STEADY_CONNECT_TIMEOUT (3s). A healthy peer answers in milliseconds, so a dead stage now fails in ~3s instead of ~30s. Restores receive_persistent_lane_ready as the shared bounded-handshake helper (dropped during the main merge) and removes a now-obsolete retry test that covered pre-#1011 retry behavior. Adds tests asserting the steady-state deadline stays well under the warmup deadline and that the handshake read fails fast on a silent downstream.
Records verified planner behaviour (skippy-coordinator/topology.rs, skippy-topology, host-runtime call site): - latency is a placement cost (rtt_ms penalty), not just relay-only exclusion - planner selects a node subset; does not have to use every eligible node - stage count is gated on a decode-TPOT target (shallower-that-meets beats deeper-that-does-not) And the gaps that matter at many-node scale: - no peer-to-peer RTT matrix in production (edge_signals never wired; only coordinator-RTT is used) -> co-located nodes cannot be exploited - network estimate is max(coordinator RTT) x node_count, a worst-case proxy - no first-class prefer-fewer/never-place-above-Y policy beyond the TPOT gate
…latency-bound split)
A pooled downstream lane whose stage died while checked in was a dead TCP stream; reusing it blocked the next generation read forever (handshake read-timeout is cleared for pooled lanes so long generations don't truncate). checkout() now probes lane liveness with a nonblocking peek and discards a dead lane so it reconnects with the short steady-state deadline instead of hanging. Validated on a loopback 2-node split with a mid-flight worker kill: new request now fails faster than main (60s vs main's 90s baseline). Does not fully solve the recovered-local routing path, tracked as follow-up.
…elined-decode # Conflicts: # crates/mesh-llm-host-runtime/src/mesh/mod.rs # crates/mesh-llm-host-runtime/src/mesh/tests.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
crates/skippy-server/src/lib.rs (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider importing these from
skippy_server::frontendinstead of expanding crate-root re-exports.
frontendis a public module, so consumers likeresolver/speculative.rscanuse skippy_server::frontend::{SpeculativeDecodeConfig, ...}directly rather than growing the root re-export surface.As per coding guidelines: "Minimize crate-root re-exports. Root re-exports are acceptable as temporary compatibility shims during refactors, but new code should prefer importing from the owning module 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/skippy-server/src/lib.rs` around lines 30 - 33, Update the imports in the affected consumer to use the public skippy_server::frontend module for SpeculativeDecodeConfig and the other frontend-owned symbols, instead of adding or relying on crate-root re-exports. Keep only symbols that genuinely belong at the crate root and preserve existing behavior.Source: Coding guidelines
crates/skippy-server/src/frontend/native_mtp/verify_window.rs (1)
320-465: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
execute_native_mtp_verify_windowinto helpers
execute_native_mtp_verify_windowis 444 lines. Extracting only the telemetry block still leaves it well above the 200-line Clippy limit, so this needs a broader split—at least the debug-telemetry section plus another helper for the main proposal/verify state updates.🤖 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 320 - 465, Split execute_native_mtp_verify_window into multiple focused helpers to bring the function below Clippy’s 200-line limit. Extract the shown telemetry construction and emission into a dedicated helper, and move the main proposal/verification state-update logic into another helper while preserving existing behavior and data flow.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/skippy-bench/src/evals/adapters/speed_bench.rs`:
- Around line 35-60: Update the JSONL write in capture_response_timings so the
generated Python script writes an actual newline after each serialized timing
record, rather than the literal backslash-n text. Preserve the existing JSON
serialization and locking behavior.
In `@crates/skippy-server/src/frontend/native_mtp/decode.rs`:
- Around line 225-241: Update observe_adaptive_verify_window so
adaptive_verify_window_width_min uses the already-incremented
adaptive_verify_window_count to detect the first observation, rather than
checking whether the stored minimum is zero. Preserve zero as a legitimate
minimum and apply min(existing, width) for every subsequent observation.
In `@crates/skippy-server/src/frontend/native_mtp/pipeline.rs`:
- Around line 58-80: Update next_window so dispatched_native_mtp_token_count
also advances for the expected_free_target removed by candidates.pop_front()
when that token belongs to the native proposal prefix. Preserve the existing
drained proposal counting and ensure the next window’s native_mtp_token_count
reflects both removed portions.
In `@docs/skippy/PIPELINED_VERIFY_WINDOW.md`:
- Around line 287-306: Update the three command examples under “No MTP
Baseline,” “Native MTP Only,” and “MTP With Cache-backed N-gram Extension” so
each uses the matching --speculative-strategy value: disabled, mtp, and
mtp-cache respectively, while preserving the existing command options.
In `@scripts/build-release.sh`:
- Around line 141-145: Separate the LLAMA_STAGE_BUILD_DIR declaration from its
command-substitution assignment in the release build flow. Keep the existing
build-llama.sh invocation and environment variables unchanged, then export
LLAMA_STAGE_BUILD_DIR afterward so its exit status propagates and set -e can
detect failures.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/native_mtp/verify_window.rs`:
- Around line 320-465: Split execute_native_mtp_verify_window into multiple
focused helpers to bring the function below Clippy’s 200-line limit. Extract the
shown telemetry construction and emission into a dedicated helper, and move the
main proposal/verification state-update logic into another helper while
preserving existing behavior and data flow.
In `@crates/skippy-server/src/lib.rs`:
- Around line 30-33: Update the imports in the affected consumer to use the
public skippy_server::frontend module for SpeculativeDecodeConfig and the other
frontend-owned symbols, instead of adding or relying on crate-root re-exports.
Keep only symbols that genuinely belong at the crate root and preserve existing
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f5beaea-dc81-448e-99fc-19b1af451b08
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (92)
crates/mesh-llm-cli/src/lib.rscrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-config/src/lib.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/validate.rscrates/mesh-llm-host-runtime/Cargo.tomlcrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/package.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/translation.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/types.rscrates/mesh-llm-host-runtime/src/mesh/stage_transport.rscrates/mesh-llm-host-runtime/src/mesh/tests/requirements.rscrates/mesh-llm-host-runtime/src/plugin/mod.rscrates/mesh-llm-host-runtime/src/runtime/mod.rscrates/mesh-llm-host-runtime/src/runtime/options.rscrates/mesh-llm-host-runtime/tests/fixtures/config_schema_defaults_ui_reference.jsoncrates/mesh-llm/src/lib.rscrates/openai-frontend/src/completions.rscrates/skippy-bench/src/chat_corpus.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/main.rscrates/skippy-bench/src/verify_window_local.rscrates/skippy-correctness/src/runner/native_mtp.rscrates/skippy-ffi/README.mdcrates/skippy-ffi/build.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/preflight.rscrates/skippy-prompt/src/prompt_cli/generation.rscrates/skippy-prompt/src/prompt_cli/speculative.rscrates/skippy-prompt/src/prompt_cli/tests.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-server/README.mdcrates/skippy-server/src/binary_transport.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/tests.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend.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/generation_flow.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/native_mtp/batched_verify.rscrates/skippy-server/src/frontend/native_mtp/decode.rscrates/skippy-server/src/frontend/native_mtp/draft.rscrates/skippy-server/src/frontend/native_mtp/env.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/trim.rscrates/skippy-server/src/frontend/native_mtp/verifier.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/tests.rscrates/skippy-server/src/frontend/wire_messages.rscrates/skippy-server/src/lib.rsdocs/README.mddocs/USAGE.mddocs/skippy/CONFIGURATION.mddocs/skippy/DEAD_PEER_FAST_FAIL_PLAN.mddocs/skippy/PIPELINED_VERIFY_WINDOW.mddocs/skippy/TOPOLOGY_PLANNER.mddocs/skippy/WAN_SPLIT_PERF.mddocs/skippy/family/qwen-results.mddocs/skippy/speculative_decoding.mddocs/specs/layer-package-repos.mdevals/skippy-coding-agent-loop.jsonlscripts/build-mac.shscripts/build-release.shscripts/tests/test_build_release.pythird_party/llama.cpp/patches/0017-Expose-upstream-ngram-simple-draft-ABI.patchthird_party/llama.cpp/patches/0018-Expose-stateful-N-gram-cache-ABI.patchwebsite/src/docs/pages/CLI.md
💤 Files with no reviewable changes (3)
- crates/skippy-server/src/frontend/native_mtp/trim.rs
- crates/skippy-server/src/frontend/native_mtp/env.rs
- crates/skippy-server/src/frontend/native_mtp/batched_verify.rs
|
|
||
| timings_path = os.environ.get("SKIPPY_BENCH_RESPONSE_TIMINGS_PATH") | ||
| timings_lock = threading.Lock() | ||
| original_json = requests.models.Response.json | ||
|
|
||
| def capture_response_timings(self, *args, **kwargs): | ||
| response = original_json(self, *args, **kwargs) | ||
| timings = response.get("timings") if isinstance(response, dict) else None | ||
| if ( | ||
| timings_path | ||
| and isinstance(timings, dict) | ||
| and not getattr(self, "_skippy_timings_captured", False) | ||
| ): | ||
| # Preserve only scalar timing counters; never copy request or response content. | ||
| safe_timings = { | ||
| key: value | ||
| for key, value in timings.items() | ||
| if isinstance(key, str) and isinstance(value, (bool, int, float)) | ||
| } | ||
| with timings_lock: | ||
| with open(timings_path, "a", encoding="utf-8") as output: | ||
| output.write(json.dumps({"timings": safe_timings}, sort_keys=True) + "\\n") | ||
| self._skippy_timings_captured = True | ||
| return response | ||
|
|
||
| requests.models.Response.json = capture_response_timings |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python -c 'import json; print(json.dumps({"test": 1}) + "\\n" + json.dumps({"test": 2}))'Repository: Mesh-LLM/mesh-llm
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/skippy-bench/src/evals/adapters/speed_bench.rs"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant lines =="
sed -n '1,120p' "$file" | cat -nRepository: Mesh-LLM/mesh-llm
Length of output: 5588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SKIPPY_BENCH_RESPONSE_TIMINGS_PATH|response timings|timings" crates/skippy-bench -SRepository: Mesh-LLM/mesh-llm
Length of output: 4721
Write an actual newline in the JSONL output
crates/skippy-bench/src/evals/adapters/speed_bench.rs:35-60
\\n is emitted literally into the Python script here, so the timings file gets \n text instead of a line break and stops being valid JSONL. Replace it with \n.
🤖 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/evals/adapters/speed_bench.rs` around lines 35 - 60,
Update the JSONL write in capture_response_timings so the generated Python
script writes an actual newline after each serialized timing record, rather than
the literal backslash-n text. Preserve the existing JSON serialization and
locking behavior.
| pub(in crate::frontend) fn observe_adaptive_verify_window( | ||
| &mut self, | ||
| width: usize, | ||
| previous_width: usize, | ||
| next_width: usize, | ||
| ) { | ||
| self.adaptive_verify_window_count += 1; | ||
| self.adaptive_verify_window_width_sum += width; | ||
| self.adaptive_verify_window_width_min = if self.adaptive_verify_window_width_min == 0 { | ||
| width | ||
| } else { | ||
| self.adaptive_verify_window_width_min.min(width) | ||
| }; | ||
| self.adaptive_verify_window_width_max = self.adaptive_verify_window_width_max.max(width); | ||
| self.adaptive_verify_window_grow_count += usize::from(next_width > previous_width); | ||
| self.adaptive_verify_window_shrink_count += usize::from(next_width < previous_width); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sentinel-zero conflates "unset" with a legitimate zero width.
adaptive_verify_window_width_min uses 0 both as the "not yet observed" sentinel and as a real width value. If a genuine width == 0 observation occurs first (e.g. AdaptiveVerifyWindow::width returns 0 when available_tokens == 0 near the generation limit), any subsequent non-zero width will incorrectly overwrite the true minimum instead of taking min(existing, new), since the check self.adaptive_verify_window_width_min == 0 can't distinguish "never set" from "legitimately zero."
Use the already-incremented adaptive_verify_window_count to detect the first observation instead of relying on the value itself.
🐛 Proposed fix
self.adaptive_verify_window_count += 1;
self.adaptive_verify_window_width_sum += width;
- self.adaptive_verify_window_width_min = if self.adaptive_verify_window_width_min == 0 {
+ self.adaptive_verify_window_width_min = if self.adaptive_verify_window_count == 1 {
width
} else {
self.adaptive_verify_window_width_min.min(width)
};📝 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.
| pub(in crate::frontend) fn observe_adaptive_verify_window( | |
| &mut self, | |
| width: usize, | |
| previous_width: usize, | |
| next_width: usize, | |
| ) { | |
| self.adaptive_verify_window_count += 1; | |
| self.adaptive_verify_window_width_sum += width; | |
| self.adaptive_verify_window_width_min = if self.adaptive_verify_window_width_min == 0 { | |
| width | |
| } else { | |
| self.adaptive_verify_window_width_min.min(width) | |
| }; | |
| self.adaptive_verify_window_width_max = self.adaptive_verify_window_width_max.max(width); | |
| self.adaptive_verify_window_grow_count += usize::from(next_width > previous_width); | |
| self.adaptive_verify_window_shrink_count += usize::from(next_width < previous_width); | |
| } | |
| pub(in crate::frontend) fn observe_adaptive_verify_window( | |
| &mut self, | |
| width: usize, | |
| previous_width: usize, | |
| next_width: usize, | |
| ) { | |
| self.adaptive_verify_window_count += 1; | |
| self.adaptive_verify_window_width_sum += width; | |
| self.adaptive_verify_window_width_min = if self.adaptive_verify_window_count == 1 { | |
| width | |
| } else { | |
| self.adaptive_verify_window_width_min.min(width) | |
| }; | |
| self.adaptive_verify_window_width_max = self.adaptive_verify_window_width_max.max(width); | |
| self.adaptive_verify_window_grow_count += usize::from(next_width > previous_width); | |
| self.adaptive_verify_window_shrink_count += usize::from(next_width < previous_width); | |
| } |
🤖 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/decode.rs` around lines 225 -
241, Update observe_adaptive_verify_window so adaptive_verify_window_width_min
uses the already-incremented adaptive_verify_window_count to detect the first
observation, rather than checking whether the stored minimum is zero. Preserve
zero as a legitimate minimum and apply min(existing, width) for every subsequent
observation.
| pub(in crate::frontend) fn next_window( | ||
| &mut self, | ||
| verify_width: usize, | ||
| ) -> Option<PipelinedCandidateWindow> { | ||
| let verify_width = verify_width | ||
| .min(self.parallel_verify_width) | ||
| .min(self.candidates.len()); | ||
| if verify_width == 0 { | ||
| return None; | ||
| } | ||
| let native_mtp_token_count = self | ||
| .proposal | ||
| .native_mtp_token_count() | ||
| .saturating_sub(self.dispatched_native_mtp_token_count) | ||
| .min(verify_width); | ||
| let proposal_tokens = self.candidates.drain(..verify_width).collect(); | ||
| self.dispatched_native_mtp_token_count += native_mtp_token_count; | ||
| Some(PipelinedCandidateWindow { | ||
| proposal_tokens, | ||
| expected_free_target: self.candidates.pop_front(), | ||
| native_mtp_token_count, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files crates/skippy-server/src/frontend/native_mtp/pipeline.rs
ast-grep outline crates/skippy-server/src/frontend/native_mtp/pipeline.rs --view expanded
wc -l crates/skippy-server/src/frontend/native_mtp/pipeline.rsRepository: Mesh-LLM/mesh-llm
Length of output: 2355
🏁 Script executed:
sed -n '1,260p' crates/skippy-server/src/frontend/native_mtp/pipeline.rsRepository: Mesh-LLM/mesh-llm
Length of output: 7711
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('crates/skippy-server/src/frontend/native_mtp/pipeline.rs')
print(p.exists(), p.stat().st_size if p.exists() else None)
print(p.read_text().splitlines()[0:220])
PYRepository: Mesh-LLM/mesh-llm
Length of output: 8043
🏁 Script executed:
rg -n "struct NativeMtpHybridProposal|fn native_mtp_token_count|ngram_tail_rejected|native_mtp_prefix_rejected|dispatched_native_mtp_token_count|expected_free_target" crates/skippy-server/src/frontend/native_mtp crates/skippy-server/src/frontend -g '*.rs'Repository: Mesh-LLM/mesh-llm
Length of output: 10890
🏁 Script executed:
sed -n '1,240p' crates/skippy-server/src/frontend/native_mtp/mod.rs
printf '\n---\n'
sed -n '1,260p' crates/skippy-server/src/frontend/native_mtp/proposal.rs
printf '\n---\n'
rg -n "native_mtp_token_count\(|ngram_tail_rejected\(|native_mtp_prefix_rejected\(" crates/skippy-server/src/frontend/native_mtp -g '*.rs'Repository: Mesh-LLM/mesh-llm
Length of output: 5094
🏁 Script executed:
sed -n '1,260p' crates/skippy-server/src/frontend/native_mtp/proposal.rs
printf '\n---\n'
rg -n "native_mtp_token_count\(|ngram_tail_rejected\(|native_mtp_prefix_rejected\(" crates/skippy-server/src/frontend/native_mtp -g '*.rs'Repository: Mesh-LLM/mesh-llm
Length of output: 4339
🏁 Script executed:
rg -n "native_mtp_prefix_rejected|ngram_tail_rejected|native_mtp_token_count" crates/skippy-server/src -g '*.rs'Repository: Mesh-LLM/mesh-llm
Length of output: 9182
🏁 Script executed:
sed -n '129,205p' crates/skippy-server/src/frontend/native_mtp/hybrid.rs
printf '\n---\n'
sed -n '490,520p' crates/skippy-server/src/frontend/native_mtp/verify_window.rs
printf '\n---\n'
sed -n '1030,1160p' crates/skippy-server/src/frontend/embedded_generation.rsRepository: Mesh-LLM/mesh-llm
Length of output: 12033
Count the reserved free target in the native-prefix offset dispatched_native_mtp_token_count only advances by the drained proposal slice, but expected_free_target also removes one candidate. If that token is still native, the next window will compute the wrong native_mtp_token_count and misclassify the tail as native.
🤖 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/pipeline.rs` around lines 58 -
80, Update next_window so dispatched_native_mtp_token_count also advances for
the expected_free_target removed by candidates.pop_front() when that token
belongs to the native proposal prefix. Preserve the existing drained proposal
counting and ensure the next window’s native_mtp_token_count reflects both
removed portions.
| ```bash | ||
| mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft | ||
| ``` | ||
|
|
||
| Use `[models.speculative] strategy = "disabled"` to make this an explicit | ||
| baseline instead of relying on environment variables. | ||
|
|
||
| ### Native MTP Only | ||
|
|
||
| ```bash | ||
| mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft | ||
| ``` | ||
|
|
||
| Use `[models.speculative] strategy = "mtp"` to force this control. | ||
|
|
||
| ### MTP With Cache-backed N-gram Extension | ||
|
|
||
| ```bash | ||
| mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the duplicated bash commands to reflect the respective modes.
The bash code blocks under "No MTP Baseline", "Native MTP Only", and "MTP With Cache-backed N-gram Extension" are identical. Consider adding the appropriate CLI --speculative-strategy flags to each example so they match their headings and serve as better copy-paste snippets.
📝 Proposed fix to differentiate the examples
### No MTP Baseline
```bash
-mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft
+mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft \
+ --speculative-strategy disabledUse [models.speculative] strategy = "disabled" to make this an explicit
baseline instead of relying on environment variables.
Native MTP Only
-mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft
+mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft \
+ --speculative-strategy mtpUse [models.speculative] strategy = "mtp" to force this control.
MTP With Cache-backed N-gram Extension
-mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft
+mesh-llm serve meshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_M --split --no-draft \
+ --speculative-strategy mtp-cache</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🤖 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/PIPELINED_VERIFY_WINDOW.md` around lines 287 - 306, Update the
three command examples under “No MTP Baseline,” “Native MTP Only,” and “MTP With
Cache-backed N-gram Extension” so each uses the matching --speculative-strategy
value: disabled, mtp, and mtp-cache respectively, while preserving the existing
command options.
| export LLAMA_STAGE_BUILD_DIR="$( | ||
| LLAMA_STAGE_BACKEND="$BACKEND" \ | ||
| LLAMA_STAGE_LINK_MODE=static \ | ||
| "$SCRIPT_DIR/build-llama.sh" --print-build-dir | ||
| )" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Separate declaration and assignment to avoid masking the return value.
When export and command substitution are combined on the same line, the exit code of export (which is typically 0) will mask any failure from the build-llama.sh script. This prevents set -e from aborting the build if the script fails.
🛠️ Proposed fix to separate assignment
- export LLAMA_STAGE_BUILD_DIR="$(
+ LLAMA_STAGE_BUILD_DIR="$(
LLAMA_STAGE_BACKEND="$BACKEND" \
LLAMA_STAGE_LINK_MODE=static \
"$SCRIPT_DIR/build-llama.sh" --print-build-dir
)"
+ export LLAMA_STAGE_BUILD_DIR📝 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.
| export LLAMA_STAGE_BUILD_DIR="$( | |
| LLAMA_STAGE_BACKEND="$BACKEND" \ | |
| LLAMA_STAGE_LINK_MODE=static \ | |
| "$SCRIPT_DIR/build-llama.sh" --print-build-dir | |
| )" | |
| LLAMA_STAGE_BUILD_DIR="$( | |
| LLAMA_STAGE_BACKEND="$BACKEND" \ | |
| LLAMA_STAGE_LINK_MODE=static \ | |
| "$SCRIPT_DIR/build-llama.sh" --print-build-dir | |
| )" | |
| export LLAMA_STAGE_BUILD_DIR |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 141-141: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 `@scripts/build-release.sh` around lines 141 - 145, Separate the
LLAMA_STAGE_BUILD_DIR declaration from its command-substitution assignment in
the release build flow. Keep the existing build-llama.sh invocation and
environment variables unchanged, then export LLAMA_STAGE_BUILD_DIR afterward so
its exit status propagates and set -e can detect failures.
Source: Linters/SAST tools
3 participants
Summary
Skippy can now use native MTP, upstream llama.cpp N-gram proposal, or a composite of both while preserving target verification for every committed token. The old synchronous
VerifySpanhot path is replaced byVerifyWindow, and deeper asynchronous verification is enabled only when request-local evidence predicts that overlap will repay stale speculative work.This also makes speculative strategy a product configuration surface. Model packages can select native MTP, simple prompt lookup, or stateful cache N-gram behavior, while
mesh-llmandskippy-servercan apply deployment overrides.Architecture
VerifySpanmessages and hot-path machinery with versionedVerifyWindowrequest/reply messages. No internal compatibility lane remains.verify_window_pipeline_depthas a maximum. A rolling exact-width profile estimates useful continuation overlap versus expected stale work before issuing a dependent future window.VerifyWindowverification when the observed shape is not profitable.Product Configuration
model-package.json, built-in config schema, validation, and package preflight.simple,cache, and disabled N-gram proposal modes.mesh-llmandskippy-serverconfiguration and CLI documentation.Protocol
This intentionally breaks the internal Skippy stage decode protocol. Old and new staged-runtime binaries do not interoperate. Public mesh gossip, the OpenAI-compatible API, package selection, and mixed-version public mesh behavior are unchanged.
Lab Validation
Both hosts were rebuilt from scratch at
2678813b4and reported0.72.1+g2678813b4. The model was loaded asmeshllm/GLM-4.7-Flash-MTP-GGUF:Q4_K_Mwith the normal--splitproduct path. The planner selectedmicstudio 0..47andstudio54 47..48, four direct private-LAN lanes, F16 activation wire data, context 4096, temperature 0, seed 42, and concurrency 1.Primary long-coding SPEED-Bench (
qualitative, coding, OSL 1024, limit 4), three repetitions per condition:Frozen historical control (
qualitative, coding and reasoning, OSL 512, limit 4), one repetition:Across the hybrid telemetry runs, N-gram proposed 2,624 tokens and contributed 1,121 accepted tail tokens. Pure N-gram fallback occurred 364 times. The adaptive policy recorded 875 window observations, 144 permit checks, and 97 permits. Metrics-server reports zero dropped events and zero export errors.
Verification
cargo test -p skippy-server --lib(273 passed)cargo check -p skippy-servercargo clippy -p skippy-server --all-targets -- -D warningscargo check -p mesh-llmcargo clippy -p mesh-llm --all-targets -- -D warningscargo fmt --all -- --checkjust release-buildon both lab hostsRemaining Performance Gate
The subsystem meets the functional gates: native MTP beats no MTP, hybrid N-gram contributes accepted tails, and hybrid beats native MTP. It does not meet the absolute 48.68 tok/s target derived from #875. The normal planner's
47/1topology is not comparable with #875/#858's historical22/26topology, so a topology-matched run remains necessary before claiming that absolute promotion result.Summary by CodeRabbit
New Features
Improvements
Documentation