[megatron] fix: support FP8 padding for BSHD actor forward - #6884
Conversation
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
Code Review
This pull request enables FP8 support for the BSHD format in Megatron-Core (MCore) models. It removes the previous assertion blocking FP8 usage with BSHD, introduces a helper function _align_bshd_max_seqlen_for_fp8 to align sequence lengths for Transformer Engine FP8 block quantization, and integrates this alignment logic into both preprocess_bshd and preprocess_bshd_no_padding. Additionally, a new unit test has been added to verify the FP8 padding alignment and roundtrip processing. There are no review comments, and the changes look solid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Qwen3.5 THD format has supported in NVIDIA/Megatron-LM#2645, please upgrade mcore==0.18.0. We may drop BSHD support in future release. |
|
Thanks for the pointer. This PR targets release/v0.7.1 specifically, where our current stack still needs BSHD because the mcore 0.18 THD support is not available in that branch/environment yet. The change is scoped to the existing BSHD path and should not affect THD/remove-padding users. If the project prefers not to take BSHD improvements even for release/v0.7.1, I’m happy to keep this as a downstream patch. |
|
@gem-mint Got it, please fix pre-commit. Could you submit this PR to main as well? |
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: gem-mint <146161229+gem-mint@users.noreply.github.com>
|
@wuxibin89 Thanks! opened the main-branch companion PR here: #6887. The PR carries the same BSHD FP8 actor-padding fix adapted to the current main branch. |
What does this PR do?
Enables Megatron actor/ref forward passes in BSHD format to run with Transformer Engine FP8 enabled.
Qwen3.x/GDN-style models can require
use_remove_padding=False, which routes Megatron actor updates through the BSHD forward path. Before this change, that path rejected FP8 with an assertion and did not apply the FP8 block-alignment padding that TE kernels require.Root cause
model_forward_gen(..., data_format="bshd")explicitly asserted thatfp8 is None, so actor update/log-prob computation failed as soon asoverride_transformer_config.fp8=hybridore4m3was enabled.After removing the assertion, BSHD still needed the same kind of FP8-aware sequence padding used by the packed/no-padding paths: TE block FP8 kernels require the local token extent to be divisible by 128 after TP/CP partitioning.
Design & Code Changes
verl/models/mcore/model_forward.py.use_fp8_paddingthrough BSHD input preprocessing and logits-processor argument preprocessing._align_bshd_max_seqlen_for_fp8inverl/models/mcore/util.pyto align BSHD sequence length for TE FP8 block quantization while preserving TP/CP divisibility constraints.preprocess_bshd_no_paddingto keep BSHD FP8 alignment logic consistent.Duplicate-work check
No linked issue was provided for this fix.
Checks run before opening this PR:
Results:
preprocess_thd_engineFP8 + CP out-of-bounds issue for THD/SFT. This PR targets BSHD actor forward/logprob padding and removes the BSHD FP8 assertion, so it is not a duplicate.Test
Ran successfully:
Attempted but not run in this local Windows runtime:
External validation: the patched v0.7.1 tree was used to start a Qwen3.x-35B-A3B GRPO training run with Megatron + SGLang and actor FP8 enabled.
AI assistance
This PR was prepared with AI assistance. Human review done before marking the PR ready for review.