Repository navigation
[trainer, megatron, fsdp] fix: forward-KL topk distillation shape mismatch with pad_bshd_to_minibatch_max - #8061
Open
alwaysyiyu wants to merge 1 commit into
Conversation
…batch_max When pad_bshd_to_minibatch_max is enabled, the student forward pads every micro-batch to the mini-batch global max seqlen (forced_max_seqlen), but compute_forward_kl_topk still splits the teacher top-k tensors at the per-micro-batch max seqlen. With heterogeneous sequence lengths the two shapes diverge and the alignment assertion fails. Thread forced_max_seqlen from the micro batch's non-tensor data through compute_topk_loss into the BSHD teacher split so the teacher tensors are padded to the same global max as student_logits. The THD path is unaffected (pad_bshd_to_minibatch_max only applies when use_remove_padding is False), and the FSDP path only gains a signature-parity parameter.
alwaysyiyu
requested review from
PeterSH6,
eric-haibin-lin,
tongyx361 and
vermouth1992
as code owners
September 29, 2026 11:46
7 of 8 tasks
wengeezhang
added a commit
to wengeezhang/verl
that referenced
this pull request
Oct 22, 2026
…P8 padding compute_forward_kl_topk packs the teacher's top-k log-probs and ids itself and asserts they match the student logits in the first two dimensions. Under an FP8 recipe the model forward pads the student inputs for the recipe's token alignment, and the teacher tensors were packed without that padding, so Megatron top-k distillation failed the assertion with any FP8 recipe (THD: the per-sequence alignment; BSHD: use_fp8_padding). _lm_head_logits_processor now passes quant_align_size (get_thd_quant_align_size(tf_config)) through compute_topk_loss to the Megatron loss, which packs the teacher tensors with it on THD and with use_fp8_padding derived from it on BSHD. Other strategies get no extra argument. BSHD has a second mismatch that does not depend on FP8: with pad_bshd_to_minibatch_max (on by default) the engine pads each micro-batch to the mini-batch max (forced_max_seqlen), and the teacher tensors are not padded to match. verl-project#8061 fixes that, so it is left out here. Tested on 2xB200 with a tiny Qwen3-MoE (MXFP8 training, MXFP8 vLLM rollout, the teacher on the second GPU), 5 steps: without the fix, top-k distillation on THD fails the assertion; with it, it passes. Unit tests cover the THD and BSHD packing of the teacher tensors and the argument compute_topk_loss passes per strategy. Review: verl-project#8127 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
7 of 8 tasks
wuxibin89
pushed a commit
that referenced
this pull request
Oct 22, 2026
…udent's CP split (#8332) ### What does this PR do? Follow-up to #8325, from a finding in its review ([comment](#8325 (comment))). `compute_forward_kl_topk` packs the teacher's top-k tensors itself. #8325 gave it the student's low-precision padding but not the student's CP split, so on THD the teacher rows could match `student_logits` in shape and still belong to other tokens: - **CP layout**: `dsv4_hybrid` splits the student's rows contiguously, while the teacher's were split zigzag, the default. With CP=2, MXFP8 and 16 sequences of 17 tokens, rank 0 holds 256 rows on both sides, and 128 of its 136 valid student tokens get the teacher values of another token or of a padding row (all 136 on rank 1). With 32-token sequences, 248 of 256 rows differ on rank 0 and all 256 on rank 1, and that was already the case before #8325. #8325 turned the 17-token case from a failed shape assertion into the same silent misalignment. - **Dynamic CP** (not reachable end to end today): the engine rejects dynamic CP with distillation (`Dynamic CP does not support distillation`). The micro-batch's `local_cp_size` is still passed down, so the teacher's tensors would be split over the same group as the student's if that combination is enabled; the unit tests cover the packing. `forward_step` now passes the `cp_layout` and `local_cp_size` it gives the model forward to the logits processor, which forwards them with `quant_align_size` through `distillation_ppo_loss` and `compute_topk_loss` to the Megatron loss. The teacher tensors are packed with the same THD arguments as the student inputs. Other strategies get no extra arguments. The BSHD path is unchanged: `preprocess_bshd_engine` always splits zigzag over the static CP group, for the student and the teacher alike. `pad_to_length` is already rejected with top-k distillation. ### Checklist Before Starting - [x] Search for similar PRs: [cp_layout distillation](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+cp_layout+distillation), [forward_kl_topk](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+forward_kl_topk). No open PR passes the CP split to the teacher packing. #8061 fixes the BSHD `forced_max_seqlen` mismatch; the two PRs touch neighbouring lines of `compute_forward_kl_topk` and `compute_topk_loss`, so whichever lands second needs a small rebase. - [x] Format the PR title as `[{modules}] {type}: {description}`. ### Test ```bash pytest -q tests/utils/test_megatron_fp8_padding_on_cpu.py tests/workers/test_megatron_distillation_only_on_cpu.py \ tests/workers/test_megatron_value_head_cp_layout_on_cpu.py # 49 passed pre-commit run --files <changed files> # passed ``` The new tests give every token its own number and compare, row by row, what the KL receives with the student's rows (CP state stubbed): | test | cases | |---|---| | static CP=2 | both layouts × both CP ranks × no / 16 / 32-token alignment × 17- and 32-token sequences (24) | | dynamic CP | a group of 2 within CP 4, both layouts × both ranks in the group (4) | | the failure mode | student contiguous, teacher zigzag: equal shapes, 128 of 136 tokens misaligned | | `forward_step` | a `dsv4_hybrid` model's layout and the micro-batch's `local_cp_size` reach the top-k loss | With the teacher packing reverted to ignore the CP split, the 12 contiguous and the 4 dynamic-CP cases fail. The logits processor and `compute_topk_loss` tests check that the arguments reach the Megatron loss and no other backend. The other CPU tests of the distillation, padding, router replay and CP-layout code pass as on main. Not run on GPUs. ### API and Usage Example No config or API change. `compute_forward_kl_topk`, `compute_topk_loss`, `distillation_ppo_loss` and `MegatronEngineWithLMHead._lm_head_logits_processor` take two more optional arguments, `cp_layout` (default `"zigzag"`) and `local_cp_size` (default `None`); the defaults keep the previous behaviour. ### Design & Code Changes - `verl/workers/engine/megatron/transformer_impl.py`: `forward_step` hands the logits processor the `cp_layout` and `local_cp_size` of the student forward; `_lm_head_logits_processor` passes them to the top-k loss. - `verl/trainer/distillation/losses.py`: `distillation_ppo_loss` and `compute_topk_loss` forward them to the Megatron loss only. - `verl/trainer/distillation/megatron/losses.py`: `compute_forward_kl_topk` packs the teacher's THD tensors with `quant_align_size`, `local_cp_size` and `cp_layout`, as the model forward packs the student's. - Tests: `tests/utils/test_megatron_fp8_padding_on_cpu.py`, `tests/workers/test_megatron_distillation_only_on_cpu.py`. ### 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: 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. Developed with AI assistance. I reviewed every changed line, ran the tests above, and can defend the change end to end. 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?
When
pad_bshd_to_minibatch_max=True(introduced in #6901), the student forward pads everymicro-batch to the mini-batch's global max seqlen via
forced_max_seqlen, butcompute_forward_kl_topkstill splits the teacher top-k tensors withpreprocess_bshd_enginewithout passing
forced_max_seqlen. With heterogeneous sequence lengths the teacher ispadded to the per-micro-batch max instead, so the two shapes diverge and the assertion
fails.
This threads the micro-batch's
forced_max_seqlen(already stored as non-tensor data by theengine) through
compute_topk_lossinto the BSHD teacher split. It picks up the earlier closedattempt #7012 (which recomputed the value as
student_logits.shape[1] * cp_size) and instead usesthe authoritative
forced_max_seqlenrecorded by the engine, matching the student path exactly.Related: #6901 (introduced the padding), #7012 (prior closed attempt).
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI)[trainer, megatron, fsdp] fix: forward-KL topk distillation shape mismatch with pad_bshd_to_minibatch_maxTest
Validated on an Ascend NPU cluster running on-policy distillation (OPD) with Qwen3.6-35B-A3B MoE,
TP=2 CP=2,
pad_bshd_to_minibatch_max=True, and heterogeneous prompt/response lengths. Before thefix the step aborts with the shape-mismatch assertion above; after the fix the step completes and
student_logits.shape[:2] == teacher_split.shape[:2]holds. The THD path andpad_bshd_to_minibatch_max=Falseare unaffected (noforced_max_seqlenis stored).API and Usage Example
No user-facing API/config change.
compute_forward_kl_topkgains a backward-compatibleforced_max_seqlen: int | None = Noneparameter (internal), andcompute_topk_lossreadsforced_max_seqlenfrom the micro-batch's non-tensor data. Callers need no change:Design & Code Changes
verl/trainer/distillation/losses.py: readforced_max_seqlenfrom the micro batch and forward it to the distillation loss fn.verl/trainer/distillation/megatron/losses.py: acceptforced_max_seqlenand pass it to bothpreprocess_bshd_enginecalls in the BSHD branch (THD branch untouched).verl/trainer/distillation/fsdp/losses.py: accept the parameter for signature parity (unused; FSDP2 does not usepad_bshd_to_minibatch_max).Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.