feat(metrics): spec-decode acceptance counters for the DFlash draft path - #787
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>
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
…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>
|
Hi, I already fix the PR you mentioned
|
|
LGTM — all review items are addressed and CI is green. Please rebase onto main and we're good to go. |
|
sure, I'll do that |
36d8205 to
30081f1
Compare
…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: - SpecDecodeCounters lives in its own metrics module with a hard K bound (MAX_SPEC_TOKENS = 32, warn-and-clamp on overflow). Qwen3Executor accumulates cumulative counters 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. - publish_scheduler_stats diffs each snapshot against the last it forwarded to recover per-step deltas for vLLM's monotonic counters, and attaches spec_decoding_stats only on intervals that actually drafted (avoids NaN acceptance-rate log spam). The per-position vector is sized to the drafter's K (verify_span - 1) so the Python _num_accepted_tokens_per_pos logger never index-errors. Keeps Qwen3.5 and the sim frontend building after LoadSnapshot grew. Tests: engine spec_counters_* (tally + overflow clamp), bridge spec_delta_telescopes_to_cumulative + idle_intervals_omit_spec_decoding_stats, and a sim frontend e2e that scrapes /metrics for the counters end to end. Squashed from 6 commits (pegainfer-project#604 base, MAX_SPEC_TOKENS=32, metrics module refactor, qwen35/sim build fix, docs, e2e scrape test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: scatyf3 <13589360+scatyf3@user.noreply.gitee.com>
30081f1 to
2f985b8
Compare
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.