Skip to content

fix: fixing fsdp_manual_registration logic in perf script#2223

Merged
youngeunkwon0405 merged 1 commit intomainfrom
fix-script-ubr-logic
Feb 5, 2026
Merged

fix: fixing fsdp_manual_registration logic in perf script#2223
youngeunkwon0405 merged 1 commit intomainfrom
fix-script-ubr-logic

Conversation

@youngeunkwon0405
Copy link
Copy Markdown
Contributor

@youngeunkwon0405 youngeunkwon0405 commented Feb 4, 2026

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Changelog

  • Add specific line by line info of high level changes in this PR.

GitHub Actions CI

See the CI sectionin the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

If you haven't finished some of the above items you can still open "Draft" PR.

Additional Information

  • Related to # (issue)

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a performance configuration logic issue where a setting was being applied in unintended scenarios. The setting now correctly applies only when both required conditions are met.

Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
@youngeunkwon0405 youngeunkwon0405 added the bug Something isn't working label Feb 4, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Feb 4, 2026

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.

@youngeunkwon0405 youngeunkwon0405 enabled auto-merge (squash) February 4, 2026 22:57
@youngeunkwon0405
Copy link
Copy Markdown
Contributor Author

/ok to test 873d4e8

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

A single conditional check in _set_nccl_ub_overrides was tightened to enable fsdp_manual_registration only when both Megatron FSDP is in use and NCCL UB is enabled, instead of whenever Megatron FSDP was in use regardless of NCCL UB status.

Changes

Cohort / File(s) Summary
NCCL UB Override Logic
scripts/performance/utils/overrides.py
Tightened conditional in _set_nccl_ub_overrides to add NCCL UB enablement check alongside Megatron FSDP check before setting fsdp_manual_registration.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • Fix: perf script ddp nccl-ub #2158: Touches the same file and function, specifically related to NCCL UB override logic and Megatron FSDP/fsdp_manual_registration interaction.

Suggested reviewers

  • sanandaraj5597
  • malay-nagda
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing fsdp_manual_registration logic in the performance script by tightening a conditional check.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Test Results For Major Changes ✅ Passed Only 1 line modified correcting conditional logic in performance utilities. Bug fix qualifies as minor change per criteria.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-script-ubr-logic

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youngeunkwon0405 youngeunkwon0405 merged commit 61f3c46 into main Feb 5, 2026
50 checks passed
@youngeunkwon0405 youngeunkwon0405 deleted the fix-script-ubr-logic branch February 5, 2026 04:09
sowmen pushed a commit to sowmen/Megatron-Bridge that referenced this pull request Feb 11, 2026
…o#2223)

Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
Signed-off-by: sowmen <sowmendipta@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants