Skip to content

feat(metrics): spec-decode acceptance counters for the DFlash draft path - #787

Open
scatyf3 wants to merge 6 commits into
pegainfer-project:mainfrom
scatyf3:feat/spec-decode-acceptance-metrics
Open

feat(metrics): spec-decode acceptance counters for the DFlash draft path#787
scatyf3 wants to merge 6 commits into
pegainfer-project:mainfrom
scatyf3:feat/spec-decode-acceptance-metrics

Conversation

@scatyf3

@scatyf3 scatyf3 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #604 , add acceptance length to /metrics via 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 · GitHub

However, we use fix length array for num_accepted_tokens_per_pos instead of vector, this is because LoadSnapshot need copy trait. Thus we hardcode a max MAX_SPEC_TOKENS = 32, which is hard to exceed it in real practice.

#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
pub struct SpecDecodeCounters {
    /// Configured max draft tokens proposed per verify step (`K`), and the
    /// number of leading [`Self::num_accepted_tokens_per_pos`] entries that
    /// carry meaning. Never exceeds [`MAX_SPEC_TOKENS`].
    pub num_spec_tokens: u64,
    /// Draft proposals — one per request per verify step.
    pub num_drafts: u64,
    /// Draft tokens proposed for verification (pre-acceptance) in total.
    pub num_draft_tokens: u64,
    /// Draft tokens accepted in total, excluding the bonus token.
    pub num_accepted_tokens: u64,
    /// Accepted-token count indexed by draft position: `[i]` is how often the
    /// `i`-th draft was accepted. Only `[..num_spec_tokens]` is meaningful; the
    /// tail stays zero so the array width can be a constant.
    pub num_accepted_tokens_per_pos: [u64; MAX_SPEC_TOKENS],
}

we set up an observe draft function to record sd status after verify

// openinfer-engine/src/engine.rs
impl SpecDecodeCounters {
    /// Fold one request's verify outcome into the totals
    pub fn observe_draft(&mut self, num_draft_tokens: usize, num_accepted: usize) {
        self.num_drafts += 1;
        self.num_draft_tokens += num_draft_tokens as u64;
        self.num_accepted_tokens += num_accepted as u64;
        let tallied = num_accepted.min(self.num_spec_tokens as usize);
        for slot in self.num_accepted_tokens_per_pos.iter_mut().take(tallied) {
            *slot += 1;
        }
    }
}

