Use Blocking Wait if both Blocking Wait and Async Error Handling Are Set #47926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Given that we're soon enabling async error handling in PET, we should make the behavior explicit when users have set NCCL_BLOCKING_WAIT in their own code while also using PET. This PR essentially gives blocking wait precedence (for now). This way the blast radius of the PET change is smaller, while we continue working with blocking wait users and discussing whether moving to async error handling may be a good fit. Differential Revision: [D24928149](https://our.internmc.facebook.com/intern/diff/D24928149/) [ghstack-poisoned]
osalpekar
requested review from
mingzhe09088,
mrshenli,
pietern,
pritamdamania87,
rohan-varma and
zhaojuanmao
as code owners
facebook-github-bot
added
cla signed
oncall: distributed
osalpekar
added a commit
that referenced
this pull request
Nov 13, 2020
Given that we're soon enabling async error handling in PET, we should make the behavior explicit when users have set NCCL_BLOCKING_WAIT in their own code while also using PET. This PR essentially gives blocking wait precedence (for now). This way the blast radius of the PET change is smaller, while we continue working with blocking wait users and discussing whether moving to async error handling may be a good fit. Differential Revision: [D24928149](https://our.internmc.facebook.com/intern/diff/D24928149/) ghstack-source-id: 116553583 Pull Request resolved: #47926
💊 CI failures summary and remediationsAs of commit 675f47a (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 1 failure confirmed as flaky and can be ignored:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 2 times. |
Codecov Report
@@ Coverage Diff @@
## gh/osalpekar/105/base #47926 +/- ##
=========================================================
- Coverage 81.25% 81.25% -0.01%
=========================================================
Files 1838 1838
Lines 198256 198256
=========================================================
- Hits 161098 161097 -1
- Misses 37158 37159 +1 |
|
This pull request has been merged in 5d51b63. |
tugsbayasgalan
pushed a commit
to tugsbayasgalan/pytorch
that referenced
this pull request
Nov 16, 2020
…Set (pytorch#47926) Summary: Pull Request resolved: pytorch#47926 Given that we're soon enabling async error handling in PET, we should make the behavior explicit when users have set NCCL_BLOCKING_WAIT in their own code while also using PET. This PR essentially gives blocking wait precedence (for now). This way the blast radius of the PET change is smaller, while we continue working with blocking wait users and discussing whether moving to async error handling may be a good fit. ghstack-source-id: 116553583 Test Plan: Simple FBL run/CI Reviewed By: jiayisuse Differential Revision: D24928149 fbshipit-source-id: d42c038ad44607feb3d46dd65925237c564ff7a3
Comment on lines
+456
to
+461
| if (blockingWait_ && asyncErrorHandling_) { | ||
| LOG(INFO) << "[Rank " << rank_ | ||
| << "] NCCL_BLOCKING_WAIT and NCCL_ASYNC_ERROR_HANDLING " | ||
| << "should not both be enabled. " | ||
| << "Only NCCL_BLOCKING_WAIT is being used in this process."; | ||
| asyncErrorHandling_ = false; |
There was a problem hiding this comment.
@osalpekar Sorry a bit late on this, but I think we should instead throw an error if both NCCL_BLOCKING_WAIT and NCCL_ASYNC_ERROR_HANDLING is set and ask user to set only one. It is usually confusing for users if we have such behavior where we unset one option. User's might not look closely at the logs and would feel that there might be a bug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Given that we're soon enabling async error handling in PET, we should make the behavior explicit when users have set NCCL_BLOCKING_WAIT in their own code while also using PET. This PR essentially gives blocking wait precedence (for now). This way the blast radius of the PET change is smaller, while we continue working with blocking wait users and discussing whether moving to async error handling may be a good fit.
Fixes: #47943
Differential Revision: D24928149