Repository navigation
Return top-p/top-k sampling mask/nucleas - #27408
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ByronHsu
left a comment
There was a problem hiding this comment.
Thanks for adding this. The Python/SRT side looks broadly safe to me, and the feature itself seems useful.
A few requests before this moves forward:
-
Can we rename the PR/title to avoid making this sound DeepSeek V3.2-specific? From the implementation,
return_sampling_maskis generic sampling metadata. It is not inherently tied to one model. -
Please add an end-to-end test in the usual server-launch + sample-request style. The smoke script is useful for manual validation, but I’d like a checked-in test that launches a small server/model, sends a
/generaterequest withreturn_sampling_mask=true, and verifies. -
Please revert the
sgl-model-gatewayraw-JSON/extra-body plumbing in this PR. Switching/generatefromJson<GenerateRequest>toJson<Value>and threadingroute_generate_with_extrathrough the router stack feels too hacky for a known first-class field.For Miles, let’s make this change directly in
radixark/sgl-router-for-milesinstead. That is the router we will use going forward, and it has the protocol structs plus router code in one repo, so we can addreturn_sampling_maskdirectly toGenerateRequestand keep the handler typed. -
Please clean up the new
scripts/additions. -
After this lands on
main, please merge/cherry-pick it intosglang-miles.
Thank you for your review! Your suggestions make sense to me, this PR was more so intended to be a proof-of-concept prior to further cleanup. I will work on doing the suggestions over the coming days. |
09b21c1 to
1102107
Compare
|
Hi @ByronHsu, I believe I have addressed the main points of your feedback and have opened a much cleaner routing PR here radixark/sgl-router-for-miles#7 to use serde. I added an e2e server test script as well in place of the old smoke test script. Please let me know if you have additional feedback that I can address -- thanks! |
|
lgtm. please turn the pr to "ready for review" and make the ci pass. current lint is failing too |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
627102b to
34ff1b3
Compare
|
/tag-and-rerun-ci |
b4fc262 to
e175d71
Compare
Read req.return_sampling_mask directly in session_controller instead of getattr, give the streaming accumulator field a default, and set the flag in the session token-share unit test mock.
e175d71 to
cf117ee
Compare
CI StatesLatest PR Test (Base): ✅ Run #29158553156 |
# Conflicts: # python/sglang/srt/disaggregation/prefill.py
Main's sgl-project#27408 added an unconditional req.return_sampling_mask read in MetadataBuffers.set_buf; the SimpleNamespace req stub predates it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… sort The top-p nucleus capture ran a full-vocab `probs.sort` (~150k) AND a per-row `.item()` GPU->CPU sync on every decode step (both in `get_top_p_token_ids_from_probs` and `renorm_logprob_over_top_p`, which the sampler calls per token). On the Qwen3.6 codeforces S2 top-p run this slowed decode enough to push trajectories into the 1800s trajectory-timeout -> tokens_per_gpu_per_sec down, aborted-trajectory length shorter, empty_trajectory_ratio ~15% (baseline ~0%). See notes/training_logs/ioi/2026-07-16_qwen3_6_stage2_topp_mask_replay_observations.md. Port the structure of upstream sglang PR sgl-project#27408 (Return top-p/top-k sampling mask/nucleus) against our v0.5.10 tree: - `_top_p_keep_mask_bounded`: gather only the top-K (`torch.topk`, MASK_TOPK_DEFAULT=128) instead of a full-vocab sort, since the kept nucleus is a tiny prefix (measured width p99 ~11 / max ~35). K is DECOUPLED from the sampling top_k on purpose: no recipe / `--rollout-top-k` change, no distribution change -- it only bounds capture width. Silent-truncation guard: any row whose nucleus reaches the K-th column is recomputed with the full sort and spliced in (warns), so correctness never depends on K. - `get_top_p_token_ids_from_probs`: also drop the per-row `.item()` sync -- vectorized `nonzero` + a single batched `.cpu()`, split back into per-row lists on host. - `renorm_logprob_over_top_p`: same sort removed (same per-decode-step hot path); build the vocab-order keep-mask by setting only kept positions (nonzero gather) since the bounded path pads unused columns with index 0. No slime-side / launcher / wire-format change (still top_p_token_ids + offsets). Tests: test/srt/test_top_p_replay_logprob.py +2 (topk-vs-full-sort equivalence, wide-nucleus fallback); all 9 pass. New extractor verified byte-exact (order+values) vs the old one across seeds. CPU-validated only; on-GPU perf recovery pending.
Implement keep-sampling-mask / top-p & top-k replay runtime support through request parsing, sampling, scheduling, detokenization, disaggregation metadata, and the model gateway.
Motivation
Modifications
Accuracy Tests
test/registered/sampling/test_sampling_mask.pywas added to check the following:return_sampling_mask=Trueare identical to those returned whenreturn_sampling_mask=False, the default path.return_logprob = Trueandtop_logprobs_num = TOP_Kwith temperature 1, which returns the unnormalized logprobs of the TOP_K most probable tokens. From here, we can reconstruct the sampling mask (modulo some possible boundary off-by-ones) and compute the renormalized logprobs offline.Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ⏳ Run #29291142253
Latest PR Test (Extra): ❌ Run #29291141839