feat(kernels): verify-side target probs + chain rejection sampling primitives (#512)#534
feat(kernels): verify-side target probs + chain rejection sampling primitives (#512)#534n-WN wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f162433592
ℹ️ 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".
| /// kernel, not as a renorm, so a min_p target distribution is not | ||
| /// representable here yet — callers must keep such requests off the | ||
| /// speculative path. | ||
| pub fn gpu_verify_probs_into( |
There was a problem hiding this comment.
Re-export the speculative sampling entry points
These new primitives are declared pub inside the private sampling module, but openinfer-kernels/src/ops.rs only re-exports the older sampling APIs (gpu_sample_batch_into, etc.). In that setup model crates such as the upcoming Qwen verify-path integration cannot call either gpu_verify_probs_into or gpu_spec_accept_into; they are only reachable from this module’s tests. Please add them to the public ops re-export list (and core re-exports if needed) so the kernels-layer API added here is usable outside openinfer-kernels.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — they were pub only inside the private module, unreachable from model crates. Re-exported through ops in d0de3b1 (the upcoming verify-path slice consumes them from there).
|
Thanks for this — the approach is sound: reusing FlashInfer's renorm pipeline + Suggested changes1.
|
d0de3b1 to
e151669
Compare
|
Thanks for the careful review — all four points addressed, plus the statistical test you suggested. Rebased onto 1. File split. Moved 2. Duplication.
3. 4. 5. Statistical test. Added |
…view) Address @xiaguan's openinfer-project#534 review: - Move gpu_verify_probs_into + gpu_spec_accept_into (and their tests) into ops/spec_sampling.rs; sampling.rs 1336 -> 1060 lines, so the speculative code no longer drives the file over the soft size limit. - Extract shared C helper gather_softmax_renorm_flashinfer (gather + softmax + optional top-k/top-p renorm), used by both the verify path and the min_p sampling path; extract Rust prepare_sampling_params shared by the batched sampler and the verify path. min_p stays caller-side (sampler packs it, verify rejects it). - Expose onehot_draft as a bool parameter on gpu_spec_accept_into instead of hardcoding 1, so the slice-2 verify path can pass a real drafter's probs. - Guard vocab > 0 in gpu_spec_accept_into. - Add spec_accept_partial_acceptance_matches_the_rate_law: a 512-row Bernoulli(0.7) convergence test for the p in (0,1) acceptance law. Verified on H100: cargo test -p openinfer-kernels --lib (sampling + spec) passes 11/11.
e151669 to
92f9887
Compare
xiaguan
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the math checks out, the slice boundary is clean, and the corner-point tests are well designed. I also ran the suite locally on an RTX 5070 Ti (SM120): 11/11 pass, so the consumer-GPU side is covered too.
A few things I'd like addressed before merge:
-
Missing FFI guard on the two new
extern "C"entry points.gpu_chain_speculative_sampling_cudaandgpu_verify_probs_flashinfer_cudalackOPENINFER_FFI_GUARD_BEGIN/END, whilegpu_sample_batch_flashinfer_cudain the same file has it. The guard exists precisely because FlashInfer can throw from host-side dispatch, and a C++ exception crossing the FFI boundary aborts the process with no message (see 8028dfa). Note the sharedgather_softmax_renorm_flashinfercurrently runs guarded on the sampler path but unguarded on the verify path. -
Return a named struct instead of
(Vec<i32>, Vec<i32>). The PR body itself calls out that committing onacceptedwould emit wrong tokens — but the API returns two same-typed Vecs positionally, so one swapped destructuring compiles fine and silently corrupts output. Something likeSpecAcceptCounts { emitted, acceptance_telemetry }encodes the invariant in the type and the trap disappears. -
Seeded-request replay.
ChainSpeculativeSamplingfolds the row index into the philox subsequence, so a fixed-seed request's draw stream depends on batch composition — the exact trap the sampler comment in this file documents and works around with n_rows=1 calls. The doc says "same discipline as the batched sampler", but no such per-row path exists here. Please state explicitly that per-request seeded replay is not representable and seeded requests must stay off the speculative path (and make sure the slice-2 gate enforces it, like the min_pensure!already does). -
Per-call
alloc_zerosfor the counters. This function will run every verify step in slice 2; the codebase convention is allocate-once scratch (BatchSamplingScratch/SampleScratch: "never reallocate per step"). Moving the two counter buffers into a scratch now avoids a signature change later.
Two smaller notes, fine to defer:
- The
accepted/emitteddistinction test assertsaccepted[1] >= 0, which is trivially true;assert_eq!(accepted[1], 1)holds with the same confidence as the other corner-point assertions. Theonehot_draft=falsepath also has no coverage yet. - For slice 2: please route the executor through
openinfer-sample(the policy crate every model routes through, see dd58508) rather than importing these primitives fromopeninfer_kernels::opsdirectly — a one-line re-export inopeninfer-samplekeeps the single-entry invariant. The dense one-hot materialization (memset of batch·K·vocab·4B per step) is also worth an A/B measurement in slice 2 before we rely on it.
Looking forward to the next revision!
…view) Address @xiaguan's openinfer-project#534 review: - Move gpu_verify_probs_into + gpu_spec_accept_into (and their tests) into ops/spec_sampling.rs; sampling.rs 1336 -> 1060 lines, so the speculative code no longer drives the file over the soft size limit. - Extract shared C helper gather_softmax_renorm_flashinfer (gather + softmax + optional top-k/top-p renorm), used by both the verify path and the min_p sampling path; extract Rust prepare_sampling_params shared by the batched sampler and the verify path. min_p stays caller-side (sampler packs it, verify rejects it). - Expose onehot_draft as a bool parameter on gpu_spec_accept_into instead of hardcoding 1, so the slice-2 verify path can pass a real drafter's probs. - Guard vocab > 0 in gpu_spec_accept_into. - Add spec_accept_partial_acceptance_matches_the_rate_law: a 512-row Bernoulli(0.7) convergence test for the p in (0,1) acceptance law. Verified on H100: cargo test -p openinfer-kernels --lib (sampling + spec) passes 11/11.
92f9887 to
f2f2c8d
Compare
|
All four addressed (plus both minor notes), rebased onto current
Minor notes: the telemetry corner assertion is now |
|
Thanks for the fast turnaround on the review — the revision reads well. One change of plan on scoping, though: let's not land the kernels slice standalone. Please carry this straight through slice 2 (executor wiring + gate relaxation), either by extending this PR or stacking the slice-2 PR on top of it immediately — whichever you prefer, but they merge together or back-to-back. Corner-point + rate-law unit tests are good evidence for the primitives, but #512 is a 0.2.0 milestone item and what we accept it on is end-to-end accuracy and performance, which only exist once the executor is wired. Concretely, the acceptance bar for the combined work: Accuracy (e2e, real model — Qwen3-4B + the DFlash drafter):
Performance (e2e):
Plumbing stays as agreed: executor goes through If any of this is under-specified, raise it here before writing code — happy to pin down the exact statistical test / bench protocol. |
|
Agreed — merging the primitives without the executor evidence would leave #512's actual claim untested. Plan: stacked slice-2 PR on top of this one, merged back-to-back (slice 2 already exists as a branch: executor wiring through the Two things done meanwhile:
Before writing the evidence harness, pinning down the two under-specified pieces — please confirm or adjust: Statistical equivalence (sampled spec-on vs spec-off):
Perf:
If the seed/prompt counts or the lm-eval task should be different (or you want a specific prompt list checked in for reproducibility), say the word — otherwise I'll build exactly the above and check the harness in next to the smoke. |
…imitives (openinfer-project#512) The two GPU primitives non-greedy speculative decoding needs, verified on H100 (CUDA 12.9), 3/3 deterministic tests: gpu_verify_probs_into — the batched sampling pipeline's gather + softmax + top-k/top-p renorm WITHOUT the terminal sampling kernel: the post-filter target distribution lands in the caller's buffer, filtered tokens as exact zeros. Distribution-equivalent to the fused sampling fast path (it filters at draw time, this filters then draws — the law over tokens is identical). min_p rows are rejected fail-loud: min_p is a sampling-time mask, not a renorm, so that target distribution is not representable here yet. gpu_spec_accept_into — wraps FlashInfer's ChainSpeculativeSampling: accept draft i with min(1, p_target/q_draft), first rejection resamples from relu(target - draft) renormalized, full acceptance emits the bonus token, tail is -1-filled. onehot_draft derives q = delta(x - draft) on device for a greedy/argmax proposer — the degenerate proposal under which acceptance is min(1, p_target(draft)) and rejection sampling stays distribution-exact, so DFlash's greedy drafts need no proposal-probability plumbing. One semantic trap pinned in doc + test: the kernel returns two counters with different meanings — emitted is the accepted-prefix length (the commit signal); accepted keeps counting hypothetical acceptances past the first rejection (FlashInfer's acceptance-rate telemetry). Committing on accepted would emit wrong tokens. Tests are corner-point deterministic, not statistical: a target that puts mass 1.0 on each draft forces full acceptance + bonus; a target with zero mass on the draft forces first-step rejection with a single-token residual, making the resample exact.
gpu_verify_probs_into / gpu_spec_accept_into were pub only inside the private sampling module; the ops re-export list is what model crates see.
…view) Address @xiaguan's openinfer-project#534 review: - Move gpu_verify_probs_into + gpu_spec_accept_into (and their tests) into ops/spec_sampling.rs; sampling.rs 1336 -> 1060 lines, so the speculative code no longer drives the file over the soft size limit. - Extract shared C helper gather_softmax_renorm_flashinfer (gather + softmax + optional top-k/top-p renorm), used by both the verify path and the min_p sampling path; extract Rust prepare_sampling_params shared by the batched sampler and the verify path. min_p stays caller-side (sampler packs it, verify rejects it). - Expose onehot_draft as a bool parameter on gpu_spec_accept_into instead of hardcoding 1, so the slice-2 verify path can pass a real drafter's probs. - Guard vocab > 0 in gpu_spec_accept_into. - Add spec_accept_partial_acceptance_matches_the_rate_law: a 512-row Bernoulli(0.7) convergence test for the p in (0,1) acceptance law. Verified on H100: cargo test -p openinfer-kernels --lib (sampling + spec) passes 11/11.
- Wrap gpu_verify_probs_flashinfer_cuda and gpu_chain_speculative_sampling_cuda
in OPENINFER_FFI_GUARD_BEGIN/END like the sibling sampler entry — FlashInfer
can throw from host-side dispatch, and an exception crossing the FFI boundary
aborts with no message. The Rust side now appends ffi_exception_message.
- Return SpecAcceptCounts { emitted, acceptance_telemetry } instead of a
positional (Vec<i32>, Vec<i32>): committing on the telemetry counter emits
wrong tokens, and two same-typed Vecs let a swapped destructuring compile.
- Document that per-request seeded replay is NOT representable in the chain
kernel (the row index folds into the philox subsequence, so a fixed seed's
draw stream depends on batch composition); seeded requests must stay off the
speculative path, enforced by the slice-2 gate like min_p already is.
- Move the accumulate-into counters into an allocate-once SpecAcceptScratch
(memset per call) — this runs every verify step in slice 2.
- Tighten the telemetry corner assertion to accepted[1] == 1 and add an
onehot_draft=false corner-point test (explicit proposal distribution:
certain accept at q=0.5/p=1.0, certain reject with residual resample).
Verified on H100: 12/12 (batch_sampling 7, spec_accept 3, verify_probs 2).
f2f2c8d to
aee0906
Compare
Slice 1 of #512 — the two GPU primitives non-greedy speculative decoding needs, as a self-contained kernels-layer PR. Executor wiring (verify-path integration, gate relaxation, the statistical equivalence gate) follows as the next slice on top.
gpu_verify_probs_into— the target distribution itselfThe batched sampling pipeline's gather + softmax + top-k/top-p renorm, stopped before the terminal sampling kernel: the post-filter probabilities land in the caller's
n_rows × vocabbuffer, filtered tokens as exact zeros (that matters for the rejection residual). Distribution-equivalent to the fused sampling fast path — it filters at draw time, this filters then draws; the law over tokens is identical, which is all rejection sampling's correctness needs.min_prows are rejected fail-loud: min_p is applied inside the sampling kernel, not as a renorm, so that target distribution is not representable here — such requests stay off the speculative path (gate keeps them out in the next slice).gpu_spec_accept_into— chain rejection samplingWraps the vendored FlashInfer
ChainSpeculativeSampling: accept draftiwithmin(1, p_target/q_draft); the first rejection resamples fromrelu(target − draft)renormalized and stops; full acceptance emits the bonus token from the target at position K; the tail is-1-filled — exactly the "longest accepted prefix + one model token" contractaccept_greedyalready has, so the executor seam stays shape-compatible.onehot_draft: DFlash proposes greedily, i.e. the proposal is the degenerateq(x) = δ(x − draft). Under that proposal, acceptance reduces tomin(1, p_target(draft))and the residual is the target with the draft token's mass removed — still distribution-exact. Deriving the one-hot rows on device means no draft-side proposal-probability plumbing is needed at all for the current proposer (realqsupport is already in the signature for a future sampled drafter).A semantic trap, pinned in doc + test
The kernel returns two counters with different meanings:
emittedis the accepted-prefix length — the commit signal;acceptedkeeps counting hypothetical acceptances past the first rejection (FlashInfer's acceptance-rate telemetry). Committing onacceptedwould emit wrong tokens; the wrapper doc says so and the test asserts the distinction.Verification (H100, CUDA 12.9) — 3/3, corner-point deterministic (not statistical)
verify_probs_renorm_matches_the_sampling_law: closed-form softmax of{2,1,0}matches to 5e-3;top_k=2renorm leaves the filtered token an exact 0.0 and renormalizes survivors to{0.7311, 0.2689}; rows sum to 1.verify_probs_rejects_min_p_rows: fail-loud path.spec_accept_full_acceptance_and_certain_rejection: a target with mass 1.0 on each draft forces full acceptance + bonus ([100, 200, 777], emitted=2); a target with zero mass on the draft forces first-step rejection where the residual is a single token, making the resample exact ([555, -1, -1], emitted=0).🤖 Generated with Claude Code