Conversation
Hert4
marked this pull request as draft
September 22, 2026 11:33
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Closes #267. Adds
DiffusionSamplerResource, and moves OmniVoice onto it.The autoregressive
Samplercannot express what a diffusion step needs: it draws one token per request and returns tokens only, while a diffusion step scores every unrevealed position at once and the caller needs the log-probabilities back to decide which of them to keep. It also bakesAPPLY_PENALTYas a Triton constexpr at capture, around seen-token buffers and a repetition penalty that have no meaning over a canvas rewritten every step. Hence a separate resource rather than a mode, which is what we agreed on the issue.Contract.
sample(request_ids, c_logits, u_logits, seq_lens, iterations)takes[rows, positions, vocab]packed across requests and returns(tokens, logprobs)for every position. Guidance scale and temperature are per-request, broadcast to per-position vectors, so a batch mixing them is one path rather than a Python loop. Which positions are still masked, how many to reveal, the confidence ranking and the write all stay in the model: they depend on a schedule and a layer penalty that belong to the decoder.Three things worth flagging in the review.
Scoring moved from
postprocesstoforward_batched, because that is where the batch still exists.predict_tokens_with_scoringandfilter_top_kare gone fromunmask.py. Keeping them would have left the parity tiers testing a copy of the maths that production no longer runs; all three now go through the resource.An item at
guidance_scale == 0no longer builds an unconditional document at all.does_cfgexisted but nothing called it, so every request paid for logits scoring discards.How was it tested?
ruff check .passes. On a H200 with the checkpoint on a PVC, the whole file including the three GPU parity tiers:Tier A is still token-exact against the reference through the new path, which is the part that says the port is unchanged.
The live server too, since unit tests never exercise the engine building the resource from the spec, injecting it into
engine_inputs.resources, or the conductor callingget_request_resource_configs:This is not a performance change, and I measured it rather than assuming. Per-exec medians of the backbone node from
--log-stats, 154 requests each side, same pod, same weights, back to back:The Python side of a step is 1.8 ms out of 33, about 5%, and batching the scoring does not move it. I had expected otherwise. A closed-loop throughput sweep agreed that the two are indistinguishable, but that harness had a counting bug so I am not quoting its numbers.
Not covered: the mixed-guidance path in
_unconditional_half, where one step carries both a CFG and a non-CFG request. Unit tests cover the layout and the slicing, but no live request has gone through it; the server runs above are each uniform. Flagging rather than letting the test names imply more.Checklist
ruff check .passesOne correction to something I wrote in #249: "the gap to upstream's ~40x is almost all CUDA graphs". That was wrong. Their README says CUDA graphs are for batch 1 and that "at batch >= 4 the plain FlashInfer path is already the fastest configuration", and their own table puts graphs at 1.17x (RTF 0.0430 to 0.0367). The 40x comes from batch, not from graphs. I measured this on the hardware and will post the numbers on #267, since they change what is worth doing next.
🤖 Generated with Claude Code