🚬 Add bounded MTP + N-gram pipelining for latency-limited split inference - #1026
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.
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces standalone N-gram and checkpoint-based speculative flows with native-MTP cache handling, stage-state v9 authoritative positions, stale-window scheduling, updated ABI bindings, and revised CLI, configuration, tests, fixtures, scripts, and documentation. ChangesSpeculative decoding and protocol
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (2)
crates/skippy-protocol/src/binary/mod.rs (1)
315-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't isolate the kind-exclusion branch.
The third assertion changes
kindtoPrefillEmbdbut leavespos_startat-1(set in the prior step), so it's still exercising the sign check, not proving that a non-decode kind returnsNoneeven with a valid position.♻️ Proposed fix
decode.pos_start = -1; assert_eq!(decode.authoritative_session_position(), None); + decode.pos_start = 17; decode.kind = WireMessageKind::PrefillEmbd; assert_eq!(decode.authoritative_session_position(), None);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-protocol/src/binary/mod.rs` around lines 315 - 338, Update the test only_decode_messages_carry_authoritative_session_positions so the non-decode kind assertion restores pos_start to a valid non-negative value before changing kind to PrefillEmbd. Keep the existing negative-position assertion separate, ensuring the final assertion exercises kind exclusion rather than the sign check.crates/skippy-server/src/frontend/speculative.rs (1)
647-647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant variable shadowing.
The outer
commit_countvariable is already accurately incremented at the start of the loop, meaning it is exactly equal toaccepted_before_reject + 1when the first rejection occurs. You can safely remove this let binding.♻️ Proposed refactor
- let commit_count = accepted_before_reject + 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/speculative.rs` at line 647, Remove the inner commit_count binding at the first-rejection path and reuse the outer commit_count value already incremented by the loop. Preserve the existing rejection handling and all subsequent uses of commit_count.
🤖 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 437-440: Update the effective_strategy selection to use an if
expression based on ngram.is_some() instead of matching on a boolean, preserving
the existing native-mtp+ngram-cache and native-mtp values.
---
Nitpick comments:
In `@crates/skippy-protocol/src/binary/mod.rs`:
- Around line 315-338: Update the test
only_decode_messages_carry_authoritative_session_positions so the non-decode
kind assertion restores pos_start to a valid non-negative value before changing
kind to PrefillEmbd. Keep the existing negative-position assertion separate,
ensuring the final assertion exercises kind exclusion rather than the sign
check.
In `@crates/skippy-server/src/frontend/speculative.rs`:
- Line 647: Remove the inner commit_count binding at the first-rejection path
and reuse the outer commit_count value already incremented by the loop. Preserve
the existing rejection handling and all subsequent uses of commit_count.
🪄 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: be4c5a25-81f6-4a55-ba9f-0ab46d2bd24a
📒 Files selected for processing (87)
SKIPPY_PROTOCOL_TODO.mdcrates/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/plugin/config.rscrates/mesh-llm-host-runtime/src/runtime/local/native_runtime_events/tests.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-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-ffi/README.mdcrates/skippy-ffi/src/lib.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/session.rscrates/skippy-server/README.mdcrates/skippy-server/src/binary_transport/binary_messaging.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/binary_messaging/telemetry.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/binary_transport/stage_execution.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/backend.rscrates/skippy-server/src/frontend/embedded_execution.rscrates/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/mod.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/tests/multimodal.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/lib.rscrates/skippy-server/src/runtime_state.rsdocs/CLI.mddocs/USAGE.mddocs/design/TESTING.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.patch
💤 Files with no reviewable changes (22)
- docs/skippy/speculative_decoding.md
- docs/specs/speculative-decoding-wiring-plan.md
- crates/skippy-prompt/src/prompt_cli/draft.rs
- crates/mesh-llm-host-runtime/tests/fixtures/config_schema_defaults_ui_reference.json
- crates/skippy-server/src/frontend/backend.rs
- crates/skippy-prompt/src/prompt_cli/topology.rs
- crates/skippy-server/src/frontend/generation_flow.rs
- crates/skippy-prompt/src/prompt_cli/launch.rs
- third_party/llama.cpp/patches/0017-Expose-upstream-ngram-simple-draft-ABI.patch
- crates/mesh-llm/src/lib.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs
- crates/mesh-llm-host-runtime/tests/fixtures/schema_driven_controls_valid.toml
- crates/skippy-server/src/frontend/tests/multimodal.rs
- crates/skippy-server/src/binary_transport/binary_messaging.rs
- crates/skippy-prompt/src/prompt_cli/binary_repl.rs
- crates/skippy-server/src/frontend/generation/types.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/mesh-llm-host-runtime/src/inference/skippy/resolver/native_mtp_tests.rs
- crates/skippy-server/src/frontend/wire_messages.rs
- crates/skippy-prompt/src/prompt_cli/args.rs
|
@michaelneale Follow-up plan for answering the parallel-busy-time / WAN-latency question and bringing this closer to Shard. The positional protocol in this PR is the prerequisite: verify windows carry authoritative positions, stages repair KV locally, and rejection no longer requires checkpoint/restore ACKs or repair/reverify. The remaining work is to turn the current depth-2 burst into a continuously refilled pipeline and prove that it hides latency. A useful sizing condition is:
Once that many independent windows are circulating, propagation delay becomes pipeline fill rather than an inter-token stall; throughput approaches the slowest stage/link service rate. Initial fill, rejection recovery, and link serialization still remain visible. Implementation plan:
The success signal is not merely reaching depth 8. It is stage idle time collapsing as depth grows, exposed downstream wait approaching zero, TPS plateauing at slowest-stage throughput, and additional depth increasing stale work without improving throughput. This is the same core loop as Shard's pipelined coordinator: direct return, multiple causally speculative chunks in flight, FIFO completion, and stale-result discard after divergence. The comparison we need to prove is whether our positional repair plus MTP+Ngram can achieve the same WAN latency hiding without reintroducing rollback/recovery serialization. |
|
One thing worth being explicit about for the WAN goal (#936 / #1025): this reaches depth 2, but not the deeper "keep the pipe full" regime shard uses, because the proposer is still coupled to the verify traversal. The next MTP draft comes from the target's own forward ( Shard's |
|
@michaelneale The structural depth-two cap you identified is removed in The important clarification is that two is not configured or steady-state concurrency. It is now named and emitted as The proposer is no longer blocked on the single On the two-node GLM-4.7 split with 100 ms injected one-way downstream delay, the four-prompt matched result was:
The exact final binary canary then reached selected and actual depth 8, spent 47.71% of decode time with multiple windows in flight, and produced 30.75 decode TPS / 13.076 s latency. So the answer to “are both sides busy in parallel / is latency hidden?” is: materially, but not completely. At 100 ms the active depth grows to seven/eight and cuts request latency by ~60% in the matched run. Propagation overlaps while credits are full. Startup, proposal gaps, divergence recovery, bandwidth serialization, and the first corrected reply still leave bubbles; the 35-48% parallel-occupancy result tells us exactly how much room remains. On LAN the controller only reached actual depth two and spent 17.69% parallel, which is the expected behavior when deeper speculation does not repay itself. This is now directly comparable to Shard's pipeline loop: bounded fill, predictor-ahead speculative chunks, FIFO commit, stale discard. Shard is given a depth bound; it does not infer the economically optimal depth from a serial trace. The extra piece here is the latency/stale-cost controller around that loop, plus positional repair instead of rollback. The PR description now contains the full LAN, 100 ms, prior-baseline, and #938 comparison tables and the validation record. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/skippy-server/src/binary_transport/binary_messaging/async_forwarder.rs (1)
137-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the write duration once and don't label failed writes as a successful
write_ms.
elapsed_ms(job.enqueued_at)is computed twice (once forresult, once for the telemetry attribute), and the"llama_stage.forward_write_ms"attribute is inserted unconditionally, even whenwrite_stage_message_after_propagationfails. This mixes error cases into a duration metric meant to reflect a successful write, which will muddy exactly the latency/occupancy telemetry this PR's follow-up plan relies on.♻️ Proposed fix
fn forward_job(writer: &mut TcpStream, telemetry: &Telemetry, job: AsyncForwardJob) { - let result = - write_stage_message_after_propagation(writer, &job.message, job.wire_dtype, job.condition) - .context("async forward activation frame downstream") - .map(|()| elapsed_ms(job.enqueued_at)) - .map_err(|error| format!("{error:#}")); + let write_result = + write_stage_message_after_propagation(writer, &job.message, job.wire_dtype, job.condition) + .context("async forward activation frame downstream"); + let write_ms = elapsed_ms(job.enqueued_at); let write_end_unix_nanos = now_unix_nanos() as u64; let mut attrs = job.attrs; - attrs.insert( - "llama_stage.forward_write_ms".to_string(), - json!(elapsed_ms(job.enqueued_at)), - ); + attrs.insert("llama_stage.forward_ok".to_string(), json!(write_result.is_ok())); + if write_result.is_ok() { + attrs.insert("llama_stage.forward_write_ms".to_string(), json!(write_ms)); + } telemetry.emit_debug_span( "stage.binary_downstream_write", attrs, job.enqueued_unix_nanos, write_end_unix_nanos, ); + let result = write_result + .map(|()| write_ms) + .map_err(|error| format!("{error:#}")); let _ = job.done.send(result); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/binary_transport/binary_messaging/async_forwarder.rs` around lines 137 - 156, Update forward_job to compute elapsed_ms(job.enqueued_at) once after the write attempt, reuse that value for the successful result and telemetry, and only insert "llama_stage.forward_write_ms" into attrs when write_stage_message_after_propagation succeeds; failed writes must retain their error result without a successful-write duration attribute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@crates/skippy-server/src/binary_transport/binary_messaging/async_forwarder.rs`:
- Around line 137-156: Update forward_job to compute elapsed_ms(job.enqueued_at)
once after the write attempt, reuse that value for the successful result and
telemetry, and only insert "llama_stage.forward_write_ms" into attrs when
write_stage_message_after_propagation succeeds; failed writes must retain their
error result without a successful-write duration attribute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b56628db-7553-4c70-aaeb-c92499a09aa8
📒 Files selected for processing (14)
crates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rscrates/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/direct_return.rscrates/skippy-server/src/binary_transport/wire.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/types.rscrates/skippy-server/src/frontend/native_mtp/verify_window.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/skippy-server/src/frontend/generation/types.rs
- crates/skippy-server/src/binary_transport/binary_messaging.rs
- crates/skippy-server/src/frontend/native_mtp/verify_window.rs
- crates/skippy-server/src/binary_transport/binary_messaging/connection.rs
- crates/skippy-server/src/frontend/embedded_generation.rs
|
@michaelneale This diagnosis was exactly right for the implementation at the time of the comment: a target-produced The follow-up at
I also found and fixed a correctness bug while tracing this: batched stage-0 serial correction could execute on the rejected suffix because alignment and decode were not atomic. All earlier Corrected two-node GLM-4.7 split, controlled 100 ms one-way delay, 3 matched repetitions / 1,152 generated tokens per condition:
That is +32.65% decode TPS and -19.14% latency despite 30.88 points lower acceptance. The hybrid proposed 1,284 tokens and accepted 678, versus MTP's 570 proposed / 477 accepted: lower acceptance ratio, but more useful speculative work completed. The busy-time answer is workload-dependent and the new description separates it explicitly:
So this now runs ahead independently in the Shard sense, but it does not claim continuous saturation: the bounded N-gram horizon plus fill/drain still leave bubbles. It fills enough to hide substantial latency when the text is draftable and refuses to manufacture stale traffic when it is not. One nuance on “doesn't Shard figure depth out?”: current Shard has an optional acceptance-EMA throttle from 1 to a configured maximum. It starts optimistic at that maximum; it does not infer the maximum from measured traversal latency. This PR adds that latency-derived target on top of the refill/FIFO/stale-discard loop. The PR description now contains the corrected results, #938 comparison, Shard source comparison, and exact validation details. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-server/src/frontend/generation_flow.rs`:
- Around line 859-867: Update the batch decode call to pass sampling only when
request.sampling.enabled is true, matching the conditional behavior used by
embedded_generation.rs. Also consume batch_outcome.session_alignment and emit
the same debug telemetry used for session auto-alignments in that path.
🪄 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: 527b9311-7140-4628-aa90-b4215312e7ac
📒 Files selected for processing (5)
crates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/decode_batcher.rscrates/skippy-server/src/frontend/decode_scheduler.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation_flow.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/skippy-server/src/binary_transport/binary_messaging/connection.rs
- crates/skippy-server/src/frontend/embedded_generation.rs
- crates/skippy-server/src/frontend/decode_scheduler.rs
|
@michaelneale Final follow-up to the structural coupling point in your comment: The hybrid no longer has to drain all active windows when one bounded proposal runs out. Before consuming the current horizon, it queries the committed-only N-gram index with the complete optimistic suffix (including dispatched-but-uncommitted tokens), appends any continuation, and immediately spends free verify credits. The suffix is never indexed until target commit. FIFO commit, epoch stale-discard, and positional repair are unchanged. Fresh 3-process matched GLM-4.7 A/B at controlled 100 ms one-way delay (6 requests / 1,152 generated tokens per condition):
Outputs matched MTP hashes in all repetitions. Relative to the corrected pre-refill hybrid baseline already on this PR, this is another +3.06% decode TPS / +2.12% wall TPS / -2.07% latency. The busy-in-parallel answer is now measured rather than inferred:
At 0/50/100 ms delay, hybrid parallel time rises 46.05% -> 51.84% -> 58.35%, while its wall-TPS advantage over MTP rises +34.67% -> +61.73% -> +71.65%. That is the WAN behavior we wanted: propagation is increasingly hidden behind useful work, but fill/drain, finite predictor horizon, and serialized bandwidth remain visible. Cap 2/4/8 on the 100 ms copy prompt reaches actual depth 2/3/3 and wall TPS 12.046/12.509/12.510. So cap 8 is not "concurrency eight" and forcing it would not help—the scheduler finds the plateau at actual depth 3. This now matches Shard's predictor-ahead fill/FIFO/stale-discard loop; the extra mechanism here is latency-derived depth plus positional repair rather than rollback. The PR description has the full tables, #938 comparison, exact binary identity, and validation record. |
i386
changed the title
|
@michaelneale Final production update on the “how busy are the stages / is WAN latency hidden?” question. First, one correction to my earlier explanation: authoritative position is enough to repair attention KV, but not the whole GLM-4.7 MTP sidecar. The sidecar also owns a recurrent hidden prefix and pending hidden vector. We now restore those from a bounded, process-local checkpoint on speculative trim. There is still no rollback/repair message or replay on the wire. Final matched sustained copy result at 100 ms one-way delay:
So yes: on draftable c1/c2 work, multiple dependent verify windows occupy the split concurrently and materially hide the 100 ms delay. At c4/c8 the independent requests already occupy the pipeline, so the global credit policy removes optional N-gram work instead of oversubscribing it. The representative code/reasoning/copy/tool matrix is deliberately less exciting: LAN -0.96%, 20 ms +0.82%, 50 ms -0.69%, 100 ms +1.56%, and mixed WAN +0.61%. That meets the aggregate no-harm goal, but does not meet the requested +20% mixed-WAN bar. The >=20% proof is specific to sustained, highly draftable c1 text. Relative to Shard, the refill/FIFO/stale-discard mechanism is now similar, but our scheduling adds MTP anchoring, latency/profitability depth selection, and globally fair request credits. Shard still has broader multi-stage and resumable failure machinery. Here, killing stage 1 during prefill now returns a bounded 502 in 29.06 seconds instead of hanging; the active request is not resumed. The PR description now has the clean final tables, #938 and previous-run comparison, correctness repair, failure results, final binary identity, and production caveat. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/native_mtp/verify_window.rs (1)
26-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFunction complexity:
execute_native_mtp_verify_windowremains very large and now handles more branching.This function spans ~430 lines with 30+ parameters and now adds
exact_positional_verify,decision_rejected_native_mtp_prefix, anddependent_target_rejectedhandling on top of the existing accept/reject/telemetry logic. 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." Consider extracting the proposal-buffer accept/reject accounting (lines 225-259) and the telemetry-attribute population (lines 319-450) into dedicated helpers.🤖 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 26 - 456, Reduce the complexity of execute_native_mtp_verify_window by extracting the proposal-buffer accept/reject accounting into a semantically named helper and moving telemetry-attribute population into a dedicated helper. Preserve the existing outputs, cooldown decisions, buffer exhaustion handling, and emitted telemetry values while keeping execute_native_mtp_verify_window focused on orchestration.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-server/src/frontend/speculative_credits.rs`:
- Around line 196-216: Update SpeculativeCreditRequest::drop to perform cleanup
only when this handle’s registered flag is true. Preserve the existing waiter
removal and request deactivation/removal behavior for successfully registered
requests, while ensuring duplicate handles returned by register() cannot modify
or remove the shared request entry.
In `@crates/skippy-server/src/frontend/speculative.rs`:
- Around line 284-291: Update the validation error returned by the cache N-gram
proposer to interpolate skippy_runtime::NGRAM_CACHE_MAX_NGRAM instead of
hardcoding 4, keeping the existing ngram_min/ngram_max validation conditions
unchanged.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/native_mtp/verify_window.rs`:
- Around line 26-456: Reduce the complexity of execute_native_mtp_verify_window
by extracting the proposal-buffer accept/reject accounting into a semantically
named helper and moving telemetry-attribute population into a dedicated helper.
Preserve the existing outputs, cooldown decisions, buffer exhaustion handling,
and emitted telemetry values while keeping execute_native_mtp_verify_window
focused on orchestration.
🪄 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: c30e86ce-0b07-425f-80cf-3fcbd5dadddd
📒 Files selected for processing (34)
crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/runtime/survey.rscrates/skippy-metrics/src/lib.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/kv_eviction.rscrates/skippy-server/src/binary_transport/restore_prefill_decode.rscrates/skippy-server/src/frontend.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_credits.rscrates/skippy-server/src/frontend/tests/mod.rscrates/skippy-server/src/frontend/tests/multimodal.rscrates/skippy-server/src/frontend/tests/prefill.rscrates/skippy-server/src/frontend/tests/prompting.rscrates/skippy-server/src/kv_integration/resident_prefix.rsdocs/plugins/telemetry.mddocs/skippy/PIPELINED_VERIFY_WINDOW.mdthird_party/llama.cpp/patches/0020-Re-prime-native-MTP-after-state-restoration.patchthird_party/llama.cpp/patches/0021-Fix-N-gram-confidence-threshold-indexing.patchthird_party/llama.cpp/patches/0022-Restore-native-MTP-state-on-speculative-trim.patch
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/skippy-server/src/frontend.rs
- crates/skippy-server/src/frontend/tests/prompting.rs
- crates/skippy-server/src/binary_transport/binary_messaging/connection.rs
- crates/skippy-server/src/frontend/tests/multimodal.rs
- crates/skippy-server/src/frontend/generation/server.rs
- crates/skippy-server/src/frontend/native_mtp/mod.rs
- crates/skippy-server/src/frontend/embedded_execution.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs
- crates/skippy-server/src/binary_transport/binary_messaging/async_forwarder.rs
- crates/skippy-server/src/frontend/decode_scheduler.rs
- crates/skippy-server/src/frontend/native_mtp/decode.rs
|
Let's hold off on merging this one for now — it overlaps with an active research/WIP effort on the split-runtime decode path (both rewrite To be clear this isn't a quality concern — the benchmark writeup is excellent and the net simplification (deleting the checkpoint ring / adaptive-depth / credit-pool machinery) is genuinely appealing. I just want to sequence it deliberately against the in-flight work. Also worth flagging: this collides internally with #1034 (skippy-ffi ABI) and #1032 ( Will follow up on sequencing. Leaving open, just please don't merge yet. |
|
@michaelneale — tagging you for review, particularly the new stage-overlap/latency evidence and the Shard comparison in the PR description. |
ndizazzo
left a comment
ndizazzo
left a comment
There was a problem hiding this comment.
Lightweight review pass - found a couple things
| description: 'Speculative runtime mode.', | ||
| inheritedLabel: 'Inherited by speculative defaults', | ||
| valueSchema: { kind: 'enum', values: ['draft', 'ngram'] }, | ||
| valueSchema: { kind: 'enum', values: ['draft', 'disabled'] }, |
There was a problem hiding this comment.
@i386 Is this the configuration surface you want for this work? This means that the user can't select Ngram MTP via any options config.
There was a problem hiding this comment.
@i386 this comment seems to have been missed - @michaelneale opened #1056
| | `llama_stage.verify_window.direct_return_upstream_opened` | Skippy decode summary | Boolean indicating that the preferred upstream-opened v10 prediction-return sink completed its handshake. | | ||
| | `llama_stage.verify_window.direct_return_reverse_fallback` | Skippy decode summary | Boolean indicating that the final stage used the bounded reverse-open v10 prediction-return fallback after the preferred sink was unavailable. | |
There was a problem hiding this comment.
Somewhat broken naming convention for these - not namespaces to mesh_llm.
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
…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.
|
🤖 Pushed by micn's AI agent. Pushed two small WAN-hardening commits onto this branch (
Why: this PR's latency sweep (+161%→+281%) is injected-delay on LAN. These two fixes are what made a real internet split serve at all in the lab. Next step is the first real-WAN datapoint for v10: M5 (Sydney) ↔ cheap vast.ai AU 4090, GLM-4.7-Flash-MTP Q4_K_M 2-stage locked-topology split, native MTP vs MTP+ngram width 4 / depth 2. Numbers will be posted here. Validation: Not ported (deliberately): the lab branch's pre-warmed return-sink pool. v10 reworked |
|
🤖 Posted by micn's AI agent (working on this branch with micn). First real-WAN validation of v10 — resultsRan the first real-internet split test of this branch today: M5 (Sydney, Apple M5 Max, stage 0) ↔ vast.ai RTX 4090 (AU, stage 1, layers 27..48), 21–25ms real WAN RTT, GLM-4.7-Flash-MTP Q4_K_M, 2-stage split, greedy decode,
Raw per-rep JSON (full The two commits at the tip (
|
…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.
|
🤖 Posted by micn's AI agent. Follow-up to finding 2 in the WAN report above: pushed Root cause: Fix shape (second-opinion reviewed): new Validation: fmt clean, Still queued (not yet on branch): prepared-return-sink pool port for the cold-open 502 itself (finding 1). |
…ngram * origin/main: Add GLM DSA correctness trace reporting (#1033) Add GLM DSA package contract validation (#1032) Add direct quant layer package tooling (#1031) Lock split topology placement (#1050) fix nightly stability Qwen thinking ui(data-mode): migrate legacy harness storage to v2 (#1040) fix: Windows Vulkan runtime dependencies (#1046) test: validate MI300 runtime selection and placement (#1045) fix: support ROCm RDNA APU kernels safely (#1044) fix: ROCm GPU enumeration (#1039) Document public packaging installation channels Fix Windows autoupdate import (#1030) Update llama.cpp upstream pin
…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.
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.
3 participants
What changed
This replaces the earlier repair-oriented experiment with a deliberately breaking, Shard-shaped stage protocol for native GLM-4.7 MTP + request-local N-gram speculation. There is no standalone draft model or draft-model experiment in this PR.
verify_window_pipeline_depth;The public mesh and OpenAI protocols are unchanged. The internal stage protocol is intentionally incompatible; all stages in a split must run v10.
Canonical benchmark gate: NVIDIA SPEED-Bench under a locked topology
The tables below are controlled diagnostic microbenchmarks, not the final headline benchmark. Before this is promoted or merged, the performance claim will be rerun with the full upstream NVIDIA SPEED-Bench qualitative split (all 880 samples and all categories, OSL 1024, temperature 0) through the OpenAI-compatible endpoint.
Both A/B arms will use the locked-topology implementation merged from #1050 /
jd/locked-split-topology:model-package.jsonSHA-256;micstudio.local:0..25thenstudio54-3.local:25..48(25/23);micstudio.local:0..47thenstudio54-3.local:47..48(47/1);--split-topology-lockpassed to both nodes;/api/runtime/stagesbefore traffic starts;The only serving-policy difference between matched runs will be native MTP versus native MTP + request-local N-gram width 4 / fixed depth 2. The locked 25/23 split is the primary production-like comparison; the locked 47/1 split is explicitly an asymmetric latency-hiding stress case, not the headline topology. Full qualitative runs use request concurrency 1 at natural LAN latency and at 100 ms injected stage latency. Concurrency 8 is a separate saturation run on the balanced split. The report will include per-category and overall decode TPS, end-to-end latency, TTFT, ITL, acceptance, in-flight occupancy, stale work, and joined stage-compute overlap. The SPEED-Bench throughput split will separately test low-, mixed-, and high-entropy load.
Current diagnostic result
Two-node GLM-4.7 Flash MTP Q4_K_M split, 47/1 layers, eight lanes, F16 activations, 32K context, 100 ms injected inter-stage delay. The selected setting is N-gram 4, verification width 4, fixed depth 2.
This is the result we set out to prove: MTP + N-gram beats native MTP even though its acceptance rate is substantially lower. Acceptance is not throughput; overlapping useful target work can outweigh rejected proposals.
On a separate four-prompt novel-code corpus, selected depth 2 was essentially neutral: 8.7018 versus 8.7921 decode TPS (-1.03%), with 4/4 exact hashes and 32.8% acceptance. The win is therefore workload-dependent rather than a universal claim.
Concurrency
Same mixed corpus and selected depth-2 configuration:
Depth is intentionally fixed per request. At c8, depth 2 can place 16 windows against eight lanes and slightly overfill the service. This is the current operating-envelope limit; the deleted global-credit/adaptive machinery is not being smuggled back in. Production policy should choose depth with total admitted concurrency in mind or batch callers into shared pipeline jobs as Shard does.
Does speculation fill the pipeline?
Yes, on eligible repeated spans. At depth 2 and 100 ms injected delay:
The queue is not merely configured to two; it is measurably full for almost the entire eligible interval. Accumulated verification time exceeding wall decode time and the burstier ITL distribution show overlapped in-flight target work. TTFT is unchanged because this optimization starts after prefill.
How much are the stages actually computing in parallel?
Debug traces now join the stage-0 and stage-1 target-compute spans by run, request, session, and window. Each trace matched all 97 windows on both stages; every simultaneous-compute pair was from different windows, which is the pipeline behavior we wanted to observe.
So the precise answer to the review question is nuanced: the inference FIFO is full and hides stage/network latency, but both stage runtimes are simultaneously computing for only 4.5–6.2% of the verify interval at depth 2. The 47/1 tail is intentionally tiny; in the 25/23 split, the slower stage 1 is busy 71.4% of the interval while stage 0 works ahead in shorter bursts. The gain comes from keeping the bottleneck supplied, overlapping different windows, hiding the ~100 ms edge delay (median same-window stage-0-end to stage-1-start was 113–117 ms), and amortizing traversals with wider chunks—not from both GPUs being continuously busy together.
These are host-measured synchronous stage-compute spans, not Metal hardware-counter samples. They measure when each stage runtime is executing target work; they do not claim cycle-level shader occupancy.
Network-latency sweep
Highly repeatable 384-token workload, 47/1 split, width 4, depth 2; exact output in every row:
The delay between stages is therefore substantially hidden when the request-local horizon can keep the FIFO supplied. It is not eliminated: TTFT/prefill remain serial, divergence wastes later work, and low-reuse workloads cannot sustain the horizon.
Depth and width frontier
The same 100 ms repeat workload establishes the upper bound and where added depth stops buying much throughput:
Throughput approaches the bottleneck service rate around depth 8; depth 16 adds only 4.4%. Depth 2 is selected for mixed traffic because it captures overlap without the concurrency/stale-work exposure of a deep per-request queue.
At depth 2, the width sweep separates pipeline overlap from wider target chunks:
Width 1 is already +10.2% over native MTP at 100 ms, showing that overlap alone helps. Full chunks provide the larger gain by amortizing each traversal across more verified tokens.
Topology
The effect is not limited to the asymmetric 47/1 placement:
Both rows use the 100 ms repeat workload, width 4, depth 2, and exact output comparison.
Comparison with the previous runs
These are separate run families, so the percentage deltas are the honest comparison—not the absolute TPS values.
The new path trades the old c1 peak for much stronger c2/c4 scaling and a far simpler execution model. It also exposes the real c8 overfill limit rather than obscuring it with a global credit controller.
How similar is this to Shard / issue #1025?
This now copies Shard's important execution pattern much more directly:
The important differences are:
That answers the central questions in #1025: parallel speculation can hide stage/network latency when the candidate horizon stays supplied; depth must cover the pipeline rather than be hard-coded to concurrency two; and production concurrency still needs shared batching/admission so independent per-request queues do not overfill the lanes.
Validation
cargo fmt --all --checkcargo test -p skippy-protocol --lib— 43 passedcargo test -p skippy-server --lib— 281 passedcargo test -p mesh-llm-config --lib— 95 passedcargo test -p mesh-llm-host-runtime --lib— 1,695 passed, 8 ignoredmesh-llmMESH_LLM_DYNAMIC_NATIVE_RUNTIME=0 just release-buildjd/locked-split-topology/ Lock split topology placement #1050 into this branch;178a6c44937154dc4c4eff0d166f4a044c4fceba;cargo test -p skippy-coordinator --lib— 37 passed;cargo test -p mesh-llm-cli --lib— 72 passed;cargo test -p mesh-llm-host-runtime --lib split_topology— 10 passed;cargo check -p mesh-llmand warning-denying Clippy across the topology-touched crates;Production limitations
Addresses the implementation questions raised in #1025; package-default promotion should remain a separate production decision.