[1/n][torch/elastic][upstream] Move torchelastic/rendezvous to torch/distributed/rendezvous (#141) #53172

kiukchung · 2021-03-03T06:42:40Z

Summary:
Pull Request resolved: pytorch/elastic#141

Upstreams two modules to torch:

torchelastic.rendezvous
torchelastic.utils

These modules were chosen as [1/n] since they are the leaf modules in torchelastic.

NOTE: This diff is NOT ready to land - I still have a few dependencies to test/codemod. torch only has a single buck target: caffe2:torch which makes this a bit tricky.

Test Plan:

buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/test/...
buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/data/test/...
buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/test/...
buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/fb/...
buck test mode/dev-nosan //pytorch/elastic/torchelastic/...

+ Sandcastle

Differential Revision: D26718746

facebook-github-bot · 2021-03-03T06:42:46Z

💊 CI failures summary and remediations

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

3/4 failures possibly* introduced in this PR
- 3/3 non-scanned failure(s)
1/4 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_xla_linux_bionic_py3_6_clang9_test (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun) ❄️

Mar 05 01:23:02 urllib.error.HTTPError: HTTP Error 403: Forbidden

Mar 05 01:23:02   File "/opt/conda/lib/python3.6/urllib/request.py", line 532, in open
Mar 05 01:23:02     response = meth(req, response)
Mar 05 01:23:02   File "/opt/conda/lib/python3.6/urllib/request.py", line 642, in http_response
Mar 05 01:23:02     'http', request, response, code, msg, hdrs)
Mar 05 01:23:02   File "/opt/conda/lib/python3.6/urllib/request.py", line 570, in error
Mar 05 01:23:02     return self._call_chain(*args)
Mar 05 01:23:02   File "/opt/conda/lib/python3.6/urllib/request.py", line 504, in _call_chain
Mar 05 01:23:02     result = func(*args)
Mar 05 01:23:02   File "/opt/conda/lib/python3.6/urllib/request.py", line 650, in http_error_default
Mar 05 01:23:02     raise HTTPError(req.full_url, code, msg, hdrs, fp)
Mar 05 01:23:02 urllib.error.HTTPError: HTTP Error 403: Forbidden
Mar 05 01:23:02 
Mar 05 01:23:02 ----------------------------------------------------------------------
Mar 05 01:23:02 Ran 1 test in 0.414s
Mar 05 01:23:02 
Mar 05 01:23:02 FAILED (errors=1)
Mar 05 01:23:03 
0it [00:00, ?it/s]
Mar 05 01:23:03 + cleanup
Mar 05 01:23:03 + retcode=1
Mar 05 01:23:03 + set +x
Mar 05 01:23:03 =================== sccache compilation log ===================

ci.pytorch.org: 2 failed

Failed: pr/caffe2-pytorch-linux-bionic-rocm4.0.1-py3.6-test
Failed: pr/pytorch-linux-bionic-rocm4.0.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 to the (internal) Dr. CI Users group.

facebook-github-bot · 2021-03-03T06:43:00Z

This pull request was exported from Phabricator. Differential Revision: D26718746

facebook-github-bot · 2021-03-03T21:04:25Z

This pull request was exported from Phabricator. Differential Revision: D26718746

facebook-github-bot · 2021-03-04T07:07:30Z

This pull request was exported from Phabricator. Differential Revision: D26718746

