Repository navigation
Guard blockwise FP8 backward (E5M2) wgrad against all-zero gradient blocks - #7523
HollowMan6 wants to merge 2 commits into
Conversation
5dfc4f9 to
7386024
Compare
7386024 to
865a5cb
Compare
|
Thank you @Phlip79 ! Please let me know if there are any other comments, thanks! |
865a5cb to
f7ea550
Compare
|
/ok to test f7ea550 |
f7ea550 to
2f68553
Compare
jaredcasper
left a comment
There was a problem hiding this comment.
What tests have been done to ensure this doesn't impact other training runs in fp8? What, if any, is the speed impact of this?
| block_scaling_recipe_cls = transformer_engine.common.recipe.Float8BlockScaling | ||
| fp8_recipe = block_scaling_recipe_cls(fp8_format=fp8_format) | ||
| bwd_qparams = block_scaling_recipe_cls.fp8_quant_bwd_grad | ||
| fp8_recipe.fp8_quant_bwd_grad = transformer_engine.common.recipe.QParams( |
There was a problem hiding this comment.
This monkey patching TE? Shouldn't this just be fixed in TE?
There was a problem hiding this comment.
Yes, the current code updates only the recipe instance to make the guard effective Bbut it's only related to the default recipe choice so I think eventually we should be flexible to customize the recipe. To avoid the monkey patch, TE ideally need to accept additional fp8_quant_bwd_grad for its transformer_engine.common.recipe.Float8BlockScaling, so that something like this can work: f7ea550
There was a problem hiding this comment.
Sorry for delay. So why not fix this in TE and avoid the monkey patch? If it's just the default recipe then fix the default recipe in TE? Adding the flexibility can come later if needed.
There was a problem hiding this comment.
Thank you! I opened NVIDIA/TransformerEngine#3651 on the TE, I prefer keeping this PR opened until that one is discussed/merged/released for other's reference. Thanks again.
The change is restricted to the Float8BlockScaling recipe path. This modification only constructs the recipe and updates backward quantization parameters during initialization with Transformer Engine API, so I don't expect it will introduce impact on speed. |
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
4d81cd0 to
2a32b86
Compare
|
/ok to test 2a32b86 |
| fp8_recipe.fp8_quant_bwd_grad = transformer_engine.common.recipe.QParams( | ||
| power_2_scale=bwd_qparams.power_2_scale, | ||
| amax_epsilon=1e-12, | ||
| random_hadamard_transform=bwd_qparams.random_hadamard_transform, |
There was a problem hiding this comment.
that's confusing code, because there is no hadamard transform to fp8 block scaling at all, so it's supposed to be False unless we are talking about nvfp4
| # Guard backward (E5M2) wgrad against all-zero gradient blocks: | ||
| # amax=0 → scale=inf → NaN. | ||
| block_scaling_recipe_cls = transformer_engine.common.recipe.Float8BlockScaling | ||
| fp8_recipe = block_scaling_recipe_cls(fp8_format=fp8_format) |
There was a problem hiding this comment.
I don't think we should use E5M2 for backward pass whenever we are using block scaling recipe, E5M2 is not gonna be useful for this recipe, so what was the motivation for using E5M2 in the first place?
This should be fixed TE agreed, although I don't think we should use E5M2 in the first place. Also it's a bit surprising to me this issue would happen because there is already a guard in the kernel to take care of this: https://github.com/NVIDIA/TransformerEngine/blob/62b13895c2e6edabfae7cfe6d2ae761358503c3d/transformer_engine/common/recipe/recipe_common.cuh#L24 So I would be curious to understand why would this issue happen, thanks. |
|
Thank you @zhongbozhu for pointing this out that there is already a guard in the kernel. For context, I was working on FP8 E2E RL support for GLM5.3 Flash in verl. I was referring to https://github.com/verl-project/verl-recipe/blob/4da0651ebfc7ddbf240e41bace809e05911a5526/low_precision/run_dapo_qwen3_moe_30b_megatron_fp8e2e.sh#L96-L99 (E5M2 backward + blockwise) The NaN was kind of random and hard to say if there's a deterministic way to reproduce it, and right now I'm not sure if it was caused by some issues in the sibling PRs instead (#7521 / #7522 / #7054) and whether it has been fixed later in those PRs. I will try to remove the patches here/in TE and let you know if I found a deterministic way to reproduce it. |
|
@HollowMan6 Hi, thanks for the pointer, it's an interesting choice for them to use E5M2, since my impression is that you do not need E5M2 in blockwise recipe at all, you just need E4m3, the E5M2 backward recipe should be only be used in a per-tensor based recipe, not blockwise recipe. Also the fp8 blockwise recipe is only recommended on Hopper, and in Blackwell we should just do mxfp8, and with E4M3 for both fwd and bwd. |
Faithful dev port of main PR NVIDIA#7523 at 2a32b86: construct the blockwise FP8 recipe with an explicit backward-gradient QParams whose amax_epsilon is 1e-12, so an all-zero E5M2 wgrad block gets a finite scale instead of inf and no longer turns the weight gradient into NaN. Forward quantization parameters and the other backward QParams fields keep the Transformer Engine recipe defaults. Add the blockwise case to the FP8 determinism test and a recipe check. Every line the PR adds or removes in fp8_utils.py and test_fp8_determinism.py is applied verbatim. Dev adaptations: - test_fp8_determinism.py: dev lacks main's Blackwell-only test_grouped_mxfp8_parameter_updates_replay_bit_exact (and its inspect and determinism.kernels.harness imports), so test_blockwise_recipe_guards_zero_backward_blocks is appended after dev's test_bit_exact_under_quantization instead of after that test. Not ported: none. Already on dev: none. Upstream-PR: NVIDIA#7523 Upstream-Base: daa0074 Upstream-Head: 2a32b86 Scope: faithful dev port; deviations are listed above Ported-by: Robin Zhang <robinz@nvidia.com> Signed-off-by: Hollow Man <hollowman@opensuse.org> Signed-off-by: Robin Zhang <robinz@nvidia.com>
|
Closing as it seems this defensive change is no longer needed. |
What does this PR do?
Guard the blockwise FP8 recipe's backward (E5M2) weight-gradient quantization against all-zero gradient blocks, where
amax = 0gives an infinite scale and NaN gradients.Split out of #7054 as discussed there: a small
fp8_utils.pyfix that is independent of the GLM-5.3-Flash model work. Sibling PRs: #7521 (mHC precision controls) and #7522 (KPool DSA indexer); #7054 now carries only the KDA layers.Changes
Float8BlockScalingis now constructed with an explicitfp8_quant_bwd_grad=QParams(power_2_scale=<recipe default>, amax_epsilon=1e-12).amax_epsilonfloors the per-block amax, so a gradient block that is entirely zero gets a finite scale instead ofinf; blocks with a non-zero amax are unaffected, and the forward quantization parameters are unchanged.power_2_scaleis read from the recipe class default so the guard follows Transformer Engine's setting.Motivation
With
fp8_recipe=blockwise, a weight-gradient block that is entirely zero hasamax = 0, so its E5M2 scale becomesinfand the quantized gradient turns into NaN, which then poisons the whole weight gradient. This came up in the GLM-5.3-Flash FP8 work in #7054 but is not specific to it.Testing
No unit test added: the change is a recipe parameter and only affects blocks whose gradient amax is exactly zero (previously NaN).
Issue tracking
For PRs from open-source community contributors:
Linked issue: none (small fix split out of #7054)
Contribution process
Pre-checks
jit_fuser/torch.compile, CUDA extension, TE or external-library dispatch, or a scatter/index accumulation), I have added or updated its bit-exact determinism test and registered it intests/unit_tests/determinism/kernels/manifest.py(guide)Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.