[NCCL] Support Wait Timeout in ProcessGroupNCCL #40946
Closed
Conversation
This file contains hidden or 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
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
This was referenced Jul 2, 2020
💊 CI failures summary and remediationsAs of commit 19f3fa1 (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 3 fixed upstream failures:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
mrshenli
reviewed
Jul 7, 2020
|
|
||
| // Same as calling synchronize() for NCCL work. | ||
| bool wait(std::chrono::milliseconds timeout = kUnsetTimeout) override; | ||
| bool wait(std::chrono::milliseconds timeout = kNoTimeout) override; |
Contributor
There was a problem hiding this comment.
Any reason for using default timeout value for wait, but explicitly avoiding introduce an argument to ProcessGroupNCCL::WorkNCCL::synchronize()? Will default value also work for synchronize()?
Member
Author
There was a problem hiding this comment.
Briefly chatted with Hongyi about this - we were mainly aiming to change the wait API only without many other side-effects on user facing functions, so decided to keep synchronize as is. If you think it's better to have a timeout with default value for synchronize, we can do that as well.
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
jiayisuse
approved these changes
Jul 15, 2020
| void ProcessGroupNCCL::WorkNCCL::synchronize() { | ||
| // Call Synchronize without a timeout. We use this method to avoid adding a | ||
| // timeout argument to the public synchronize API. | ||
| synchronizeInternal(kNoTimeout); |
Contributor
There was a problem hiding this comment.
Thanks for making synchronizeInternal() accepts explicit timeout
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown. Differential Revision: [D22127898](https://our.internmc.facebook.com/intern/diff/D22127898/) [ghstack-poisoned]
Contributor
|
This pull request has been merged in edf3dc7. |
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:
Adds timeout to ProcessGroupNCCL::wait. Currently, WorkNCCL objects already have a timeout set during ProcessGroupNCCL construction. The new wait function will override the existing timeout with the user-defined timeout if one is provided. Timed out operations result in NCCL communicators being aborted and an exception being thrown.
Differential Revision: D22127898