// openinfer-qwen3/src/executor/spec.rs
impl Qwen3Executor {
	pub(super) fn execute_speculative_verify_impl(
	        &mut self,
        plan: VerifyPlan<'_>,
    ) -> Result<VerifyResult> {
	    // verify
	    // record
        if let Some(counters) = self.spec_decode_counters.as_mut() {
            for (req, req_result) in plan.requests.iter().zip(&result.requests) {
                let num_draft_tokens = req.as_slice().len().saturating_sub(1);
                counters.observe_draft(num_draft_tokens, req_result.matched_draft_tokens);
            }
        }
}

we test counter's correctness via spec_counters_observe_draft_tallies_positions and spec_counters_clamp_oversized_k

convert and publish sd status via frontend bridge

we Carry SpecDecodeCounters on the LoadSnapshot and fill spec_decoding_stats in the bridge.

#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
pub struct LoadSnapshot {
    pub kv_used_blocks: u64,
    pub kv_total_blocks: u64,
    /// Requests currently occupying a decode/prefill slot.
    pub num_running_reqs: u64,
    /// 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>,
}

we calculate delta in frontend bridge, convert our SpecDecodeCounters to vllm frontend format SpecDecodingStats and publish them:

fn spec_decode_delta(last: &SpecDecodeCounters, cur: &SpecDecodeCounters) -> SpecDecodingStats {
    let width = (cur.num_spec_tokens as usize).min(MAX_SPEC_TOKENS);
    let num_accepted_tokens_per_pos = cur.num_accepted_tokens_per_pos[..width]
        .iter()
        .zip(&last.num_accepted_tokens_per_pos)
        .map(|(cur_pos, last_pos)| cur_pos.saturating_sub(*last_pos))
        .collect();
    SpecDecodingStats {
        num_spec_tokens: cur.num_spec_tokens,
        num_drafts: cur.num_drafts.saturating_sub(last.num_drafts),
        num_draft_tokens: cur.num_draft_tokens.saturating_sub(last.num_draft_tokens),
        num_accepted_tokens: cur
            .num_accepted_tokens
            .saturating_sub(last.num_accepted_tokens),
        num_accepted_tokens_per_pos,
    }
}

async fn publish_scheduler_stats(
    engine_index: u32,
    mut load_rx: watch::Receiver<LoadSnapshot>,
    output_tx: mpsc::UnboundedSender<EngineCoreOutputs>,
    shutdown: CancellationToken,
) -> Result<()> {
    let mut last_spec = SpecDecodeCounters::default();
    loop {
        let spec_decoding_stats = if let Some(cur) = &snapshot.spec_decode {
            let delta = spec_decode_delta(&last_spec, cur);
            last_spec = *cur;
            // Intervals with no verify step are the common case — prefill,
            // idle, and plain decode all publish without drafting. Reporting
            // one would divide by a zero `num_drafts` in the frontend's
            // acceptance-rate log, so leave the field `None` there, as vLLM's
            // own scheduler does. Nothing is lost by dropping it: every counter
            // moves only inside `observe_draft`, so a zero `num_drafts` delta
            // means nothing else moved either.
            (delta.num_drafts > 0).then_some(delta)
        } else {
            // No drafter (or one that just went away): forget the totals so a
            // drafter loaded later starts its diff from zero instead of being
            // saturated away against a stale high-water mark.
            last_spec = SpecDecodeCounters::default();
            None
        };
		let stats = SchedulerStats {
            num_running_reqs: snapshot.num_running_reqs,
            num_waiting_reqs: snapshot.num_waiting_reqs,
            kv_cache_usage: if snapshot.kv_total_blocks == 0 {
                0.0
            } else {
                snapshot.kv_used_blocks as f64 / snapshot.kv_total_blocks as f64
            },
            spec_decoding_stats,
            ..SchedulerStats::default()
        };
    }

here, correctness is checked by spec_delta_telescopes_to_cumulative

e2e test

To finalize our counter's correctness test, we use the measure metric from docs/models/qwen3/dspark-integration.md as reference. where they has detailed data from acceptance length to per position histgram:

config	rounds	mean accepted draft	zero-accept	full-7	hist 0..7
DSpark	19,294	2.52	29.2%	17.4%	[5636, 3942, 2394, 1549, 1042, 742, 639, 3350]
DFlash	21,214	2.30	32.2%	13.9%	[6838, 4340, 2685, 1648, 1071, 962, 731, 2939]

document's mesurement protocol is: 5090 GPU / CUDA 13.1,target Qwen3-4Bvllm-bench --temperature 0 --ignore-eos,draft dspark_qwen3_4b_block7 vs dflash_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)

  1. tools/bench/run_spec_accept_sweep.sh
  2. tools/bench/spec_accept_metrics.py verify-log args

our result is shown in this table.

config rounds mean accepted draft zero-accept full-7 hist 0..7
DSpark 21,256 2.76 27.5% 20.6% [5836, 3872, 2635, 1641, 1235, 967, 686, 4384]
DFlash 23,808 2.36 30.3% 14.3% [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 /metrics report and the dflash_lane.rs debug log agree exactlyverify-log compares 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread openinfer-engine/src/engine.rs Outdated
/// 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>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 xiaguan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Please fix the existing automated P1 in the Qwen3.5 scheduler. openinfer-qwen35/src/scheduler.rs:838 still constructs LoadSnapshot without the new spec_decode field. 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.

  2. Please restore green CI. CPU Clippy currently fails because LoadSnapshot grew to 328 bytes and openinfer-sim/tests/frontend_e2e.rs:185 passes it by value (large_types_passed_by_value). The DCO sign-off check is also failing.

  3. Please give the metrics types a module boundary and reduce the commentary. I do not think the placement is the contributor's fault: LoadSnapshot already lived in engine.rs, and there was no existing engine metrics module to extend. However, this PR adds about 128 lines there and takes engine.rs from 940 to 1068 lines. Please introduce an openinfer-engine/src/metrics.rs (with the necessary re-exports) for LoadSnapshot, 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 around MAX_SPEC_TOKENS and the repeated idle-interval narration obscure a fairly small contract.

  4. 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_cumulative mostly 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 /metrics E2E 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.

  5. Do not silently change the configured K in the public snapshot. SpecDecodeCounters::new stores min(K, 32) in a field documented as the configured num_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.

@scatyf3

scatyf3 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for your review~ I'll fix my PR by the end of this week at the latest

scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
…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>
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
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>
@scatyf3
scatyf3 force-pushed the feat/spec-decode-acceptance-metrics branch 2 times, most recently from 4896d83 to dfcb7f2 Compare August 4, 2026 06:16
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
…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>
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
…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>
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
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>
scatyf3 and others added 6 commits August 4, 2026 12:55
…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>
@scatyf3
scatyf3 force-pushed the feat/spec-decode-acceptance-metrics branch from dfcb7f2 to da765f9 Compare August 4, 2026 17:28
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
…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>
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
…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>
scatyf3 added a commit to scatyf3/pegainfer that referenced this pull request Aug 4, 2026
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>
@scatyf3
scatyf3 force-pushed the feat/spec-decode-acceptance-metrics branch from da765f9 to 36d8205 Compare August 4, 2026 20:50
@scatyf3

scatyf3 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Hi, I already fix the PR you mentioned

  1. misc:
    1. add spec_decode to qwen3.5 path
    2. fix cpu clippy by pass loadsnapshot as reference instead of pass it directly
    3. fix the DCO sign-off check
  2. reconstruct:
    move LoadSnapshot and SpecDecodeCounters to openinfer-engine/src/metrics.rs
  3. shorten the comments t
  4. test
    1. remove  extra tail-zero/sum/monotonic assertions
    2. add cpu sim e2e test
  5. remove silent change for K = min(K, 32), now engine will reject spec decoding request when K>32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

metrics: spec-decode acceptance counters for the DFlash draft path

2 participants