[feat] Init true on policy with qwen_dense - #1052
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a 'true-on-policy' launch contract for Qwen3-dense models, centralizing configuration for SGLang and Megatron backends. It includes updates to rollout log-probability computation, enabling recomputation via SGLang prefill, and adds robust handling for tensor-parallel vocab gathering and loss masking. My feedback focuses on improving the safety of list iteration in cp_utils.py by recommending strict=True for zip to ensure data alignment.
|
|
||
| local_masks = [] | ||
| for i, (total_length, response_length, loss_mask) in enumerate( | ||
| zip(total_lengths, response_lengths, loss_masks, strict=False) |
There was a problem hiding this comment.
Using zip with strict=False can hide potential bugs if the input lists (total_lengths, response_lengths, loss_masks) have mismatched lengths. It's safer to use strict=True to ensure that all per-sample lists are correctly aligned. If they have different lengths, it would be better to raise an error and fail fast.
| zip(total_lengths, response_lengths, loss_masks, strict=False) | |
| zip(total_lengths, response_lengths, loss_masks, strict=True) |
| def get_base_gpu_id(args, rank): | ||
| num_gpus = min(args.num_gpus_per_node, args.rollout_num_gpus_per_engine) | ||
| if args.colocate: | ||
| if getattr(args, "colocate", False): |
There was a problem hiding this comment.
can we keep the original implementation here (just use args.xxx rather than getattr) to avoid silent errors?
| return pg_loss, loss_masks, metrics | ||
|
|
||
|
|
||
| _POLICY_LOSS_DUMP_COUNTER = 0 |
There was a problem hiding this comment.
nit: can we make this part more clean? (e.g., if the debug utils are not expected to be frequently used, we can remove them; if they are expected to be used in the future, can we move to a separate file, and just plug several lines in core logics?)
| train_fp8: bool = False | ||
| enable_megatron_bridge: bool = False | ||
| enable_mis: bool = False | ||
| tensor_model_parallel_size: int | None = None |
There was a problem hiding this comment.
nit: to keep the readability and reproducibility of these scripts, I might suggest not adding too many arguments and changing too much here, maybe we can just provide one verified config and let the user to edit the script according to their task (e.g. just one --true-on-policy, and any other necessary args; and we directly code the recommended settings in this file). This will also make it easier for us to maintain
| return eval_datasets | ||
|
|
||
|
|
||
| def _maybe_enable_true_on_policy_sglang_cp_lm_head(args) -> None: |
There was a problem hiding this comment.
qq: why is this function empty?
Zhichenzzz
left a comment
There was a problem hiding this comment.
Most backends LGTM! Thank you for the great work! Just a quick reminder, there is a small issue raised by ci test, https://github.com/radixark/miles/actions/runs/25076368286/job/73469945178?pr=1052#step:9:347
| for i, (total_length, response_length, loss_mask) in enumerate( | ||
| zip(total_lengths, response_lengths, loss_masks, strict=False) | ||
| ): | ||
| max_seq_len = max_seq_lens[i] if max_seq_lens is not None else None | ||
| prompt_length = total_length - response_length | ||
| _, _, _, tokens_offset = get_logits_and_tokens_offset_with_cp( | ||
| total_length, response_length, qkv_format, max_seq_len | ||
| ) | ||
| loss_mask_0 = loss_mask[tokens_offset[0][0] - prompt_length : tokens_offset[0][1] - prompt_length] | ||
| loss_mask_1 = loss_mask[tokens_offset[1][0] - prompt_length : tokens_offset[1][1] - prompt_length] | ||
| local_masks.append(torch.cat([loss_mask_0, loss_mask_1], dim=0)) |
There was a problem hiding this comment.
Could we extract L147–157 and the duplicate slicing at L98–109 into a shared _slice_loss_mask_for_local_cp helper, so both call sites share a single source of truth?
Implements PR 11 from miles_migration.md on top of origin/main. Wires
the Miles-side half of true-on-policy exact-zero alignment between
SGLang rollout and Megatron scoring, all gated on --true-on-policy-mode
so the default off-policy path is unchanged.
Contract A (deterministic runtime mode):
- Add --recompute-logprobs-via-prefill (requires --true-on-policy-mode).
- When --true-on-policy-mode is set, auto-select sglang
rl_on_policy_target=fsdp_tp (tp>1) or fsdp (tp=1) and enable
deterministic inference.
- --recompute-logprobs-via-prefill also enables SGLang
prefill-only-deterministic-inference.
Contract D (logprob scoring):
- compute_log_probs / _calculate_log_probs_and_entropy_true_on_policy
now: gather full padded TP vocab -> truncate to real vocab_size
-> FP32 log_softmax -> gather target tokens.
- get_log_probs_and_entropy threads vocab_size through so the
truncation happens after gather, not before log_softmax.
- policy_loss_function computes train_rollout_logprob_abs_diff from
the train-side recomputed log-probs (not old_log_probs), which is
what the exact-zero acceptance bar measures.
Tests:
- tests/fast/utils/test_true_on_policy_logprobs.py:
- TP=1 truncate-after-gather parity.
- Fake TP-sharded vocab gather/truncate/log-softmax parity.
- tests/fast/backends/training_utils/test_true_on_policy_loss_metrics.py:
- train_rollout_logprob_abs_diff uses recomputed train log-probs;
zero when rollout matches, non-zero when perturbed.
- tests/fast/utils/test_arguments.py:
- --recompute-logprobs-via-prefill parsing.
- --true-on-policy-mode propagation to SGLang server args for
TP=1 fsdp and TP>1 fsdp_tp, with and without prefill recompute.
Compatibility:
- All new behavior is dormant unless --true-on-policy-mode is passed.
- The fused vocab-parallel CE path is still the default for
compute_log_probs when true_on_policy_mode is False.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn>
Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com>
Co-authored-by: Yi Zhang <1109276519@qq.com>
pre-commit isort fix for the new test file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
The previous _gather_true_on_policy_full_logits used Megatron's all_gather_last_dim_from_tensor_parallel_region whose backward is reduce-scatter. When each TP rank computes the same replicated loss from the gathered full vocabulary, reduce-scatter sums TP_size identical gradients into the local logits, scaling the gradient by TP_size (visible as a ~1.8x grad_norm vs the off-policy baseline at clipfrac=0). Replace with a typed _ReplicatedLossAllGatherLastDim autograd Function whose backward narrows (splits) the gradient along the gathered last dim — the correct inverse of the forward all-gather when the downstream loss is replicated on every rank. Also includes pre-commit auto-fixes (isort, black) for the true-on-policy stack files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Per review feedback, drop the in-progress journal files that should not ship in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
Co-authored-by: zju-stu-lizheng <lizheng.cs@zju.edu.cn> Co-authored-by: zyxiyy02 <282300612+zyxiyy02@users.noreply.github.com> Co-authored-by: Yi Zhang <1109276519@qq.com>
6fac9bb to
2047f7d
Compare
2047f7d to
798b791
Compare
Summary
Initial framework introducing true-on-policy for Qwen3-dense across SGLang, Megatron, and Miles. This is one of three tightly-coupled PRs that must land together — they share a single contract identifier
qwen3_dense_true_on_policy_v1defined by a vendored schema in each repo.Companion PRs (must land in lockstep):
Target
Bit-identical (exact-zero) logprob parity between the SGLang rollout engine and the Megatron trainer for every scored response token at TP=1, TP>1, PP>1, and Ulysses CP for Qwen3-4B (dense).
Design
Three-layer contract architecture:
SGLangSpecProviderlayer classes.The contract object owns its own runtime policy — adding a new architecture (e.g. Qwen3-MoE) is one new contract object + one new model profile entry, not edits across three repos.
In this PR (Miles)
miles/true_on_policy/package:schema.py— vendored shared identity, byte-identical with SGLang and Megatron copiescontracts.py—TrueOnPolicyContractdefinition + lookup registrymodel_profiles.py—QWEN3_DENSE_PROFILE(sole registered family)config.py—TrueOnPolicyConfig→TrueOnPolicyKernelPolicy→TrueOnPolicyLaunchPlan; renders SGLang / Megatron / env args from typed contractapply_true_on_policy_script_defaults(args)andbuild_true_on_policy_launch_plan(args)entry pointsscripts/run_qwen3_4b.pylauncher with--true-on-policy=trueswitch and Megatron model_provider routing throughconfig.true_on_policy_contractmiles/utils/ppo_utils.py:compute_log_probs(..., true_on_policy_mode=True)— full-vocab gather → real-vocab truncate → fp32 log-softmax → gather_ReplicatedLossAllGatherLastDimautograd Function (the grad_norm bug fix). Megatron's standardall_gather_last_dim_from_tensor_parallel_regionhas a reduce-scatter backward, which is correct when each rank contributes a distinct output gradient. In the true-on-policy logprob path every TP rank computes the same scalar loss from the gathered full vocabulary, so reduce-scatter sums TP_size identical gradients into the local logits and scales them by TP_size. Replaced with a typed autograd Function whose backward narrows (splits) the gradient — the correct inverse of the forward all-gather when the downstream loss is replicated.miles/backends/training_utils/loss.py—true_on_policy_modehandling (dtype management, response-only logits slicing across CP modes)--use-sglangand--sglang-rl-on-policy-targetflags. Only emits--true-on-policy-contract.Validation
tests/fast/true_on_policy/,tests/fast/utils/test_true_on_policy_logprobs.py)_ReplicatedLossAllGatherLastDimbackward (verifies narrow vs reduce-scatter)Out of scope
TrueOnPolicyModelProfileentry per familyTest plan
_ReplicatedLossAllGatherLastDimfix)🤖 Generated with Claude Code