Add master to RPC test #27776
Closed
Add master to RPC test #27776
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
I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Oct 11, 2019
I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) ghstack-source-id: 91786275 Pull Request resolved: #27776
xush6528
added
the
module: rpc
I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Oct 11, 2019
Pull Request resolved: #27776 I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) ghstack-source-id: 91799723
I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) [ghstack-poisoned]
I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Oct 16, 2019
Pull Request resolved: #27776 I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: [D5445858](https://our.internmc.facebook.com/intern/diff/D5445858/) ghstack-source-id: 91988737
mrshenli
reviewed
Oct 16, 2019
| assert fut.wait() is None, "Sending termination signal failed." | ||
|
|
||
| # Close RPC. | ||
| rpc.join_rpc() |
There was a problem hiding this comment.
join_rpc here still does termination detection, so that we don't know if the above master-based termination would work or not (I think it could still leak futures, no?). Shall we replace this join_rpc with a local shutdown?
There was a problem hiding this comment.
We don't need to delete join_rpc implementation in this PR, just add a local shutdown and let's see if it would work.
There was a problem hiding this comment.
That's my plan.
After
- this PR has merged.
- local shutdown API is ready.
I will change this to shutdown.
This PR works, because there is another RpcAgent which implements join as shutdown, and the test_asymmetric_load is failing for that RpcAgent. This PR fix the test, proving it works.
There was a problem hiding this comment.
I see. That makes sense. I am a little worried if the flakiness in the other RpcAgent is due to leaking futures. Regardless, let's get this in first, then we can fix flakiness, and drop join_rpc().
|
This pull request has been merged in 3523e54. |
xush6528
added a commit
that referenced
this pull request
Oct 17, 2019
This is a followup for #27776. Especially the comment, #27776 (comment). The plan is as follow. - Add local stop API to RpcAgent. - Change unit tests to use stop rather than join. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Oct 17, 2019
This is a followup for #27776. Especially the comment, #27776 (comment). The plan is as follow. - Add local stop API to RpcAgent. - Change unit tests to use stop rather than join. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) ghstack-source-id: 92120728 Pull Request resolved: #28238
xush6528
added a commit
that referenced
this pull request
Oct 17, 2019
This is a followup for #27776, especially the comment, #27776 (comment). The plan is as follow. - Add local stop API to RpcAgent. - Change unit tests to use stop rather than join. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Oct 17, 2019
Pull Request resolved: #28238 This is a followup for #27776, especially the comment, #27776 (comment). The plan is as follow. - Add local stop API to RpcAgent. - Change unit tests to use stop rather than join. ghstack-source-id: 92132935 Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/)
thiagocrepaldi
pushed a commit
to thiagocrepaldi/pytorch
that referenced
this pull request
Feb 4, 2020
Summary: Pull Request resolved: pytorch#27776 I think it’s not worth it to equip other `RPCAgent` with collective communication capability, i.e. 1) have GLOO contained in `RPCAgent`, or 2) Implemented ::barrier() and ::drain() based on RPC messaging. The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py. I think having those unit tests to have a master is simpler than equipping `RPCAgent` with collective communication capability. Differential Revision: D5445858 fbshipit-source-id: 56ee24703abd8c5b366829430bef657e0f1dfeba
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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:
I think it’s not worth it to equip other
RPCAgentwith collective communication capability, i.e. 1) have GLOO contained inRPCAgent, or 2) Implemented ::barrier() and ::drain() based on RPC messaging.The only use case that does not have a master is the OSS unit test suite, caffe2/test/rpc_test.py.
I think having those unit tests to have a master is simpler than equipping
RPCAgentwith collective communication capability.Differential Revision: D5445858