Skip to content

fix(server): reseed RNG per row at batched-prefill first-token sample - #356

Merged
inureyes merged 1 commit into
mainfrom
fix/issue-347-batched-prefill-seed-determinism
Jun 18, 2026
Merged

inureyes merged 1 commit into
mainfrom
fix/issue-347-batched-prefill-seed-determinism

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

Fixes the "last-seed-wins" determinism bug in batched prefill so a seeded request's first token no longer depends on the seeds of the other rows it happens to be batched with. The change is a one-line reseed of the global MLX RNG at the exact first-token sample point, plus a real-model regression test.

Root cause

seed_rng_if_needed seeds the process-global MLX RNG (ffi::random_seed) and is called from begin_prefill. In run_padded_batched_prefill, every row in a cohort runs begin_prefill up front (one loop) before any row runs finish_prefill (a later per-row loop that samples the first token). The first-token sampler is fused_sample, which takes only (logits, temperature, top_k, top_p, min_p) and draws from the global RNG with no per-call key. So by the time row 0 samples, the global RNG holds the LAST cohort row's seed, and every seeded row's first token is governed by whichever seed was applied last ("last-seed-wins"). PR #346 (#332 cohort splitting) widened the exposure: a cold row that previously fell back to sequential prefill now runs batched inside a BatchedCold cohort and newly inherits this behavior.

Fix

Insert seed_rng_if_needed(&seq.sampling) in finish_prefill, immediately before the first-token sample_token_optimized call, so the global RNG is reseeded to THIS row's own seed at the exact point it samples. begin_prefill's existing seeding is intentionally left in place (it is harmless once overwritten here, and is still reached from the disaggregated handoff path). finish_prefill is the single per-row first-token sampling chokepoint for every prefill path (batched cohort, sequential, full, chunked-final), so this one insertion fixes both the pre-existing all-cold case and the #332 cohort-split case holistically rather than patching each path.

Greedy is a no-op

Greedy / temperature == 0 / top_k == 1 rows take the fused_sample argmax path, which consumes no RNG, so the reseed has no effect on them. The regression test pins this.

