Allow RPC to be initialized again after shutdown. #42723

pritamdamania87 · 2020-08-07T02:57:51Z

Stack from ghstack:

Allow RPC to be initialized again after shutdown. #42723 Allow RPC to be initialized again after shutdown.

This PR is addressing #39340
and allows users to initialize RPC again after shutdown. Major changes in the
PR include:

Change to DistAutogradContainer to support this.
Ensure PythonRpcHandler is reinitialized appropriately.
Use PrefixStore in RPC initialization to ensure each new init_rpc uses a
different prefix.

Differential Revision: D22993909

This PR is addressing #39340 and allows users to initialize RPC again after shutdown. Major changes in the PR include: 1. Change to DistAutogradContainer to support this. 2. Ensure PythonRpcHandler is reinitialized appropriately. 3. Use PrefixStore in RPC initialization to ensure each new `init_rpc` uses a different prefix. Differential Revision: [D22993909](https://our.internmc.facebook.com/intern/diff/D22993909/) [ghstack-poisoned]

This PR is addressing #39340 and allows users to initialize RPC again after shutdown. Major changes in the PR include: 1. Change to DistAutogradContainer to support this. 2. Ensure PythonRpcHandler is reinitialized appropriately. 3. Use PrefixStore in RPC initialization to ensure each new `init_rpc` uses a different prefix. Differential Revision: [D22993909](https://our.internmc.facebook.com/intern/diff/D22993909/) ghstack-source-id: 109412898 Pull Request resolved: #42723

dr-ci · 2020-08-07T03:46:33Z

💊 CI failures summary and remediations

As of commit 1454298 (more details on the Dr. CI page):

1/2 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)
1/2 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_linux_xenial_py3_clang5_asan_test1 (1/1)

Step: "Download Docker image" (full log | diagnosis details | 🔁 rerun) ❄️

Fp2ts8glJWkyKdJ%2Fdcpiq3DQ6RoGbvt%2BtLdsuXvbVjFDT87vgHEtFbTYVMb7olX%2F5NFIYduDD649sXe%2Bpu4fXPR3QjrFdZG3R%2FQ2YinyrPUtXh9iGKOK5SoodAThNGyjLf4XpdnckgGvul9qI3jPE5LhH7%2F1iYZ3PHtbaA5UiZwlw2g%3D%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20200813T021149Z&X-Amz-SignedHeaders=host&X-Amz-Expires=3600&X-Amz-Credential=ASIAYTIFIPBEB35VPAXL%2F20200813%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Signature=e679487418366d9f2f0b3a454928a73457349da9d1aef252f081ae88d0e3cdef: dial tcp 52.216.65.69:443: i/o timeout

DOCKER_IMAGE: 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-py3-clang5-asan:a57b8c8191ee0d61aff57300eca0ae1f26c749bb-14542984039f09ddf816336fa65300d4cb13388c 
Parallel backend flags:  
p2ts8glJWkyKdJ%2Fdcpiq3DQ6RoGbvt%2BtLdsuXvbVjFDT87vgHEtFbTYVMb7olX%2F5NFIYduDD649sXe%2Bpu4fXPR3QjrFdZG3R%2FQ2YinyrPUtXh9iGKOK5SoodAThNGyjLf4XpdnckgGvul9qI3jPE5LhH7%2F1iYZ3PHtbaA5UiZwlw2g%3D%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20200813T021149Z&X-Amz-SignedHeaders=host&X-Amz-Expires=3600&X-Amz-Credential=ASIAYTIFIPBEB35VPAXL%2F20200813%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Signature=e679487418366d9f2f0b3a454928a73457349da9d1aef252f081ae88d0e3cdef: dial tcp 52.216.65.69:443: i/o timeout

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-xenial-rocm3.5.1-py3.6

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.

See how this bot performed.

This comment has been revised 13 times.

rohan-varma · 2020-08-07T21:12:00Z

torch/csrc/distributed/autograd/context/container.cpp

  TORCH_CHECK(
-      !container.initialized_,
-      "Container is already initialized! Cannot initialize it twice!");
+      !container.initialized_ || (worker_id == container.worker_id_),


Does this mean if we shut down RPC, and then re-init, we can't change the passed in rank? I don't think there's a reason to do so, but just confirming.

Yes, that is correct. I think it is probably safer to not allow rank changes and its easier to also just reuse the same container.

rohan-varma

Looks good, had a couple of questions and there are some test failures.

rohan-varma · 2020-08-07T21:18:29Z

torch/csrc/distributed/rpc/python_rpc_handler.cpp

