Repository navigation
Conversation
e6ba953 to
9f68c61
Compare
9f68c61 to
ad27242
Compare
|
good. Seems like the rebase include args resolve logic on main now |
|
[resolved] cc cmt Follow-up on the sampling-support replay e2e, now that [P3] The current e2e only catches large normalization errors. Proposal. Megatron still rejects Results on
Caveat on "exactly 0". It holds through rounding, not by construction. SGLang's replayed log-prob is
I'd keep the Megatron test as a smoke run until Megatron supports true-on-policy, since it is the only e2e over the Megatron replay path. Optional naming nit:
|
…#3) * test(e2e): check sampling-support replay exactly under true-on-policy The sampling-support replay e2e compares trainer and rollout log-probs with a 0.03 tolerance on one rollout. Scoring over the full vocabulary instead of the replayed support shifts the mean by only about 0.037 on that data, so a milder top-p or a support wrong on some positions passes. Under true-on-policy the actor's support-normalized log-probs equal the rollout's exactly, and --ci-test asserts log_probs == rollout_log_probs on every rollout. Megatron still rejects --true-on-policy-mode, so the new test runs FSDP: Qwen3-0.6B, top-p 0.8 / top-k 32, 5 rollouts on two GPUs. It uses gsm8k with 1024 response tokens because dapo at 128 tokens truncates every response, which zeroes advantages and leaves the weights nearly unchanged across steps. The Megatron test stays as a smoke run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): drop tolerance-based replay test and gate KL and grad norm The Megatron sampling-support replay test checked log-probs only within 0.03 over one rollout. The true-on-policy FSDP test checks them exactly on every rollout, so the Megatron test is removed. The FSDP test now also declares history gates for train_rollout_kl, ppo_kl and grad_norm. ppo_kl compares the training forward with forward-only scoring, so it covers the replay mask on the loss path, which the --ci-test log-prob assertion does not reach. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): drop the grad_norm gate from the true-on-policy replay test grad_norm is not bit-reproducible across runs of this test and is 0 on steps where every prompt's samples share one reward, so its band carries little signal here. The exact log-prob and KL gates stay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): note that exact replay log-probs hold through bf16 rounding SGLang's fp32 log(p / sum_S p) and the trainer's masked bf16 log_softmax can differ by one bf16 ulp on a small fraction of tokens. Record the measured rate and why it rarely trips --ci-test next to the check, so a nonzero train_rollout_logprob_abs_diff is not mistaken for a regression. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude review always |
|
✅ |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large, complex change to RL training correctness (rollout capture, session propagation, and dual-backend actor scoring across FSDP and Megatron), a human look would still be worthwhile even though this automated pass found nothing and a codeowner has already approved.
What was reviewed: the updated diff after the commits addressing the earlier inline feedback (bounded top-p/top-k contract validation, router forwarding fix, doc clarifications, e2e naming); the sampling-support capture/threading path through session state, config, and request-args; and the actor-side masked log-prob reconstruction in both backends. Checked whether replay-enabling flags are validated against router mode, and whether a custom Megatron loss calling get_rollout_sampling_masks would crash — both were investigated and ruled out as not blocking for this change.
Extended reasoning...
This PR adds a new sampling-support replay feature spanning rollout capture (SGLang mask extraction), session/wire serialization, and actor loss computation in both FSDP and Megatron backends across 46 files; no auth/crypto/injection surface, but it does touch core training-correctness logic owned by multiple CODEOWNERS. Since the prior review, 14 commits addressed a codeowner's (guapisolo) earlier inline objections and that same codeowner has since approved, and this run found no new bugs, so approval risk is lower; however the size and centrality of the change to RL correctness still argue for a human pass rather than auto-approval.
Summary
Add end-to-end bounded sampling-support replay for top-p/top-k rollout training.
SGLang samples each token after temperature and support filters. A full-vocabulary actor softmax is therefore not the same policy as the bounded rollout distribution. Sampling-support replay records the realized support
S_tand defines both behavior and actor scores on that fixed support:log mu(a_t | s_t, S_t).log pi_theta(a_t | s_t, S_t)by masking and renormalizing current actor logits.S_tremains fixed during optimization; Miles does not recompute the current actor top-p boundary.Sampling replay series
Runtime support uses SGLang v0.5.20 from #3321 and native-router chat forwarding from radixark/sgl-router-for-miles#20.
End-to-end behavior
Ownership and fail-closed boundaries
SGLang owns sampling-mask capacity, overflow, custom logit processors, and speculative-decoding compatibility. Miles does not duplicate those policies and does not add a speculative-decoding gate.
Miles rejects cases its trainer cannot currently replay faithfully:
Router compatibility is a transport boundary, not part of the replay algorithm. The Miles router forwards raw request bodies. The native SGLang router uses typed request schemas, so its chat and generate schemas must declare
return_sampling_mask. sgl-router-for-miles#20 adds the missing chat field and verifies forwarding to a worker.Request-specific positive top-k may differ from the launch default. SGLang enforces the physical support limit.
Current KL and distillation limitation
The actor still produces full-vocabulary logits, so sampling replay is not mathematically incompatible with KL or distillation. The current loss interface exposes one actor score per token. Replay uses that slot for the support-normalized policy score, while KL and distillation require a separate full-policy actor score. Until both are carried independently, validation rejects nonzero KL coefficient, KL loss, and on-policy distillation with replay.
Validation
--ci-testchecks rollout/trainer log-probability equality on every rollout, with log-probability-difference and KL metric gates.Acknowledgements
Thank you to Zilin Zhu for the original top-p replay implementation in THUDM/slime#2102.