Repository navigation
[algo, worker] fix: handle padding-only top-k distillation microbatches - #7887
Open
zupengwang wants to merge 1 commit into
Open
zupengwang wants to merge 1 commit into
zupengwang wants to merge 1 commit into
Conversation
Keep synthetic padding out of distillation diagnostics and allow empty observations when reducing extrema across DP ranks. Co-authored-by: Codex <noreply@openai.com>
zupengwang
marked this pull request as ready for review
September 16, 2026 16:40
zupengwang
requested review from
PeterSH6,
eric-haibin-lin,
tongyx361 and
vermouth1992
as code owners
September 16, 2026 16:40
7 of 8 tasks
5 tasks done
7 of 8 tasks
wuxibin89
pushed a commit
that referenced
this pull request
Oct 22, 2026
…of top-k distillation tensors (#8325) ### What does this PR do? Follow-up to #8127, for two of its review threads. 1. **Recipe-agnostic THD padding** ([review](#8127 (comment))). `preprocess_packed_seqs` and `preprocess_thd_engine` took two FP8-specific arguments, `use_fp8_padding` and `fp8_recipe`. They now take one integer, `quant_align_size`: the token alignment each packed sequence needs for the model's low-precision GEMMs (`None`: no low-precision padding). `get_thd_quant_align_size(config)` derives it from the `TransformerConfig`, for FP8 recipes through Megatron-Core's `get_fp8_align_size` (32 for MXFP8, 16 otherwise), so a new format such as NVFP4 or MXFP4 (#7971) adds a branch there instead of another parameter. The model forward, the fused forward and router replay all call it. The padding itself is unchanged. 2. **Top-k distillation under FP8** ([review](#8127 (comment))). `compute_forward_kl_topk` packs the teacher's top-k tensors itself and asserts they match the student logits, but it packed them without the student's FP8 padding, so Megatron top-k distillation failed that assertion under any FP8 recipe; the gap predates MXFP8. The engine now passes `quant_align_size` to the Megatron loss, which packs the teacher tensors with it on THD and with `use_fp8_padding` derived from it on BSHD. Left out: on BSHD, `pad_bshd_to_minibatch_max` (on by default) pads the student micro-batch to the mini-batch max (`forced_max_seqlen`) while the teacher tensors are not padded to match. That fails the same assertion even in BF16, and #8061 fixes it. The two PRs touch neighbouring lines in `compute_forward_kl_topk`, so whichever lands second needs a small rebase. ### Checklist Before Starting - [x] Search for similar PRs: [quant_align_size](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+quant_align_size), [distillation padding](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+distillation+padding), [pad_bshd_to_minibatch_max](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+pad_bshd_to_minibatch_max). - Not a duplicate: #8061 fixes only the BSHD `forced_max_seqlen` mismatch, which this PR leaves to it; #7887 handles metrics of empty-response micro-batches; #7971 (native NVFP4 training) adds its own padding flag to the same helpers and could add an NVFP4 branch to `get_thd_quant_align_size` instead. - [x] Format the PR title as `[{modules}] {type}: {description}`. `[BREAKING]` because two public helpers change keyword arguments (see below). ### Test CPU unit tests and pre-commit: ```bash pytest -q tests/utils/test_megatron_fp8_padding_on_cpu.py tests/workers/test_megatron_distillation_only_on_cpu.py \ tests/utils/test_megatron_bshd_preprocess.py tests/utils/test_config_router_replay_on_cpu.py \ tests/workers/engine/megatron/test_router_replay_utils_on_cpu.py tests/utils/megatron/test_router_replay_model_walk_on_cpu.py # 90 passed, 1 skipped pytest -q tests/models/test_model_forward_fused.py # 6 passed, 3 failed; the same 3 fail on main in the same environment pre-commit run --files <changed files> # passed ``` New tests cover the alignment helper, both THD preprocess paths with and without the padding, the teacher's top-k packing on THD (MXFP8 and a 16-token FP8 recipe) and BSHD, and that the engine and `compute_topk_loss` pass `quant_align_size` to the Megatron loss only. On hosts without Megatron, the tests stub Megatron-Core's `get_fp8_align_size` with the same 32/16 rule. End to end on 2×B200: - A tiny random Qwen3-MoE (2 layers, 8 experts, top-2) on GSM8K, 5 steps per run, the trainer on one GPU (TP, EP and CP 1). - MXFP8 training with Transformer Engine (`fp8_recipe=mxfp8`) and an MXFP8 vLLM rollout, except the last row. - The distillation runs put the teacher (the same model, BF16, served by vLLM) on the second GPU, with `forward_kl_topk` and top-16. - vLLM 0.29.0, Transformer Engine 2.16.1, Megatron-Core 0.19.2. - "This PR" is this PR's files, byte for byte, on #8127's head before it merged. That predates #8308, so the vLLM rollout used FlashInfer CuTe-DSL for the dense layers and FlashInfer TRT-LLM for the experts. "Before" is #8127's head without this PR. | run | code | result | |---|---|---| | GRPO, THD | this PR | passes: 5/5 steps, no non-finite metric, `rollout_corr/kl` 0.0019–0.0023 | | GRPO + router replay R2, THD | this PR | passes: `rollout_corr/kl` 0.0022–0.0027 | | top-k distillation, THD | this PR | passes: `rollout_corr/kl` 0.0018–0.0026, distillation metrics logged | | top-k distillation, THD | before | fails the shape assertion in `compute_forward_kl_topk` | | GRPO, THD | before | passes: `rollout_corr/kl` 0.0020–0.0030 (control) | | top-k distillation, BSHD | this PR | fails the same assertion: the `forced_max_seqlen` mismatch that #8061 fixes | | top-k distillation, BSHD | this PR + the micro-batch's `forced_max_seqlen` passed to the teacher packing, as in #8061 | passes: `rollout_corr/kl` 0.0018–0.0027 | | top-k distillation, BSHD, BF16 | before | fails the same assertion: that mismatch does not depend on FP8 | `rollout_corr/kl` is the KL between vLLM's and the trainer's log-probs. The fused forward, FP8 recipes other than MXFP8, and TP or CP above 1 were not run on hardware; the unit tests above cover them. ### API and Usage Example No config or CLI change. `preprocess_packed_seqs` and `preprocess_thd_engine` take `quant_align_size` instead of `use_fp8_padding` and `fp8_recipe`. `preprocess_bshd_engine` keeps `use_fp8_padding`, which verl-omni passes. Within verl-project, nothing else calls the THD helpers with the removed arguments. ```python from verl.models.mcore.util import get_thd_quant_align_size, preprocess_thd_engine quant_align_size = get_thd_quant_align_size(tf_config) # 32 for MXFP8, 16 for other FP8 recipes, None without FP8 packed, *_ = preprocess_thd_engine(input_ids, pre_process=True, quant_align_size=quant_align_size) ``` ### Design & Code Changes - `verl/models/mcore/util.py`: `get_thd_quant_align_size(config)`; the THD helpers take `quant_align_size`. Each sequence is padded to a multiple of `lcm(quant_align_size, align_size)` and the total to `align_size * 128`, as before. - `verl/models/mcore/model_forward.py`, `model_forward_fused.py`: derive `quant_align_size` from the helper; BSHD uses `use_fp8_padding = quant_align_size is not None`. - `verl/utils/megatron/router_replay_utils.py`: router replay records and replays with the helper instead of inlining the rule. - `verl/workers/engine/megatron/transformer_impl.py`: `_lm_head_logits_processor` passes `quant_align_size` to the top-k loss. - `verl/trainer/distillation/losses.py`: `compute_topk_loss` and `distillation_ppo_loss` forward it to the Megatron loss only. - `verl/trainer/distillation/megatron/losses.py`: `compute_forward_kl_topk` packs the teacher's top-k tensors with it. ### Checklist Before Submitting - [x] Read the [Contribute Guide](https://github.com/verl-project/verl/blob/main/CONTRIBUTING.md). - [x] Apply [pre-commit checks](https://github.com/verl-project/verl/blob/main/CONTRIBUTING.md#code-linting-and-formatting). - [x] Add / Update [the documentation](https://github.com/verl-project/verl/tree/main/docs). No user-facing change to document. - [x] Add unit or end-to-end test(s) to [the CI workflow](https://github.com/verl-project/verl/tree/main/.github/workflows) to cover all the code: the new tests are CPU unit tests (`*_on_cpu.py`) that CI runs. - [ ] Once your PR is ready for CI, send a message in [the `ci-request` channel](https://verl-project.slack.com/archives/C091TCESWB1). - [x] Not related to the `recipe` submodule. AI assistance was used to write the code, the tests and this description. The submitter reviewed the changes and ran the tests above. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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?
V1 batch balancing can construct synthetic samples with an all-zero
response_mask. When a top-k distillation microbatch contains only these samples, the current metric code callsmin()/max()on empty tensors and aborts training before backward. Guarding only those reductions is insufficient: omitting padding observations also gives different metric counts across DP ranks, whichMetric.aggregate_dpcurrently rejects.Represent absent observations using empty lists and empty
Metricinstances, and aggregate MIN/MAX over the observations that exist across DP ranks. If every rank has no observation, the DP extremum is undefined (NaN). SUM/MEAN retain their existing aligned-count requirement. The loss, normalization, backward participation, and valid-token metric calculations are preserved. The license checker also accepts the 2026 individual-contributor header used by the new tests (two lines).Duplicate-work check
Searched open and closed PRs for distillation + empty, student_mass, all-masked, and empty metrics, and inspected relevant patches. Rechecked immediately before publication; the related patches below are unchanged.
reduce_metricsandMetric._aggregate; it does not guard distillation tensor reductions or support unequal observation counts inMetric.aggregate_dp. This patch leaves both of those functions unchanged.Test
Validated commit:
37836a40bdc265e5f4bff615023946b90f744bb4, based onc8faa0870a115b7bc73ee76545b804f5ffb15c5b.Remote Linux host with RTX 4090; Python 3.12.14, PyTorch 2.13.0+cu130, Transformers 5.15.1, TensorDict 0.10.0.
allgather_dict_into_dictplus worker reductions preserve all real-sample metrics.GPU evidence is for the model/loss path on one GPU; it does not establish multi-GPU FSDP/NCCL behavior, end-to-end rollout training, convergence, performance, or other engine support.
AI assistance
Prepared and tested with OpenAI Codex assistance; submitted at the author's request.
API and usage
No user-facing API or configuration changes. Existing top-k distillation training automatically handles padding-only microbatches.
Submission checklist
*_on_cpu.pyCPU workflow.