-  typeParser_ = std::make_shared<jit::ScriptTypeParser>(
-      std::make_shared<PythonTypeResolver>());
+void PythonRpcHandler::init() {
+  if (!initialized_) {


I'm wondering what's the motivation for this atomic and if it's to prevent races? If there could be thread races, this atomic won't be enough to ensure initialization only once right? If there's no races, why do we need an atomic?

This function can probably be executed twice and that is safe and would happen rarely. Apart from that, the atomic is mostly to act as a memory barrier to ensure other threads are informed about this change. For example, if one thread calls cleanup and another thread calls getInstance after it, if we don't use an atomic the thread calling getInstance might still see the old value of initialized.

This PR is addressing #39340 and allows users to initialize RPC again after shutdown. Major changes in the PR include: 1. Change to DistAutogradContainer to support this. 2. Ensure PythonRpcHandler is reinitialized appropriately. 3. Use PrefixStore in RPC initialization to ensure each new `init_rpc` uses a different prefix. Differential Revision: [D22993909](https://our.internmc.facebook.com/intern/diff/D22993909/) [ghstack-poisoned]

Pull Request resolved: #42723 This PR is addressing #39340 and allows users to initialize RPC again after shutdown. Major changes in the PR include: 1. Change to DistAutogradContainer to support this. 2. Ensure PythonRpcHandler is reinitialized appropriately. 3. Use PrefixStore in RPC initialization to ensure each new `init_rpc` uses a different prefix. ghstack-source-id: 109478799 Differential Revision: [D22993909](https://our.internmc.facebook.com/intern/diff/D22993909/)

rohan-varma

LGTM!

This PR is addressing #39340 and allows users to initialize RPC again after shutdown. Major changes in the PR include: 1. Change to DistAutogradContainer to support this. 2. Ensure PythonRpcHandler is reinitialized appropriately. 3. Use PrefixStore in RPC initialization to ensure each new `init_rpc` uses a different prefix. Differential Revision: [D22993909](https://our.internmc.facebook.com/intern/diff/D22993909/) [ghstack-poisoned]

Pull Request resolved: #42723 This PR is addressing #39340 and allows users to initialize RPC again after shutdown. Major changes in the PR include: 1. Change to DistAutogradContainer to support this. 2. Ensure PythonRpcHandler is reinitialized appropriately. 3. Use PrefixStore in RPC initialization to ensure each new `init_rpc` uses a different prefix. ghstack-source-id: 109805368 Differential Revision: [D22993909](https://our.internmc.facebook.com/intern/diff/D22993909/)

facebook-github-bot · 2020-08-14T05:46:20Z

This pull request has been merged in 89b0b3b.

rohan-varma · 2020-11-16T06:42:15Z

torch/testing/_internal/distributed/rpc/rpc_test.py


+    @dist_init(setup_rpc=False)
+    def test_init_rpc_twice(self):
+        initialize_pg(self.init_method, self.rank, self.world_size)


It looks like we missed testing this, though it looks like this test fails depending on the init_method. For example if users want to init RPC twice with TCPStore, it currently does not work (see details in #46491).

pritamdamania87 requested review from mrshenli and zhaojuanmao as code owners August 7, 2020 02:57

pritamdamania87 requested a review from rohan-varma August 7, 2020 02:58

rohan-varma reviewed Aug 7, 2020

View reviewed changes

pritamdamania87 requested a review from rohan-varma August 11, 2020 04:28

rohan-varma approved these changes Aug 11, 2020

View reviewed changes

facebook-github-bot closed this in 89b0b3b Aug 14, 2020

facebook-github-bot added the merged label Aug 14, 2020

facebook-github-bot deleted the gh/pritamdamania87/151/head branch August 17, 2020 14:16

mruberry added the Merged label Oct 28, 2020

rohan-varma reviewed Nov 16, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow RPC to be initialized again after shutdown. #42723

Allow RPC to be initialized again after shutdown. #42723

Uh oh!

pritamdamania87 commented Aug 7, 2020 •

edited

Loading

Uh oh!

dr-ci bot commented Aug 7, 2020 •

edited

Loading

Uh oh!

rohan-varma Aug 7, 2020

Uh oh!

pritamdamania87 Aug 11, 2020

Uh oh!

rohan-varma left a comment

Uh oh!

rohan-varma Aug 7, 2020

Uh oh!

pritamdamania87 Aug 11, 2020

Uh oh!

rohan-varma left a comment

Uh oh!

facebook-github-bot commented Aug 14, 2020

Uh oh!

rohan-varma Nov 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Allow RPC to be initialized again after shutdown. #42723

Allow RPC to be initialized again after shutdown. #42723

Uh oh!

Conversation

pritamdamania87 commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

❄️ 1 failure tentatively classified as flaky

pytorch_linux_xenial_py3_clang5_asan_test1 (1/1)

ci.pytorch.org: 1 failed

Uh oh!

rohan-varma Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

pritamdamania87 Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

rohan-varma left a comment

Choose a reason for hiding this comment

Uh oh!

rohan-varma Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

pritamdamania87 Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

rohan-varma left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 14, 2020

Uh oh!

rohan-varma Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pritamdamania87 commented Aug 7, 2020 •

edited

Loading

dr-ci bot commented Aug 7, 2020 •

edited

Loading