Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix NVTE_UB_WITH_MPI read #1194

Merged
merged 3 commits into from
Sep 25, 2024
Merged

fix NVTE_UB_WITH_MPI read #1194

merged 3 commits into from
Sep 25, 2024

Conversation

erhoo82
Copy link
Collaborator

@erhoo82 erhoo82 commented Sep 22, 2024

Description

Fix reading NVTE_UB_WITH_MPI env var.

Type of change

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Infra/Build change
  • Code refractor

Changes

  • Fix reading str to reading as bool

Checklist:

  • I have read and followed the contributing guidelines
  • The functionality is complete
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@erhoo82
fix NVTE_UB_WITH_MPI read
8250007 
Signed-off-by: Sangkug Lym <[email protected]>
ksivaman
ksivaman approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@ksivaman ksivaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ksivaman
Copy link
Member

/te-ci pytorch

denera
denera approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@denera denera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm guessing you're trying to handle the case where NVTE_UB_WITH_MPI=0 in the environment?

@erhoo82
Copy link
Collaborator Author

erhoo82 commented Sep 25, 2024

LGTM. I'm guessing you're trying to handle the case where NVTE_UB_WITH_MPI=0 in the environment?

Yes, correct. We plan to use NCCL bootstrapping from nemo 24.09 container.

@erhoo82 erhoo82 merged commit 209b8e5 into NVIDIA:main Sep 25, 2024
14 of 15 checks passed
ptrendx pushed a commit that referenced this pull request Sep 27, 2024
@erhoo82 @ksivaman @ptrendx
fix NVTE_UB_WITH_MPI read (#1194)
c328071 
* fix NVTE_UB_WITH_MPI read

Signed-off-by: Sangkug Lym <[email protected]>

* Add default value

Signed-off-by: Sangkug Lym <[email protected]>

---------

Signed-off-by: Sangkug Lym <[email protected]>
Co-authored-by: Kirthi Shankar Sivamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksivaman ksivaman ksivaman approved these changes

@denera denera denera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erhoo82 @ksivaman @denera