…distributed/rendezvous (pytorch#53172) Summary: Pull Request resolved: pytorch#53172 Pull Request resolved: pytorch/elastic#141 Upstreams two modules to torch: 1. `torchelastic.rendezvous` 2. `torchelastic.utils` These modules were chosen as `[1/n]` since they are the leaf modules in torchelastic. ==== NOTES: ==== 1. I'm disabling etcd_rendezvous and etcd_server tests in CIRCLECI for the moment since I need to edit the test dockers to contain the etcd server binary (there's 4-5 test dockers - one for each platform so this is going to take some time for me to set up the environments and test) - T85992919. 2. I've fixed all lint errors on python files but there are ones on the cpp files on the ZeusRendezvous. I took a look at them, and I don't want to fix the linter errors right now for 2 major reasons: 1. Some of them are more than formatting changes (e.g. std::move vs pass by value) and I don't want to introduce bundled changes with the move 1. The old rendezvous code (the one we forked from in caffe2/fb) has the same problems and I think its better for us to deal with this when we deprecate caffe2/fb/rendezvous in favor of the one in torchelastic -T86012579. Test Plan: ``` buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/data/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/fb/... buck test mode/dev-nosan //pytorch/elastic/torchelastic/... ``` \+ Sandcastle Differential Revision: D26718746 fbshipit-source-id: 7f832d9e1496bf3b15945cc0a1d050dda96aac8d

facebook-github-bot · 2021-03-04T19:47:11Z

This pull request was exported from Phabricator. Differential Revision: D26718746

mrshenli · 2021-03-05T02:00:04Z

test/distributed/elastic/rendezvous/etcd_rendezvous_test.py

+from torch.distributed.elastic.rendezvous.etcd_server import EtcdServer
+
+
+@unittest.skipIf(os.getenv("CIRCLECI"), "T85992919 temporarily disabling in circle ci")


is this leaking internal task id to OSS?

Maybe we can create an Github issue to track this?

mrshenli · 2021-03-05T02:05:42Z

test/distributed/elastic/utils/distributed_test.py

+                is_server=True, server_addr=socket.gethostname(), world_size=2
+            )
+
+    def test_create_store_multi(self):


nit and can be done in followup PR: The rest of distributed tests uses a MultiProcessTestCase as base class. Does it make sense if we use that base test class here?

pytorch/torch/testing/_internal/common_distributed.py

Line 266 in ac668c5

class MultiProcessTestCase(TestCase):

Thanks for pointing this out @mrshenli ! With the awesome changes that @H-Huang and @cbalioglu have added to c10d::Store, the utils/distributed.py file can go away. I'm planning to do this when I upstream the torchelastic.agent module since that's the one that is using these util methods.

For general long term fixes that come up, I'll create github issues to track them. Feel free to point them out during the upstream!

mrshenli · 2021-03-05T02:07:33Z

torch/distributed/elastic/rendezvous/etcd_rendezvous.py

+from base64 import b64decode, b64encode
+from typing import Optional
+
+import etcd  # type: ignore[import]


cc @malfet @seemethere is this dependency OK?

I've talked to @seemethere, since this is a runtime dependency - for now we won't add the dependency to torch, but we will only add it to the test Docker image (so that we can reenable these tests). Users will have to pip install etcd_client if they want to use EtcdRendezvous but we will be adding a StandaloneRendezvous with no etcd runtime dependency.

Looks like we are doing something similar to numpy (users will have to manually install that dependency if they want to use an implementation in torch that depends on numpy) so we have precedence.

mrshenli · 2021-03-05T02:09:06Z

torch/distributed/elastic/rendezvous/etcd_server.py

+log = logging.getLogger(__name__)
+
+
+def find_free_port():


(can be done in follow up PR), there is another find_free_port implementation in common_utils.py, do we need to consolidate?

pytorch/torch/testing/_internal/common_utils.py

Line 1469 in ac668c5

def find_free_port():

ahh yes I did consider it. Thing is that the one in common_utils is under the testing/_internal directory and etcd_server is technically a part of the lib. I'll create a github issue for extracting this function into a proper util-lib module and having both use that.

mrshenli

Directory structure LGTM! Only stamping for the folder structure. I didn't review rendezvous implementations.

Looks like the tests are not yet linked to CI and docs are not yet linked to docgen. I guess those will be done in followup PRs?

mrshenli · 2021-03-05T02:11:14Z

torch/distributed/elastic/rendezvous/etcd_server.py

+     server.stop()
+
+    Args:
+        etcd_binary_path: path of etcd server binary (see above for fallback path)


other API docs usually also specific the arg type, i.e., (str)

got it, docs will be reviewed as part of phase III (in mid April) so we'll make sure there is the same "look-and-feel" with the rest of torch.

