feat(metrics): spec-decode acceptance counters for the DFlash draft path - #787
feat(metrics): spec-decode acceptance counters for the DFlash draft path#787scatyf3 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26423fd860
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// Requests admitted but not yet running (KV pressure, prefetch wait). | ||
| pub num_waiting_reqs: u64, | ||
| /// Cumulative spec-decode counters, or `None` when no draft model is loaded. | ||
| pub spec_decode: Option<SpecDecodeCounters>, |
There was a problem hiding this comment.
Keep Qwen3.5 LoadSnapshot literals buildable
Adding spec_decode as a required LoadSnapshot field breaks the Qwen3.5 feature path: openinfer-qwen35/src/scheduler.rs:838 still constructs LoadSnapshot { kv_used_blocks, kv_total_blocks, num_running_reqs, num_waiting_reqs } without either spec_decode: None or ..LoadSnapshot::default(), so cargo ... --features qwen35 fails with a missing-field error and the supported Qwen3.5 engine cannot build. Please update that scheduler publisher (and any other exhaustive literals) when extending this shared struct.
AGENTS.md reference: AGENTS.md:L9-L14
Useful? React with 👍 / 👎.
xiaguan
left a comment
There was a problem hiding this comment.
Thanks for adding this plumbing. The core transport choice looks right: the scheduler publishes cumulative counters so a coalescing watch cannot lose increments, while the frontend converts them to interval deltas before calling the vLLM metrics API. The manual real-model correlation in the PR description is also useful evidence.
I found several items that need to be addressed before merge:
-
Please fix the existing automated P1 in the Qwen3.5 scheduler.
openinfer-qwen35/src/scheduler.rs:838still constructsLoadSnapshotwithout the newspec_decodefield. That package is not in the current CI matrix, but this is a required-field Rust struct literal and will fail when Qwen3.5 is compiled. The automated review is correct and has not been addressed yet. -
Please restore green CI. CPU Clippy currently fails because
LoadSnapshotgrew to 328 bytes andopeninfer-sim/tests/frontend_e2e.rs:185passes it by value (large_types_passed_by_value). The DCO sign-off check is also failing. -
Please give the metrics types a module boundary and reduce the commentary. I do not think the placement is the contributor's fault:
LoadSnapshotalready lived inengine.rs, and there was no existing engine metrics module to extend. However, this PR adds about 128 lines there and takesengine.rsfrom 940 to 1068 lines. Please introduce anopeninfer-engine/src/metrics.rs(with the necessary re-exports) forLoadSnapshot,SpecDecodeCounters, and their focused tests. While moving it, shorten the comments to the invariants a maintainer actually needs. In particular, the long checkpoint/current-workspace/CI narrative aroundMAX_SPEC_TOKENSand the repeated idle-interval narration obscure a fairly small contract. -
Please tighten the tests instead of testing arithmetic several times. The position-prefix tally test is valuable:
[2, 1, 0]is a non-obvious representation and should stay. The async publisher test is also valuable because it exercises first publish, idle omission, and resume. The extra tail-zero/sum/monotonic assertions and most of their narration are redundant.spec_delta_telescopes_to_cumulativemostly re-tests subtraction and currently does not even cover per-position deltas; please fold the meaningful coalescing/per-position assertion into the async publisher test and remove the arithmetic-only test. A real/metricsE2E would be better, but I am fine tracking that as a follow-up rather than blocking this PR because the PR already includes a manual real-model comparison. -
Do not silently change the configured K in the public snapshot.
SpecDecodeCounters::newstoresmin(K, 32)in a field documented as the configurednum_spec_tokens. For a wider checkpoint the totals remain exact, but the metric reports a different K and silently presents a truncated acceptance curve as complete. The simplest contract is to reject unsupported K during speculative-model loading; alternatively, preserve the actual K and model truncation explicitly instead of overwriting it.
Once these are addressed, the remaining executor → scheduler → bridge → Prometheus data path looks sound to me.
|
Thank you for your review~ I'll fix my PR by the end of this week at the latest |
…K bound Addresses review items 3-5 on pegainfer-project#787. **A metrics module.** `LoadSnapshot`, `SpecDecodeCounters` and `MAX_SPEC_TOKENS` move to `openinfer-engine/src/metrics.rs`, re-exported from `engine` so every `openinfer_engine::engine::LoadSnapshot` / `openinfer_core::engine::*` import still resolves untouched. `engine.rs` goes 1068 -> 926 lines, below the 940 it sat at before this branch. The commentary is cut to the invariants: the checkpoint/current-workspace/CI narrative around `MAX_SPEC_TOKENS` and the idle-interval narration in `publish_scheduler_stats` said at length what the code says. **A rejected K, not a silently clamped one.** `SpecDecodeCounters::new` stored `min(K, 32)` in a field documented as the drafter's configured `K`. Totals stayed exact, but the metric advertised a `K` the drafter did not have and passed a truncated acceptance curve off as a complete one. It now returns `Err(SpecWidthUnsupported)`, and `load_dflash_draft_model` propagates that before any executor state moves. Nothing we ship is affected: DFlash-b16 needs 15 and dspark block7 needs 7, against a bound of 32. `spec_decode_delta` drops its now-redundant `min(K, MAX_SPEC_TOKENS)` clamp in favour of `take(K)` on the iterator, which cannot panic on a bad width the way the slice could — the guard becomes unnecessary rather than merely unused. **Tests that carry their weight.** `spec_delta_telescopes_to_cumulative` was re-testing subtraction and never covered per-position deltas at all; the coalescing and per-position assertions it should have made are folded into the async publisher test, which now checks the first-publish delta, per-position widths, idle omission, and a two-verify-step gap arriving in one coalesced snapshot. The position-prefix tally test keeps its `[2, 1, 0]` assertion and loses the tail-zero/sum/monotonic restatements around it. The clamp test becomes a rejection test. Verified: CPU Clippy and Qwen3 CUDA Clippy over CI's exact package sets with `-D warnings`, `--features qwen35` Clippy, and the engine / frontend / qwen3 / core lib tests plus the sim `frontend_e2e` suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
The review's remaining follow-up on pegainfer-project#787: nothing asserted what the exposition layer actually serves. The bridge unit tests stop at `SchedulerStats`, so the `inc_by` accumulation, the label sets, and the `_total` suffix `prometheus-client` appends were all unverified — and the registered name scrapes empty, which is exactly the kind of mistake a unit test cannot see. Runs against the simulated engine, so it needs no GPU and no draft checkpoint and lands in the existing `simulated-frontend-e2e` job. It publishes cumulative `SpecDecodeCounters` on a `LoadSnapshot` and scrapes `GET /metrics`, asserting: - the first interval's counters, - that after two verify steps arrive coalesced in one snapshot the counters read back *exactly* the scheduler's cumulative — the totals -> delta -> `inc_by` round trip is only correct if nothing is double-counted or dropped, and this equality is the one place that shows it, - per-position series for `position` 0 and 1 with nothing past `K = 2`, so the fixed `MAX_SPEC_TOKENS` array width cannot leak into the exposition, - engine 1, which never drafted, reading zero — which is also what pins each delta to a single engine. Mutation-checked: publishing cumulative instead of deltas, and dropping the per-position `take(K)`, each fail it. `wait_for_metrics` now delegates to a `wait_for_labeled_metrics` that matches arbitrary extra labels, since the per-position family is keyed by `position` on top of engine and model; existing call sites are unchanged. The new server gets its own `model_name` because the Prometheus registry is process-wide and concurrent tests would otherwise read each other's counters. Not covered: executor -> scheduler. The simulated engine has no Qwen3 executor, so `observe_draft` firing correctly from `execute_speculative_verify_impl` still rests on the manual real-model comparison in the PR description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
4896d83 to
dfcb7f2
Compare
…apshot grew Two fallouts from adding `spec_decode` to `LoadSnapshot`, both flagged on pegainfer-project#787. `openinfer-qwen35/src/scheduler.rs` publishes through an exhaustive struct literal, so the new required field broke `--features qwen35` with E0063. That package is outside CI's matrix, which is why it went unnoticed. Qwen3.5 has no draft path, so the field is `None`. `LoadSnapshot` also crossed Clippy's `large_types_passed_by_value` threshold — `[u64; 32]` of per-position accepts puts it at 328 bytes — and the sim E2E helper still took it by value, failing CPU Clippy. Takes `&LoadSnapshot` now; the watch channel still gets an owned copy. Verified: CPU Clippy (the exact CI package set, `-D warnings`) is clean, `cargo check -p openinfer-qwen35 --features qwen35` compiles, and all 13 `openinfer-sim --test frontend_e2e` tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
…K bound Addresses review items 3-5 on pegainfer-project#787. **A metrics module.** `LoadSnapshot`, `SpecDecodeCounters` and `MAX_SPEC_TOKENS` move to `openinfer-engine/src/metrics.rs`, re-exported from `engine` so every `openinfer_engine::engine::LoadSnapshot` / `openinfer_core::engine::*` import still resolves untouched. `engine.rs` goes 1068 -> 926 lines, below the 940 it sat at before this branch. The commentary is cut to the invariants: the checkpoint/current-workspace/CI narrative around `MAX_SPEC_TOKENS` and the idle-interval narration in `publish_scheduler_stats` said at length what the code says. **A rejected K, not a silently clamped one.** `SpecDecodeCounters::new` stored `min(K, 32)` in a field documented as the drafter's configured `K`. Totals stayed exact, but the metric advertised a `K` the drafter did not have and passed a truncated acceptance curve off as a complete one. It now returns `Err(SpecWidthUnsupported)`, and `load_dflash_draft_model` propagates that before any executor state moves. Nothing we ship is affected: DFlash-b16 needs 15 and dspark block7 needs 7, against a bound of 32. `spec_decode_delta` drops its now-redundant `min(K, MAX_SPEC_TOKENS)` clamp in favour of `take(K)` on the iterator, which cannot panic on a bad width the way the slice could — the guard becomes unnecessary rather than merely unused. **Tests that carry their weight.** `spec_delta_telescopes_to_cumulative` was re-testing subtraction and never covered per-position deltas at all; the coalescing and per-position assertions it should have made are folded into the async publisher test, which now checks the first-publish delta, per-position widths, idle omission, and a two-verify-step gap arriving in one coalesced snapshot. The position-prefix tally test keeps its `[2, 1, 0]` assertion and loses the tail-zero/sum/monotonic restatements around it. The clamp test becomes a rejection test. Verified: CPU Clippy and Qwen3 CUDA Clippy over CI's exact package sets with `-D warnings`, `--features qwen35` Clippy, and the engine / frontend / qwen3 / core lib tests plus the sim `frontend_e2e` suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
The review's remaining follow-up on pegainfer-project#787: nothing asserted what the exposition layer actually serves. The bridge unit tests stop at `SchedulerStats`, so the `inc_by` accumulation, the label sets, and the `_total` suffix `prometheus-client` appends were all unverified — and the registered name scrapes empty, which is exactly the kind of mistake a unit test cannot see. Runs against the simulated engine, so it needs no GPU and no draft checkpoint and lands in the existing `simulated-frontend-e2e` job. It publishes cumulative `SpecDecodeCounters` on a `LoadSnapshot` and scrapes `GET /metrics`, asserting: - the first interval's counters, - that after two verify steps arrive coalesced in one snapshot the counters read back *exactly* the scheduler's cumulative — the totals -> delta -> `inc_by` round trip is only correct if nothing is double-counted or dropped, and this equality is the one place that shows it, - per-position series for `position` 0 and 1 with nothing past `K = 2`, so the fixed `MAX_SPEC_TOKENS` array width cannot leak into the exposition, - engine 1, which never drafted, reading zero — which is also what pins each delta to a single engine. Mutation-checked: publishing cumulative instead of deltas, and dropping the per-position `take(K)`, each fail it. `wait_for_metrics` now delegates to a `wait_for_labeled_metrics` that matches arbitrary extra labels, since the per-position family is keyed by `position` on top of engine and model; existing call sites are unchanged. The new server gets its own `model_name` because the Prometheus registry is process-wide and concurrent tests would otherwise read each other's counters. Not covered: executor -> scheduler. The simulated engine has no Qwen3 executor, so `observe_draft` firing correctly from `execute_speculative_verify_impl` still rests on the manual real-model comparison in the PR description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
…ath (pegainfer-project#604) `vllm:spec_decode_num_drafts/draft_tokens/accepted_tokens_total` read 0 because nothing on the Rust side populated `SchedulerStats.spec_decoding_stats`. Wire the DFlash acceptance counts through to the frontend: - `Qwen3Executor` accumulates cumulative `SpecDecodeCounters` in `execute_speculative_verify_impl` from each committed step's per-request `matched_draft_tokens` (accepted) and verify-span length (K proposed), plus per-position accepts. Executor-side so the publish path never round-trips the worker lane. - The scheduler republishes the cumulative counters on every `LoadSnapshot`. Cumulative (not per-step) keeps the coalescing watch channel correct: the watch's `send_replace` keeps only the latest value, so a per-step delta a reader missed would be lost outright. - `publish_scheduler_stats` diffs each snapshot against the last it forwarded to recover the per-interval deltas vLLM's monotonic counters are `inc_by`'d with — the rule pegainfer-project#603 sets for the prefix-cache counters — and attaches `spec_decoding_stats` only on intervals that actually drafted (avoids NaN acceptance-rate log spam). Dropping an idle interval's delta loses nothing: every counter moves only inside `observe_draft`. The per-position counts are a fixed `[u64; MAX_SPEC_TOKENS]` rather than a `Vec`, with `num_spec_tokens` as the used width. 16 is the widest `K` we ship (`K = verify_span - 1`; anchor-first `block_size` 16 gives 16, DSpark block7 gives 7). Fixed width keeps `LoadSnapshot` `Copy`, which matters beyond the allocation: `openinfer-dynamo-backend` (its own workspace) and the glm52 scheduler contract tests read the watch with `*guard`, and neither is built by CI's package matrix — a `Vec` in the snapshot breaks both silently. Tests: engine `spec_counters_*` (tally, prefix-shape invariant, K clamp), bridge `spec_delta_telescopes_to_cumulative` + `idle_intervals_omit_spec_decoding_stats`. Doc: subsystems/frontend/prometheus-metrics.md gains the spec-decode flow and the names carry the `_total` suffix `prometheus-client` appends at exposition — the registered name returns no data. Not covered here: pegainfer-project#604's third scope item, verifying the `/metrics` acceptance rate against the DFlash perf test, which needs a GPU and a draft checkpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
Follow-up to the spec-decode acceptance counters (pegainfer-project#604); the wiring itself already landed. `MAX_SPEC_TOKENS` fixes the width of `num_accepted_tokens_per_pos`, and at 16 it was sized as an exact fit — an anchor-first `block_size` 16 checkpoint needs exactly 16 positions. But `K` is checkpoint data (`verify_span - 1`, from the drafter's `block_size`) and nothing validates an upper bound at load, so an exact fit is the wrong shape: it leaves no headroom for a wider drafter and fails silently when one appears. Raised to 32, and `SpecDecodeCounters::new` — the only place the width is set — now warns when it clamps. A metrics limit must not refuse to load a model, but it must not truncate silently either. Totals stay exact; only the per-position curve loses its tail. The clamp test follows: `spec_counters_clamp_oversized_k` covers the wider-than- the-array drafter and asserts the totals survive it. It replaces a zero-K case that tested an unreachable state — `validate_for_target` enforces `block_size >= 2`, so `K = verify_span - 1` is at least 1 and the loader cannot construct a zero-K drafter. Also trims comments across `engine.rs`, `executor.rs`, `executor/spec.rs` and `bridge.rs` where they restated what the code already says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
… doc Removes what pegainfer-project#604 added to the metrics documentation, restoring both files to their pre-pegainfer-project#604 text: the TL;DR clause, the "Spec-decode counters ride the engine-gauge path" section, the split-out read-zero bullet (folded back into the `SchedulerStats::default()` line where it started), the acceptance-rate validation procedure, and the DSpark/EAGLE follow-up sentence — plus the matching detail in the index routing row. `prometheus-metrics.md` is now byte-identical to main, and the index row is unchanged from main. The `Measured cost is noise` block stays: it came from pegainfer-project#644 (GLM5.2 per-rank scheduler metrics), not from the spec-decode work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
…apshot grew Two fallouts from adding `spec_decode` to `LoadSnapshot`, both flagged on pegainfer-project#787. `openinfer-qwen35/src/scheduler.rs` publishes through an exhaustive struct literal, so the new required field broke `--features qwen35` with E0063. That package is outside CI's matrix, which is why it went unnoticed. Qwen3.5 has no draft path, so the field is `None`. `LoadSnapshot` also crossed Clippy's `large_types_passed_by_value` threshold — `[u64; 32]` of per-position accepts puts it at 328 bytes — and the sim E2E helper still took it by value, failing CPU Clippy. Takes `&LoadSnapshot` now; the watch channel still gets an owned copy. Verified: CPU Clippy (the exact CI package set, `-D warnings`) is clean, `cargo check -p openinfer-qwen35 --features qwen35` compiles, and all 13 `openinfer-sim --test frontend_e2e` tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
…K bound Addresses review items 3-5 on pegainfer-project#787. **A metrics module.** `LoadSnapshot`, `SpecDecodeCounters` and `MAX_SPEC_TOKENS` move to `openinfer-engine/src/metrics.rs`, re-exported from `engine` so every `openinfer_engine::engine::LoadSnapshot` / `openinfer_core::engine::*` import still resolves untouched. `engine.rs` goes 1068 -> 926 lines, below the 940 it sat at before this branch. The commentary is cut to the invariants: the checkpoint/current-workspace/CI narrative around `MAX_SPEC_TOKENS` and the idle-interval narration in `publish_scheduler_stats` said at length what the code says. **A rejected K, not a silently clamped one.** `SpecDecodeCounters::new` stored `min(K, 32)` in a field documented as the drafter's configured `K`. Totals stayed exact, but the metric advertised a `K` the drafter did not have and passed a truncated acceptance curve off as a complete one. It now returns `Err(SpecWidthUnsupported)`, and `load_dflash_draft_model` propagates that before any executor state moves. Nothing we ship is affected: DFlash-b16 needs 15 and dspark block7 needs 7, against a bound of 32. `spec_decode_delta` drops its now-redundant `min(K, MAX_SPEC_TOKENS)` clamp in favour of `take(K)` on the iterator, which cannot panic on a bad width the way the slice could — the guard becomes unnecessary rather than merely unused. **Tests that carry their weight.** `spec_delta_telescopes_to_cumulative` was re-testing subtraction and never covered per-position deltas at all; the coalescing and per-position assertions it should have made are folded into the async publisher test, which now checks the first-publish delta, per-position widths, idle omission, and a two-verify-step gap arriving in one coalesced snapshot. The position-prefix tally test keeps its `[2, 1, 0]` assertion and loses the tail-zero/sum/monotonic restatements around it. The clamp test becomes a rejection test. Verified: CPU Clippy and Qwen3 CUDA Clippy over CI's exact package sets with `-D warnings`, `--features qwen35` Clippy, and the engine / frontend / qwen3 / core lib tests plus the sim `frontend_e2e` suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
The review's remaining follow-up on pegainfer-project#787: nothing asserted what the exposition layer actually serves. The bridge unit tests stop at `SchedulerStats`, so the `inc_by` accumulation, the label sets, and the `_total` suffix `prometheus-client` appends were all unverified — and the registered name scrapes empty, which is exactly the kind of mistake a unit test cannot see. Runs against the simulated engine, so it needs no GPU and no draft checkpoint and lands in the existing `simulated-frontend-e2e` job. It publishes cumulative `SpecDecodeCounters` on a `LoadSnapshot` and scrapes `GET /metrics`, asserting: - the first interval's counters, - that after two verify steps arrive coalesced in one snapshot the counters read back *exactly* the scheduler's cumulative — the totals -> delta -> `inc_by` round trip is only correct if nothing is double-counted or dropped, and this equality is the one place that shows it, - per-position series for `position` 0 and 1 with nothing past `K = 2`, so the fixed `MAX_SPEC_TOKENS` array width cannot leak into the exposition, - engine 1, which never drafted, reading zero — which is also what pins each delta to a single engine. Mutation-checked: publishing cumulative instead of deltas, and dropping the per-position `take(K)`, each fail it. `wait_for_metrics` now delegates to a `wait_for_labeled_metrics` that matches arbitrary extra labels, since the per-position family is keyed by `position` on top of engine and model; existing call sites are unchanged. The new server gets its own `model_name` because the Prometheus registry is process-wide and concurrent tests would otherwise read each other's counters. Not covered: executor -> scheduler. The simulated engine has no Qwen3 executor, so `observe_draft` firing correctly from `execute_speculative_verify_impl` still rests on the manual real-model comparison in the PR description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
dfcb7f2 to
da765f9
Compare
…apshot grew Two fallouts from adding `spec_decode` to `LoadSnapshot`, both flagged on pegainfer-project#787. `openinfer-qwen35/src/scheduler.rs` publishes through an exhaustive struct literal, so the new required field broke `--features qwen35` with E0063. That package is outside CI's matrix, which is why it went unnoticed. Qwen3.5 has no draft path, so the field is `None`. `LoadSnapshot` also crossed Clippy's `large_types_passed_by_value` threshold — `[u64; 32]` of per-position accepts puts it at 328 bytes — and the sim E2E helper still took it by value, failing CPU Clippy. Takes `&LoadSnapshot` now; the watch channel still gets an owned copy. Verified: CPU Clippy (the exact CI package set, `-D warnings`) is clean, `cargo check -p openinfer-qwen35 --features qwen35` compiles, and all 13 `openinfer-sim --test frontend_e2e` tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
…K bound Addresses review items 3-5 on pegainfer-project#787. **A metrics module.** `LoadSnapshot`, `SpecDecodeCounters` and `MAX_SPEC_TOKENS` move to `openinfer-engine/src/metrics.rs`, re-exported from `engine` so every `openinfer_engine::engine::LoadSnapshot` / `openinfer_core::engine::*` import still resolves untouched. `engine.rs` goes 1068 -> 926 lines, below the 940 it sat at before this branch. The commentary is cut to the invariants: the checkpoint/current-workspace/CI narrative around `MAX_SPEC_TOKENS` and the idle-interval narration in `publish_scheduler_stats` said at length what the code says. **A rejected K, not a silently clamped one.** `SpecDecodeCounters::new` stored `min(K, 32)` in a field documented as the drafter's configured `K`. Totals stayed exact, but the metric advertised a `K` the drafter did not have and passed a truncated acceptance curve off as a complete one. It now returns `Err(SpecWidthUnsupported)`, and `load_dflash_draft_model` propagates that before any executor state moves. Nothing we ship is affected: DFlash-b16 needs 15 and dspark block7 needs 7, against a bound of 32. `spec_decode_delta` drops its now-redundant `min(K, MAX_SPEC_TOKENS)` clamp in favour of `take(K)` on the iterator, which cannot panic on a bad width the way the slice could — the guard becomes unnecessary rather than merely unused. **Tests that carry their weight.** `spec_delta_telescopes_to_cumulative` was re-testing subtraction and never covered per-position deltas at all; the coalescing and per-position assertions it should have made are folded into the async publisher test, which now checks the first-publish delta, per-position widths, idle omission, and a two-verify-step gap arriving in one coalesced snapshot. The position-prefix tally test keeps its `[2, 1, 0]` assertion and loses the tail-zero/sum/monotonic restatements around it. The clamp test becomes a rejection test. Verified: CPU Clippy and Qwen3 CUDA Clippy over CI's exact package sets with `-D warnings`, `--features qwen35` Clippy, and the engine / frontend / qwen3 / core lib tests plus the sim `frontend_e2e` suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
The review's remaining follow-up on pegainfer-project#787: nothing asserted what the exposition layer actually serves. The bridge unit tests stop at `SchedulerStats`, so the `inc_by` accumulation, the label sets, and the `_total` suffix `prometheus-client` appends were all unverified — and the registered name scrapes empty, which is exactly the kind of mistake a unit test cannot see. Runs against the simulated engine, so it needs no GPU and no draft checkpoint and lands in the existing `simulated-frontend-e2e` job. It publishes cumulative `SpecDecodeCounters` on a `LoadSnapshot` and scrapes `GET /metrics`, asserting: - the first interval's counters, - that after two verify steps arrive coalesced in one snapshot the counters read back *exactly* the scheduler's cumulative — the totals -> delta -> `inc_by` round trip is only correct if nothing is double-counted or dropped, and this equality is the one place that shows it, - per-position series for `position` 0 and 1 with nothing past `K = 2`, so the fixed `MAX_SPEC_TOKENS` array width cannot leak into the exposition, - engine 1, which never drafted, reading zero — which is also what pins each delta to a single engine. Mutation-checked: publishing cumulative instead of deltas, and dropping the per-position `take(K)`, each fail it. `wait_for_metrics` now delegates to a `wait_for_labeled_metrics` that matches arbitrary extra labels, since the per-position family is keyed by `position` on top of engine and model; existing call sites are unchanged. The new server gets its own `model_name` because the Prometheus registry is process-wide and concurrent tests would otherwise read each other's counters. Not covered: executor -> scheduler. The simulated engine has no Qwen3 executor, so `observe_draft` firing correctly from `execute_speculative_verify_impl` still rests on the manual real-model comparison in the PR description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <scatyf3@users.noreply.github.com>
da765f9 to
36d8205
Compare
|
Hi, I already fix the PR you mentioned
|
Description
Fixes #604 , add acceptance length to
/metricsvia snapshot.Type of Change
New feature (non-breaking change which adds functionality)
SpecDecodeCounters
First, we maintain the
SpecDecodeCounters, where this counter is mirror from vllm vllm/rust/src/engine-core-client/src/protocol/stats.rs at 8e61b646e2d157f9b93451fa048f9c8530c8a67b · vllm-project/vllm · GitHubHowever, we use fix length array for num_accepted_tokens_per_pos instead of vector, this is because
LoadSnapshotneed copy trait. Thus we hardcode a maxMAX_SPEC_TOKENS = 32, which is hard to exceed it in real practice.we set up an observe draft function to record sd status after verify
we test counter's correctness via
spec_counters_observe_draft_tallies_positionsandspec_counters_clamp_oversized_kconvert and publish sd status via frontend bridge
we Carry
SpecDecodeCounterson theLoadSnapshotand fillspec_decoding_statsin the bridge.we calculate delta in frontend bridge, convert our
SpecDecodeCountersto vllm frontend formatSpecDecodingStatsand publish them:here, correctness is checked by
spec_delta_telescopes_to_cumulativee2e test
To finalize our counter's correctness test, we use the measure metric from
docs/models/qwen3/dspark-integration.mdas reference. where they has detailed data from acceptance length to per position histgram:document's mesurement protocol is: 5090 GPU / CUDA 13.1,target
Qwen3-4B,vllm-bench --temperature 0 --ignore-eos,draftdspark_qwen3_4b_block7vsdflash_qwen3_4b_block7,markov_rank=0, use chat(sharegpt) + poem(sonnet) + rand(random) + code(speed-bench), c1/c4/c8.I use A6000+CUDA 12.8 with default seed=0, other remains the same. The modified test script is in my branch (not in this pr)
tools/bench/run_spec_accept_sweep.shtools/bench/spec_accept_metrics.pyverify-log argsour result is shown in this table.
[5836, 3872, 2635, 1641, 1235, 967, 686, 4384][7223, 4894, 3180, 1997, 1314, 1023, 763, 3414]Due to original document do not report prompt subsampling seed and the difference from GPU, result is small difference but the trends remain same. The
/metricsreport and thedflash_lane.rsdebug log agree exactly —verify-logcompares rounds, accepted tokens, and the histogram bin-by-bin over the same server lifetime, and every bin is equal on both drafters. We can assert that our spec decoding counter implementation is correct.