Skip to content

test(clip_grads): add regression coverage for get_grad_norm_fp32 - #5828

Open
Mattral wants to merge 3 commits into
NVIDIA:mainfrom
Mattral:test/clip-grads-empty-grads-coverage
Open

test(clip_grads): add regression coverage for get_grad_norm_fp32#5828
Mattral wants to merge 3 commits into
NVIDIA:mainfrom
Mattral:test/clip-grads-empty-grads-coverage

Conversation

@Mattral

@Mattral Mattral commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #5530. test_clip_grads.py had no test coverage for
get_grad_norm_fp32 itself — this adds regression tests for all three
norm_type branches, including the empty-grads_for_norm case that #5530 fixed.

Changes

tests/unit_tests/optimizer/test_clip_grads.py

Testing

  • Verified locally via linting/formatting (black, isort, pylint, ruff
    all clean via tools/autoformat.sh)
  • Could not execute pytest locally (no GPU available in this dev environment,
    and every test in this suite requires torch.distributed initialization);
    relying on CI's GPU runners for actual execution
  • No changes to megatron/core/optimizer/clip_grads.py — test-only PR

Checklist

  • Commits signed off (git commit -s)
  • Rebased on main
  • Test-only change, no production code touched

Fixes #5827

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>
@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@svcnvidia-nemo-ci
svcnvidia-nemo-ci marked this pull request as draft July 15, 2026 10:25
@github-actions

Copy link
Copy Markdown
Contributor

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:

  1. Add the oncall reviewer (optional reviewer)
  2. Add required review teams based on your changes

See the contribution guide for more details.

@Mattral
Mattral marked this pull request as ready for review July 15, 2026 10:30
@svcnvidia-nemo-ci
svcnvidia-nemo-ci requested a review from a team July 15, 2026 10:30
@dimapihtar
dimapihtar requested a review from gautham-kollu July 16, 2026 18:53
@dimapihtar

Copy link
Copy Markdown
Contributor

/ok to test 254c131

@guihong-nv

Copy link
Copy Markdown
Contributor

@Mattral please fix the failing tests.

@Mattral

Mattral commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the ping

I'll investigate the unit test failure (want to confirm whether it's related to this change or a pre-existing issue in the suite) and push a fix by Monday.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Jul 24, 2026
Signed-off-by: Mattral <mattralminn@gmail.com>
auto-merge was automatically disabled July 25, 2026 11:31

Head branch was pushed to by a user without write access

@Mattral
Mattral force-pushed the test/clip-grads-empty-grads-coverage branch from 254c131 to c43b3a0 Compare July 25, 2026 11:31
@Mattral

Mattral commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

@guihong-nv . Found the root cause: the test assumed a single rank, but CI runs unit tests with --nproc-per-node 8. Every rank seeds identically and computes the same local grads, so the SUM-based all_reduce (grad_stats_parallel_group=None → WORLD group) correctly multiplies the result by world_size, the inf-norm case uses MAX instead, which is why only [2.0] and [1.0] failed. Pushed a fix that accounts for world_size in the test's expected value, watching CI now to confirm.

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-customer Waiting on the original author to respond label Jul 25, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Jul 27, 2026
@Mattral

Mattral commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Hi @guihong-nv

following up here. The fix landed last week (test now accounts for world_size in the SUM vs MAX all-reduce comparison). I notice a couple of checks are still pending

let me know if those need an approval on your end, or if there's anything else you'd like adjusted before another look.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Aug 4, 2026
@guihong-nv

Copy link
Copy Markdown
Contributor

@Mattral may you please resolve the conflict here?

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Aug 25, 2026
@Mattral

Mattral commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@guihong-nv
Resolved, kept both: my get_grad_norm_fp32 regression tests and your TestCountZerosFp32GtpPadding class, merged cleanly with no overlap in what they cover. Ready for another look.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add regression test coverage for get_grad_norm_fp32 (norm_type paths + empty grads_for_norm)

6 participants