[RPC] Infer backend type if only options are given #45065

lw · 2020-09-21T13:15:11Z

Stack from ghstack:

[RPC] Infer backend type if only options are given #45065 [RPC] Infer backend type if only options are given

To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change.

Differential Revision: D23814289

To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change. Differential Revision: [D23814289](https://our.internmc.facebook.com/intern/diff/D23814289/) [ghstack-poisoned]

To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change. Differential Revision: [D23814289](https://our.internmc.facebook.com/intern/diff/D23814289/) ghstack-source-id: 112484729 Pull Request resolved: #45065

lw · 2020-09-21T13:18:26Z

torch/distributed/rpc/__init__.py

+                    f"RPC was initialized with no explicit backend but with options "
+                    f"corresponding to {backend}, hence that backend will be used "
+                    f"instead of the default {BackendType.TENSORPIPE}. To silence this "
+                    f"warning pass `backend={backend}` explicitly."


Should we also add a deprecation warning about ProcessGroup since our long-term plan is to remove it?

Yes, it would be good to add a deprecation warning too.

dr-ci · 2020-09-21T13:32:03Z

💊 CI failures summary and remediations

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

2/2 failures possibly* introduced in this PR
- 2/2 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

Failed: Codecov - codecov/patch

codecov.io: 1 failed

Failed: codecov/patch

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 11 times.

codecov · 2020-09-21T16:51:10Z

Codecov Report

Merging #45065 into gh/lw/76/base will decrease coverage by 0.05%.
The diff coverage is 21.31%.

@@                Coverage Diff                @@
##           gh/lw/76/base   #45065      +/-   ##
=================================================
- Coverage          67.83%   67.78%   -0.06%     
=================================================
  Files                384      384              
  Lines              50047    50106      +59     
=================================================
+ Hits               33950    33962      +12     
- Misses             16097    16144      +47

Impacted Files	Coverage Δ
torch/distributed/rpc/backend_registry.py	`32.35% <0.00%> (-1.67%)`	⬇️
torch/distributed/rpc/__init__.py	`36.92% <15.00%> (-9.89%)`	⬇️
...orch/testing/_internal/distributed/rpc/rpc_test.py	`25.70% <27.77%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71aeb84...f3c1fcf. Read the comment docs.

pritamdamania87 · 2020-09-21T18:46:53Z

torch/distributed/rpc/__init__.py

+                    f"RPC was initialized with no explicit backend but with options "
+                    f"corresponding to {backend}, hence that backend will be used "
+                    f"instead of the default {BackendType.TENSORPIPE}. To silence this "
+                    f"warning pass `backend={backend}` explicitly."


Yes, it would be good to add a deprecation warning too.

pritamdamania87 · 2020-09-21T18:47:41Z

torch/distributed/rpc/__init__.py

+        if backend is None:
+            backend = BackendType.TENSORPIPE
+
+        if rpc_backend_options is None:


What happens if backend is specified as TensorPipe but backend options are ProcessGroupOptions? Do we raise an appropriate error? If so, can we have a unit test that verifies this?

pritamdamania87 · 2020-09-21T18:48:36Z

torch/distributed/rpc/__init__.py

+                    backend = candidate_backend
+                    break


Are these codecov warnings false positives? It does seem like your test should cover this line?

That's unclear. I'm assuming that the coverage is collected from this job: https://app.circleci.com/pipelines/github/pytorch/pytorch/217281/workflows/3cfc25f2-225c-4be1-9ad5-a7942dc829e7/jobs/7659664 (since it's the only one with coverage in the name). And I checked the logs and the tests are run.

One option is that the odd on-the-fly class construction trick we're doing to assemble the RPC test suites makes it hard for the coverage tool to map the test back to its original lines.

Or I misunderstood how this works.

pritamdamania87 · 2020-09-21T18:48:48Z

torch/distributed/rpc/__init__.py

+                    backend = candidate_backend
+                    break
+            else:
+                raise TypeError(


Can we add a test covering this?

To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change. Differential Revision: [D23814289](https://our.internmc.facebook.com/intern/diff/D23814289/) [ghstack-poisoned]

Pull Request resolved: #45065 To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change. ghstack-source-id: 112582130 Differential Revision: [D23814289](https://our.internmc.facebook.com/intern/diff/D23814289/)

To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change. Differential Revision: [D23814289](https://our.internmc.facebook.com/intern/diff/D23814289/) [ghstack-poisoned]

Pull Request resolved: #45065 To preserve backwards compatibility with applications that were passing in some ProcessGroupRpcBackendOptions but were not explicitly setting backend=BackendType.PROCESS_GROUP, we're here now inferring the backend type from the options if only the latter ones are passed. If neither are passed, we'll default to TensorPipe, as before this change. ghstack-source-id: 112586258 Differential Revision: [D23814289](https://our.internmc.facebook.com/intern/diff/D23814289/)

facebook-github-bot · 2020-09-23T08:13:58Z

This pull request has been merged in 76dc50e.

lw requested review from apaszke, mrshenli, pietern, pritamdamania87, rohan-varma and zhaojuanmao as code owners September 21, 2020 13:15

lw commented Sep 21, 2020

View reviewed changes

pritamdamania87 reviewed Sep 21, 2020

View reviewed changes

lw mentioned this pull request Sep 22, 2020

Update tutorials to use TensorPipeRpcBackendOptions. pytorch/tutorials#1164

Merged

pritamdamania87 approved these changes Sep 22, 2020

View reviewed changes

facebook-github-bot closed this in 76dc50e Sep 23, 2020

facebook-github-bot added the merged label Sep 23, 2020

facebook-github-bot deleted the gh/lw/76/head branch September 26, 2020 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RPC] Infer backend type if only options are given #45065

[RPC] Infer backend type if only options are given #45065

Uh oh!

lw commented Sep 21, 2020 •

edited

Loading

Uh oh!

lw Sep 21, 2020

Uh oh!

pritamdamania87 Sep 21, 2020

Uh oh!

dr-ci bot commented Sep 21, 2020 •

edited

Loading

Uh oh!

codecov bot commented Sep 21, 2020 •

edited

Loading

Uh oh!

pritamdamania87 Sep 21, 2020

Uh oh!

pritamdamania87 Sep 21, 2020

Uh oh!

pritamdamania87 Sep 21, 2020

Uh oh!

lw Sep 22, 2020

Uh oh!

pritamdamania87 Sep 21, 2020

Uh oh!

facebook-github-bot commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[RPC] Infer backend type if only options are given #45065

[RPC] Infer backend type if only options are given #45065

Uh oh!

Conversation

lw commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lw Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

pritamdamania87 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Extra GitHub checks: 1 failed

codecov.io: 1 failed

Uh oh!

codecov bot commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pritamdamania87 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

pritamdamania87 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

pritamdamania87 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

lw Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

pritamdamania87 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lw commented Sep 21, 2020 •

edited

Loading

dr-ci bot commented Sep 21, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading