fix(clip_grads): handle empty grads_for_norm in inf-norm and p-norm paths - #5530
Conversation
|
This PR has been automatically converted to draft because all PRs must start as drafts. When you are ready for review, click Ready for Review to begin the review process. This will:
See the contribution guide for more details. |
Mattral
left a comment
There was a problem hiding this comment.
looking forward to have this reviewed
0eca5c6 to
ee1cefd
Compare
janEbert
left a comment
There was a problem hiding this comment.
We can simplify a bit in the norm_type != inf code path, since both norm_type == 2.0 and norm_type != 2.0 need the same zero-tensor if grads_for_norm is an empty list. norm_type == 2.0 currently solves it with the grad_norm, which we could remove if we initialized the total_norm zero-tensor earlier. The entire norm_type == 2.0 path could then be simplified around that.
E.g.:
total_norm = torch.zeros(1, dtype=torch.float, device='cuda')
if not grads_for_norm:
pass
elif norm_type == 2.0:
dummy_overflow_buf = torch.zeros(1, dtype=torch.int, device='cuda')
# Use apex's multi-tensor applier for efficiency reasons.
# Multi-tensor applier takes a function and a list of list
# and performs the operation on that list all in one kernel.
grad_norm, _ = multi_tensor_applier(
l2_norm_impl,
dummy_overflow_buf,
[grads_for_norm],
False, # no per-parameter norm
)
# Since we will be summing across data parallel groups,
# we need the pow(norm-type).
total_norm = grad_norm**norm_type
else:
for grad in grads_for_norm:
grad_norm = torch.norm(grad, norm_type)
total_norm += grad_norm**norm_type|
Thanks for the suggestion that's a cleaner structure. I'll hoist |
6a06675 to
0e5d2e9
Compare
janEbert
left a comment
There was a problem hiding this comment.
Thank you for the contribution and quick adjustment!
Mattral
left a comment
There was a problem hiding this comment.
@janEbert
Thanks for the review! Updated the commit per your suggestion. Hoisted the zero-tensor init before both branches and dropped the redundant L2 guard. Also fixed the DCO sign-off. LMK if anything else needs changing.
|
/ok to test 0e5d2e9 |
|
You'd quickly need to run |
get_grad_norm_fp32 crashes when grads_for_norm is empty: - inf-norm path: max() over an empty generator raises ValueError. Fix: pass default=torch.tensor(0.0) to max(). - norm_type != inf paths: hoist total_norm initialisation to a shared torch.zeros(1, dtype=torch.float, device='cuda') before the norm_type == 2.0 / else branches. An early `if not grads_for_norm: pass` guard covers the empty case for both paths, removing the redundant inline guard from the L2 branch and fixing the generic p-norm branch where total_norm previously stayed as a Python float and caused a TypeError in torch.distributed.all_reduce. The L2 path previously solved the empty case with a local `else: grad_norm = torch.zeros(...)`. That guard is now unnecessary and removed; the shared zero-tensor init covers it. Signed-off-by: Min Htet Myet <mattralminn@email.com> Signed-off-by: Mattral <mattralminn@gmail.com>
Mattral
left a comment
There was a problem hiding this comment.
The autoformatter fix has been pushed. CI is now waiting for approvals from @NVIDIA/core-adlr and @NVIDIA/core-nemo.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/29325376449 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/29330988382 |
|
Previous fail due to deterministic throughput being 1.29 slower than non-deterministic baseline. I doubt it's related to this PR, probably just flaky CI. |
test_clip_grads.py previously contained a single test covering only
OptimizerConfig.grad_norm_skip_threshold's default value. get_grad_norm_fp32
itself had no coverage across any of its three norm_type branches.
Add parametrized tests for norm_type in {2.0, inf, 1.0}, covering:
- empty grads_for_norm returning 0.0 without raising (regression test
for the crash fixed in NVIDIA#5530 / issue NVIDIA#5529)
- non-empty grads_for_norm matching a plain torch.norm-based reference
computation
Signed-off-by: Min Htet Myet <88831350+Mattral@users.noreply.github.com>
…aths (NVIDIA#5530) Signed-off-by: Min Htet Myet <mattralminn@email.com> Signed-off-by: Mattral <mattralminn@gmail.com> Signed-off-by: mchochowski <mchochowski@nvidia.com>
…aths (NVIDIA#5530) Signed-off-by: Min Htet Myet <mattralminn@email.com> Signed-off-by: Mattral <mattralminn@gmail.com>
…aths (NVIDIA#5530) Signed-off-by: Min Htet Myet <mattralminn@email.com> Signed-off-by: Mattral <mattralminn@gmail.com> Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
Summary
get_grad_norm_fp32crashes when called with an empty gradient list,which occurs in practice when all parameters on a rank are filtered out
(frozen layers, shared params, TP duplicates). Two out of three norm-type
branches are affected; the L2 branch already has a correct guard.
Changes
megatron/core/optimizer/clip_grads.pyinfnorm (line 95)max()over empty generator →ValueErrordefault=torch.tensor(0.0)tomax()total_normstaysfloat 0.0;all_reducereceives a non-Tensor →TypeErrortotal_norm = torch.zeros(1, dtype=torch.float, device='cuda')before loopNo change to the L2 path or any non-empty-list behaviour.
Testing
tests/unit_tests/optimizer/test_clip_grads.pycovering empty-list calls for all three
norm_typepaths (inf, 2.0,custom p). Tests run without GPU using the local fallback
implementations.
Checklist
git commit -s)mainFixes #5529