mrshenli · 2021-03-05T02:15:00Z

torch/distributed/elastic/utils/data/elastic_distributed_sampler.py

+from torch.utils.data.distributed import DistributedSampler
+
+
+class ElasticDistributedSampler(DistributedSampler):


Do we plan to consolidate all distributed samplers into the same folder (torch.utils or torch.distributed)?

yea we should, it'll be done after the upstream is done and when we start "blending" the torchelastic modules to torch. The same needs to be done to multiprocessing, logging, events, utils.

facebook-github-bot · 2021-03-05T19:28:15Z

This pull request has been merged in 402a793.

…distributed/rendezvous (#53172) Summary: Pull Request resolved: #53172 Pull Request resolved: pytorch/elastic#141 Upstreams two modules to torch: 1. `torchelastic.rendezvous` 2. `torchelastic.utils` These modules were chosen as `[1/n]` since they are the leaf modules in torchelastic. ==== NOTES: ==== 1. I'm disabling etcd_rendezvous and etcd_server tests in CIRCLECI for the moment since I need to edit the test dockers to contain the etcd server binary (there's 4-5 test dockers - one for each platform so this is going to take some time for me to set up the environments and test) - T85992919. 2. I've fixed all lint errors on python files but there are ones on the cpp files on the ZeusRendezvous. I took a look at them, and I don't want to fix the linter errors right now for 2 major reasons: 1. Some of them are more than formatting changes (e.g. std::move vs pass by value) and I don't want to introduce bundled changes with the move 1. The old rendezvous code (the one we forked from in caffe2/fb) has the same problems and I think its better for us to deal with this when we deprecate caffe2/fb/rendezvous in favor of the one in torchelastic -T86012579. Test Plan: ``` buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/data/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/fb/... buck test mode/dev-nosan //pytorch/elastic/torchelastic/... ``` \+ Sandcastle Reviewed By: H-Huang Differential Revision: D26718746 fbshipit-source-id: 67cc0350c3d847221cb3c3038f98f47915362f51

…distributed/rendezvous (pytorch#53172) Summary: Pull Request resolved: pytorch#53172 Pull Request resolved: pytorch/elastic#141 Upstreams two modules to torch: 1. `torchelastic.rendezvous` 2. `torchelastic.utils` These modules were chosen as `[1/n]` since they are the leaf modules in torchelastic. ==== NOTES: ==== 1. I'm disabling etcd_rendezvous and etcd_server tests in CIRCLECI for the moment since I need to edit the test dockers to contain the etcd server binary (there's 4-5 test dockers - one for each platform so this is going to take some time for me to set up the environments and test) - T85992919. 2. I've fixed all lint errors on python files but there are ones on the cpp files on the ZeusRendezvous. I took a look at them, and I don't want to fix the linter errors right now for 2 major reasons: 1. Some of them are more than formatting changes (e.g. std::move vs pass by value) and I don't want to introduce bundled changes with the move 1. The old rendezvous code (the one we forked from in caffe2/fb) has the same problems and I think its better for us to deal with this when we deprecate caffe2/fb/rendezvous in favor of the one in torchelastic -T86012579. Test Plan: ``` buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/utils/data/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/test/... buck test mode/dev-nosan //caffe2/torch/distributed/elastic/rendezvous/fb/... buck test mode/dev-nosan //pytorch/elastic/torchelastic/... ``` \+ Sandcastle Reviewed By: H-Huang Differential Revision: D26718746 fbshipit-source-id: 67cc0350c3d847221cb3c3038f98f47915362f51

Summary: Pull Request resolved: pytorch/pytorch#53172 Pull Request resolved: pytorch#141 Upstreams two modules to torch: 1. `torchelastic.rendezvous` 2. `torchelastic.utils` These modules were chosen as `[1/n]` since they are the leaf modules in torchelastic. ==== NOTES: ==== 1. I'm disabling etcd_rendezvous and etcd_server tests in CIRCLECI for the moment since I need to edit the test dockers to contain the etcd server binary (there's 4-5 test dockers - one for each platform so this is going to take some time for me to set up the environments and test) - T85992919. 2. I've fixed all lint errors on python files but there are ones on the cpp files on the ZeusRendezvous. I took a look at them, and I don't want to fix the linter errors right now for 2 major reasons: 1. Some of them are more than formatting changes (e.g. std::move vs pass by value) and I don't want to introduce bundled changes with the move 1. The old rendezvous code (the one we forked from in caffe2/fb) has the same problems and I think its better for us to deal with this when we deprecate caffe2/fb/rendezvous in favor of the one in torchelastic -T86012579. Reviewed By: H-Huang Differential Revision: D26718746 fbshipit-source-id: 67cc0350c3d847221cb3c3038f98f47915362f51

kiukchung requested review from mingzhe09088, mrshenli, pritamdamania87, rohan-varma and zhaojuanmao as code owners March 3, 2021 06:42

facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue fb-exported labels Mar 3, 2021

kiukchung force-pushed the export-D26718746 branch from 6449cf3 to baf12ff Compare March 3, 2021 21:04

kiukchung force-pushed the export-D26718746 branch from baf12ff to ee0cbd1 Compare March 4, 2021 07:07

kiukchung force-pushed the export-D26718746 branch from ee0cbd1 to 1aac1f5 Compare March 4, 2021 19:47

mrshenli reviewed Mar 5, 2021

View reviewed changes

mrshenli approved these changes Mar 5, 2021

View reviewed changes

facebook-github-bot closed this in pytorch/elastic@402a793 Mar 5, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/n][torch/elastic][upstream] Move torchelastic/rendezvous to torch/distributed/rendezvous (#141) #53172

[1/n][torch/elastic][upstream] Move torchelastic/rendezvous to torch/distributed/rendezvous (#141) #53172

kiukchung commented Mar 3, 2021

facebook-github-bot commented Mar 3, 2021 •

edited

Loading

facebook-github-bot commented Mar 3, 2021

facebook-github-bot commented Mar 3, 2021

facebook-github-bot commented Mar 4, 2021

facebook-github-bot commented Mar 4, 2021

mrshenli Mar 5, 2021

mrshenli Mar 5, 2021

mrshenli Mar 5, 2021

kiukchung Mar 5, 2021 •

edited

Loading

mrshenli Mar 5, 2021

kiukchung Mar 5, 2021

mrshenli Mar 5, 2021

kiukchung Mar 5, 2021

mrshenli left a comment

mrshenli Mar 5, 2021

kiukchung Mar 5, 2021

mrshenli Mar 5, 2021

kiukchung Mar 5, 2021

facebook-github-bot commented Mar 5, 2021

		from torch.distributed.elastic.rendezvous.etcd_server import EtcdServer


		@unittest.skipIf(os.getenv("CIRCLECI"), "T85992919 temporarily disabling in circle ci")

		from torch.utils.data.distributed import DistributedSampler


		class ElasticDistributedSampler(DistributedSampler):

[1/n][torch/elastic][upstream] Move torchelastic/rendezvous to torch/distributed/rendezvous (#141) #53172

[1/n][torch/elastic][upstream] Move torchelastic/rendezvous to torch/distributed/rendezvous (#141) #53172

Conversation

kiukchung commented Mar 3, 2021

facebook-github-bot commented Mar 3, 2021 • edited Loading

💊 CI failures summary and remediations

❄️ 1 failure tentatively classified as flaky

pytorch_xla_linux_bionic_py3_6_clang9_test (1/1)

ci.pytorch.org: 2 failed

facebook-github-bot commented Mar 3, 2021

facebook-github-bot commented Mar 3, 2021

facebook-github-bot commented Mar 4, 2021

facebook-github-bot commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiukchung Mar 5, 2021 • edited Loading

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

mrshenli left a 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

facebook-github-bot commented Mar 5, 2021

facebook-github-bot commented Mar 3, 2021 •

edited

Loading

kiukchung Mar 5, 2021 •

edited

Loading