Scope: decode is out of scope (honest AC #1 re-framing)

This fix makes a seeded row's FIRST token independent of its siblings' seeds, and greedy output is fully identical regardless of batch composition. It does NOT make a seeded request's full multi-token output byte-identical between "run alone" (B=1) and "batched" (B>1). Two pre-existing, already-documented effects remain and are not this seed bug: (1) padded batched prefill is not bitwise-identical to single-sequence prefill on Metal (the #203 / #325 / #326 forward-pass jitter class), and (2) the batched fused DECODE path (execute_batched_decode -> batched_fused_sample) samples all rows in ONE fused_sample draw over [B, vocab] from the shared global RNG, so a seeded row's decode trajectory depends on batch composition (the documented batched-vs-B=1 jitter class noted in sampling.rs). Full per-sequence multi-token determinism across batch sizes would require threading a per-row RNG key into fused_sample (an FFI / MLX-side change), which is disproportionate for this priority:low bug and is left as a follow-up. AC #1 is therefore satisfied at the first-token / batched-prefill scope of this bug; the residual is the existing jitter class, not last-seed-wins.

Regression test (invariant choice)

src/server/batch/scheduler_seed_determinism_tests.rs (real qwen3-0.6b-4bit, #[ignore], soft-skips when the model dir is absent), mirroring the scheduler_cohort_parity_tests.rs harness. It deliberately does NOT compare a batched (B>1) row against a single-prefill (B=1) row, which would conflate the forward-pass jitter class. Instead it holds the cohort COMPOSITION constant (same prompts in the same positions, so the measured row's logits are byte-identical) and varies ONLY seed values, isolating the RNG variable. The measured row is always at index 0 (sampled first) and the seeded sibling at index 1 (seeded last), which is exactly where "last-seed-wins" would leak. Checks:

Test plan

  • cargo check --lib --tests --features metal,accelerate
  • cargo clippy --lib --tests --features metal,accelerate -- -D warnings
  • cargo test -p mlxcel --lib --release --features metal,accelerate batched_prefill_seed_determinism -- --ignored --nocapture (observed: sibling seeds [1,2,3] -> first token [4792, 4792, 4792]; own seeds [10..60] -> 4 distinct first tokens; greedy [334, 334, 334]; cohort-split all-cold == mixed == 334)
  • cargo fmt --check

Closes #347

A batched prefill cohort runs every row's begin_prefill (which seeds the global MLX RNG when the request carries a seed) up front, before any row reaches finish_prefill (which samples the first token). The fused first-token sampler draws from the process-global RNG with no per-call key, so the last seeded row's seed governed first-token sampling for the whole cohort ("last-seed-wins"): a seeded row's first token depended on its siblings' seeds rather than its own. PR #346 (#332 cohort splitting) widened this, since cold rows that previously fell back to sequential prefill now run inside a BatchedCold cohort.

Reseed the global RNG to the row's own seed inside finish_prefill, immediately before the first-token sample, so each row's first token depends only on its own seed. begin_prefill keeps its seeding (harmless once overwritten here, and still reached from the disaggregated handoff path). Greedy / temperature == 0 / top_k == 1 rows take the argmax path and consume no RNG, so the reseed is a no-op for them. finish_prefill is the single per-row first-token chokepoint for every prefill path, so this one insertion covers both the pre-existing all-cold case and the #332 cohort-split case.

The batched fused decode path samples all rows in one fused_sample draw over [B, vocab] from the shared global RNG and is left unchanged: a seeded row's multi-token decode output across batch sizes is the pre-existing, documented batched-vs-B=1 jitter class, and making it byte-identical would require threading a per-row RNG key into fused_sample (follow-up).

Add a real-model regression test (qwen3-0.6b-4bit, ignored, soft-skip when absent) that holds cohort composition constant and varies only seed values to isolate the RNG variable: a seeded row's first token is invariant to sibling seeds (the bug-discriminating check), depends on its own seed (positive control, non-vacuous), greedy is unaffected, and a cohort-split mixed window matches the all-cold window of the same cold composition.
@inureyes inureyes added type:bug Bug fixes, error corrections, or issue resolutions priority:low Low priority area:core mlxcel-core: MLX FFI, primitives, KV cache, layers area:inference Generation, sampling, decoding (incl. speculative, DRY) status:review Under review labels Jun 18, 2026
@inureyes

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

Reseed the global MLX RNG to each row's own seed at the batched-prefill first-token sample point, eliminating "last-seed-wins" for seeded sampling in a batched cohort (#347).

Findings

No CRITICAL, HIGH, or MEDIUM correctness findings. The one-line reseed and its regression test are correct as written; no changes were made by this review.

Verification

  • Reseed placement correct. seed_rng_if_needed(&seq.sampling) (scheduler.rs:3924) sits after the structured-output mask (3885-3909) and immediately before sample_token_optimized (3925). Nothing between consumes RNG: sample_token_optimized_core applies only deterministic token-bias and penalty graph ops before fused_sample, and the lone eval in the token-bias branch is an argmax, not a draw.
  • finish_prefill is the single first-token chokepoint for every classic prefill path: batched cohort (3418), full (3577), chunked first-chunk-terminal (3731), chunked continuation-final (3868). Sequential-cohort rows reuse the full/chunked finish sites. The speculative MTP/DFlash burst is a separate, pre-existing pipeline that bypasses finish_prefill, but it is structurally immune to last-seed-wins: a batched burst window only admits seed-identical rows (sampling_config_eq compares seed) and a B=1 burst is single-seed, so there is no distinct "last seed" to leak.
  • Regression test discriminates the bug without the f16 batched-vs-B=1 jitter confound. It holds cohort composition constant and varies only sibling seeds, measuring row 0 (sampled first) with the seeded sibling at row 1 (seeded last), which is exactly where last-seed-wins leaks. The positive control (own-seed variation yielding two or more distinct tokens) keeps the invariant non-vacuous. Two cold rows of different lengths do batch into one BatchedCold cohort on qwen3 (can_pad == true), so the test genuinely exercises the begin-all-then-finish-all ordering.
  • Greedy unaffected. seed_rng_if_needed is gated on seed.is_some() and the argmax path consumes no RNG; pinned by the greedy-invariance assertion.
  • AC chore: complete OSS release rollout #1 reframing is honest. First-token / batched-prefill determinism is achieved. The residual run-alone (B=1) vs batched (B>1) multi-token difference is the pre-existing forward-pass jitter class plus the batched-decode shared-RNG draw, both correctly identified and deferred to the FFI key change.
  • cargo check --lib --tests --features metal,accelerate passes. No placeholder code, conventions followed (test mirrors scheduler_cohort_parity_tests.rs), Closes #347 present.

Verdict: correct and complete for its stated scope.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Jun 18, 2026
@inureyes
inureyes merged commit 554196c into main Jun 18, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-347-batched-prefill-seed-determinism branch June 18, 2026 14:50
inureyes added a commit that referenced this pull request Jun 20, 2026
Audio I/O on the OpenAI /v1/audio/* surface (Whisper speech-to-text #371,
Kokoro-82M text-to-speech #374, endpoint plumbing #368), reasoning_content on
non-streaming chat (#359), pre-M5 decode +8-12% via a hardware-gated op cap
(#360), Turbo4Asym decode rerouted through dequant-SDPA (#369), batched
sampling fixes (#339, #340, #344, #356), and the Turbo KV advisor/docs aligned
to the measured sweep with ADR 0002 (#343, #376).

README: consolidate the v0.3.0 and v0.3.1 highlights under a single v0.3 entry
and add v0.3.2.
@inureyes inureyes added this to the 0.3 milestone Jun 21, 2026
@inureyes inureyes self-assigned this Aug 31, 2026
be-free-snowboy pushed a commit to freestylejs/snowbrain-mlxcel that referenced this pull request Oct 27, 2026
…lablup#356)

A batched prefill cohort runs every row's begin_prefill (which seeds the global MLX RNG when the request carries a seed) up front, before any row reaches finish_prefill (which samples the first token). The fused first-token sampler draws from the process-global RNG with no per-call key, so the last seeded row's seed governed first-token sampling for the whole cohort ("last-seed-wins"): a seeded row's first token depended on its siblings' seeds rather than its own. PR lablup#346 (lablup#332 cohort splitting) widened this, since cold rows that previously fell back to sequential prefill now run inside a BatchedCold cohort.

Reseed the global RNG to the row's own seed inside finish_prefill, immediately before the first-token sample, so each row's first token depends only on its own seed. begin_prefill keeps its seeding (harmless once overwritten here, and still reached from the disaggregated handoff path). Greedy / temperature == 0 / top_k == 1 rows take the argmax path and consume no RNG, so the reseed is a no-op for them. finish_prefill is the single per-row first-token chokepoint for every prefill path, so this one insertion covers both the pre-existing all-cold case and the lablup#332 cohort-split case.

The batched fused decode path samples all rows in one fused_sample draw over [B, vocab] from the shared global RNG and is left unchanged: a seeded row's multi-token decode output across batch sizes is the pre-existing, documented batched-vs-B=1 jitter class, and making it byte-identical would require threading a per-row RNG key into fused_sample (follow-up).

Add a real-model regression test (qwen3-0.6b-4bit, ignored, soft-skip when absent) that holds cohort composition constant and varies only seed values to isolate the RNG variable: a seeded row's first token is invariant to sibling seeds (the bug-discriminating check), depends on its own seed (positive control, non-vacuous), greedy is unaffected, and a cohort-split mixed window matches the all-cold window of the same cold composition.
be-free-snowboy pushed a commit to freestylejs/snowbrain-mlxcel that referenced this pull request Oct 27, 2026
Audio I/O on the OpenAI /v1/audio/* surface (Whisper speech-to-text lablup#371,
Kokoro-82M text-to-speech lablup#374, endpoint plumbing lablup#368), reasoning_content on
non-streaming chat (lablup#359), pre-M5 decode +8-12% via a hardware-gated op cap
(lablup#360), Turbo4Asym decode rerouted through dequant-SDPA (lablup#369), batched
sampling fixes (lablup#339, lablup#340, lablup#344, lablup#356), and the Turbo KV advisor/docs aligned
to the measured sweep with ADR 0002 (lablup#343, lablup#376).

README: consolidate the v0.3.0 and v0.3.1 highlights under a single v0.3 entry
and add v0.3.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core mlxcel-core: MLX FFI, primitives, KV cache, layers area:inference Generation, sampling, decoding (incl. speculative, DRY) priority:low Low priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(server): batched-prefill seed determinism (last-seed-wins for seeded sampling in a batched cohort)

1 participant