[4/n] [c10d] Introduce the multi-tenancy feature in TCPStore #58331

cbalioglu · 2021-05-14T21:58:46Z

Stack from ghstack:

[4/n] [c10d] Introduce the multi-tenancy feature in TCPStore #58331 [4/n] [c10d] Introduce the multi-tenancy feature in TCPStore
[3/n] [c10d] Revise the implementation of TCPStore #58330 [3/n] [c10d] Revise the implementation of TCPStore
[2/n] [c10d] Introduce the 'multiTenant' constructor parameter in TCPStore #58329 [2/n] [c10d] Introduce the 'multiTenant' constructor parameter in TCPStore
[1/n] [c10d] Introduce a new TCPStore constructor #58328 [1/n] [c10d] Introduce a new TCPStore constructor

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the TCPStore class allowing two server stores to be instantiated with the same host:port pair.

Differential Revision: D28453850

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

facebook-github-bot · 2021-05-14T21:59:01Z

💊 CI failures summary and remediations

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

3/3 failures possibly* introduced in this PR
- 1/3 non-scanned failure(s)

2 failures not recognized by patterns:

Job	Step	Action
^{pytorch_linux_bionic_py3_8_gcc9_coverage_test2}	^{Run tests}	🔁 rerun
^{binary_linux_libtorch_3_7m_cpu_gcc5_4_cxx11-abi_shared-with-deps_build}	^{Spin up environment}	🔁 rerun

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) ghstack-source-id: 129044614 Pull Request resolved: #58331

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 129087844 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

mrshenli

I haven't fully thought through the concurrency implications. After we allow multi-tenancy, do we need to worry about concurrent calls on TCPStore::* APIs?

torch/lib/c10d/test/TCPStoreTest.cpp

torch/lib/c10d/TCPStore.cpp

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 129973360 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 129995626 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 130522028 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

rohan-varma

LGTM, thanks! We will finally be able to initialize RPC and c10d without having to open an additional port, and make the user experience better.

rohan-varma · 2021-06-03T19:50:03Z

test/distributed/test_c10d_common.py

+        # Need to store in an unused variable here to ensure the first
+        # object is not destroyed before the second object is created.
+        store1 = c10d.TCPStore(addr, port, 1, True, multi_tenant=True)  # type: ignore[call-arg] # noqa: F841
+        store2 = c10d.TCPStore(addr, port, 1, True, multi_tenant=True)  # type: ignore[call-arg] # noqa: F841


Can we add assertions such as a set by store 1 can be accessed via a get from store2?

Sure, I have an assertion for that in the C++ test, but I can add it here as well.

rohan-varma · 2021-06-03T19:53:04Z

torch/lib/c10d/TCPStore.cpp


-  auto daemon = std::make_unique<TCPStoreMasterDaemon>(std::move(socket));
+    cachedServers_.emplace(server->port(), server);


Do we need this to be a map instead of a single cached server because we want to support TCPStore::start() with multiple ports? Curious if we use that feature anywhere or if there are any use cases for it.

Yes, we need a map because there can be multiple TCPStore instances using different ports. For instance:

s1 = TCPStore("localhost", port=1234, multi_tenant=True) s2 = TCPStore("localhost", port=1234, multi_tenant=True) s3 = TCPStore("localhost", port=5678, multi_tenant=True) s4 = TCPStore("localhost", port=5678, multi_tenant=True)

Besides being pedantically correct (since we are offering an API and should not make any assumptions about how our types are going to be used), one use case that comes to my mind where this can be useful is TorchElastic. There we can use two multi-tenant TCPStore instances, one for the rendezvous itself, and one for the worker. This way we can avoid unnecessarily allocating a second port.

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 130607854 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 130662972 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/) [ghstack-poisoned]

Pull Request resolved: #58331 This PR is the final part of a stack that addresses the GitHub issue #41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 130676394 Differential Revision: [D28453850](https://our.internmc.facebook.com/intern/diff/D28453850/)

facebook-github-bot · 2021-06-05T14:50:29Z

This pull request has been merged in 1d9c1cc.

…#58331) Summary: Pull Request resolved: pytorch#58331 This PR is the final part of a stack that addresses the GitHub issue pytorch#41614; it introduces the multi-tenancy feature to the `TCPStore` class allowing two server stores to be instantiated with the same host:port pair. ghstack-source-id: 130676394 Test Plan: - Run the existing and newly-introduced tests. - Run several smoke tests including the short code snippet referred in GitHub issue pytorch#41614. Reviewed By: H-Huang Differential Revision: D28453850 fbshipit-source-id: f9066b164305de0f8c257e9d5736e93fd7e21ec6

cbalioglu requested review from H-Huang, mingzhe09088, mrshenli, pritamdamania87, rohan-varma, wayi1 and zhaojuanmao as code owners May 14, 2021 21:58

facebook-github-bot added the cla signed label May 14, 2021

cbalioglu mentioned this pull request May 14, 2021

[1/n] [c10d] Introduce a new TCPStore constructor #58328

Closed

facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label May 14, 2021

This was referenced May 14, 2021

[2/n] [c10d] Introduce the 'multiTenant' constructor parameter in TCPStore #58329

Closed

[3/n] [c10d] Revise the implementation of TCPStore #58330

Closed

mrshenli reviewed May 24, 2021

View reviewed changes

torch/lib/c10d/test/TCPStoreTest.cpp Show resolved Hide resolved

torch/lib/c10d/TCPStore.cpp Outdated Show resolved Hide resolved

torch/lib/c10d/TCPStore.cpp Show resolved Hide resolved

cbalioglu requested a review from mrshenli May 26, 2021 23:12

rohan-varma approved these changes Jun 3, 2021

View reviewed changes

H-Huang approved these changes Jun 4, 2021

View reviewed changes

facebook-github-bot closed this in 1d9c1cc Jun 5, 2021

facebook-github-bot added the Merged label Jun 5, 2021

cbalioglu mentioned this pull request Jun 5, 2021

torch.distributed and RPC cannot both be initialized with the same host:port pair. #41614

Closed

facebook-github-bot deleted the gh/cbalioglu/36/head branch June 9, 2021 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4/n] [c10d] Introduce the multi-tenancy feature in TCPStore #58331

[4/n] [c10d] Introduce the multi-tenancy feature in TCPStore #58331


		auto daemon = std::make_unique<TCPStoreMasterDaemon>(std::move(socket));
		cachedServers_.emplace(server->port(), server);

[4/n] [c10d] Introduce the multi-tenancy feature in TCPStore #58331

[4/n] [c10d] Introduce the multi-tenancy feature in TCPStore #58331

Conversation

💊 CI failures summary and remediations

2 failures not recognized by patterns:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment