[rpc] allow ability to abort second call to RecvWork::wait() in ProcessGroupAgent::listenLoop #36084
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
…ssGroupAgent::listenLoop Added support to abort the call to a `RecvWork` created by `recvAnysource` but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)! [ghstack-poisoned]
rohan-varma
requested review from
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
…() in ProcessGroupAgent::listenLoop" ungraceful shutdown Added support to abort the call to a `RecvWork` created by `recvAnysource` but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)! [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Apr 6, 2020
…ssGroupAgent::listenLoop Pull Request resolved: #36084 Added support to abort the call to a `RecvWork` created by `recvAnysource` but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. ghstack-source-id: 101604969 Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)!
💊 CircleCI build failures summary and remediationsAs of commit 7df8436 (more details on the Dr. CI page):
🕵️ 2 new failures recognized by patternsThe following build failures do not appear to be due to upstream breakages:
|
…() in ProcessGroupAgent::listenLoop" ungraceful shutdown ungraceful shutdown #30330 added support to abort the call to a `RecvWork` created by `recvAnysource` but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)! [ghstack-poisoned]
…() in ProcessGroupAgent::listenLoop" ungraceful shutdown ungraceful shutdown ungraceful shutdown #30330 added support to abort the call to a `RecvWork` created by `recvAnysource` but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)! [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Apr 7, 2020
…ssGroupAgent::listenLoop Pull Request resolved: #36084 #30330 added support to abort the call to a `RecvWork` created by `recvAnysource`, but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. ghstack-source-id: 101645227 Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)!
…() in ProcessGroupAgent::listenLoop" ungraceful shutdown ungraceful shutdown ungraceful shutdown ungraceful shutdown #30330 added support to abort the call to a `RecvWork` created by `recvAnysource` but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)! [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Apr 7, 2020
…ssGroupAgent::listenLoop Pull Request resolved: #36084 #30330 added support to abort the call to a `RecvWork` created by `recvAnysource`, but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. ghstack-source-id: 101689402 Differential Revision: [D20632764](https://our.internmc.facebook.com/intern/diff/D20632764/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D20632764/)!
jjlilley
approved these changes
Apr 7, 2020
Contributor
|
This pull request has been merged in fc5d658. |
ashishfarmer
pushed a commit
to ashishfarmer/pytorch
that referenced
this pull request
Apr 13, 2020
…ssGroupAgent::listenLoop (pytorch#36084) Summary: Pull Request resolved: pytorch#36084 pytorch#30330 added support to abort the call to a `RecvWork` created by `recvAnysource`, but there is an additional call to `pg_->recv()` to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in `::shutdown()`, which can be used to avoid hangs during ungraceful shutdown. Added an internal test case in `ProcessGroupAgentTest` to ensure that an appropriate error message is raised when this happens. ghstack-source-id: 101689402 Test Plan: Added test in ProcessGroupAgentTest. We also add a basic config that allows us to control whether to abort the call to `pg->recv()` and `pg->recvAnysource()` in `FailingWaitProcessGroupGloo`. Run test binary: ```buck build mode/dev-nosan //caffe2/torch/fb/distributed/thriftRpcBackend/test:ProcessGroupAgentTest --keep-going ~/fbcode/buck-out/gen/caffe2/torch/fb/distributed/thriftRpcBackend/test/ProcessGroupAgentTest ``` P128567144 Differential Revision: D20632764 fbshipit-source-id: c0b3c391fd3e0ae711661ad99f309ee4d93f6582
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:
ungraceful shutdown
ungraceful shutdown
ungraceful shutdown
ungraceful shutdown
#30330 added support to abort the call to a
RecvWorkcreated byrecvAnysourcebut there is an additional call topg_->recv()to actually get the tensor sent over the wire (the previous call is the preamble for the tensor). This adds support to be able to abort this call as well in::shutdown(), which can be used to avoid hangs during ungraceful shutdown.Added an internal test case in
ProcessGroupAgentTestto ensure that an appropriate error message is raised when this happens.Differential Revision: D20632764
NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!