Add `RpcAgentOptions` struct type, which bundles different required arguments for different `RpcAgent`s #29972

xush6528 · 2019-11-16T18:58:37Z

Summary:

#28226 introduced worker_to_id arg to the def init_rpc function for other RpcAgent. While it's not really used by ProcessGroupAgent. Cleanup is wanted for this, as described in #29031.

To adapt to the difference of different RpcAgent, adding a RpcAgentOptions base classes, which allow leveraging inheritance to add extra fields.

closes #29031

Differential Revision: D18549919

facebook-github-bot · 2019-11-16T18:58:53Z

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

facebook-github-bot · 2019-11-18T18:36:26Z

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

test/dist_utils.py

torch/csrc/distributed/rpc/init.cpp

mrshenli · 2019-11-18T20:11:20Z

torch/csrc/distributed/rpc/init.cpp

docs for this one as well (can be done in separate PR)

Any reason this is not using shared_ptr_class_?

Didn't do it on purpose. What is the benefit of using shared_ptr_class_ ?

From pybind doc:

If no such holder type template argument is given, the default for a type named Type is std::unique_ptr, which means that the object is deallocated when Python’s reference count goes to zero.

torch/distributed/rendezvous.py

mrshenli · 2019-11-18T20:19:23Z

torch/distributed/rpc/api.py

any reason c10d::Store is not quoted?

Should quote it.

mrshenli · 2019-11-18T20:22:29Z

torch/distributed/rpc/backend_registry.py

backen -> backend

mrshenli · 2019-11-18T20:23:47Z

torch/distributed/rpc/backend_registry.py

this is a pretty long name, but I don't have a better suggestion. Does rpc_agent_options_factory work? If we do that init_backend_handler will become backend_factory.

I don't think it is a backend_factory, it always creates the same type of backend. I can't think of a better name.

facebook-github-bot · 2019-11-18T21:52:50Z

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

pritamdamania87 · 2019-11-18T21:50:43Z

torch/distributed/rpc/__init__.py

Shouldn't init_method be subsumed by rpc_agent_options?

@pritamdamania87 It's used by all backbends. So we don't want to put it into RpcAgentOptions.

pritamdamania87 · 2019-11-18T21:51:51Z

torch/distributed/rpc/__init__.py

General comment, should self_name and self_rank be name and rank? For example, init_process_group uses rank and not self_rank.

@mrshenli What do you think? If we all agree. I will do the change.

+1 for this change (drop self_ in the name)

facebook-github-bot · 2019-11-18T23:38:27Z

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

facebook-github-bot · 2019-11-18T23:39:25Z

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

mrshenli

LGTM! Thanks!

It looks like CI tests didn't launch properly (only 7 out of ~60 were launched). Please make sure all CI tests pass before landing.

facebook-github-bot

@xush6528 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

xush6528 · 2019-11-19T01:09:57Z

@pytorchbot retest this please

…rguments for different `RpcAgent`s (pytorch#29972) Summary: pytorch#28226 introduced `worker_to_id` arg to the `def init_rpc` function for other `RpcAgent`. While it's not really used by `ProcessGroupAgent`. Cleanup is wanted for this, as described in pytorch#29031. To adapt to the difference of different `RpcAgent`, adding a `RpcAgentOptions` base classes, which allow leveraging inheritance to add extra fields. closes pytorch#29031 Pull Request resolved: pytorch#29972 Differential Revision: D18549919 Pulled By: xush6528 fbshipit-source-id: 7bea84148341d00bbcb29f9bc72707ba5e6ba238

facebook-github-bot · 2019-11-19T03:06:49Z

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

facebook-github-bot · 2019-11-19T11:06:15Z

@xush6528 merged this pull request in 21dc1d4.

xush6528 requested review from apaszke, mrshenli, pietern and pritamdamania87 as code owners November 16, 2019 18:58

facebook-github-bot added the fb-exported label Nov 16, 2019

xush6528 mentioned this pull request Nov 16, 2019

Add RpcAgentOptions struct type, which bundles different required arguments for different RpcAgents #29443

Closed

xush6528 force-pushed the export-D18549919 branch from 3a4e80e to d64671f Compare November 18, 2019 18:36

mrshenli reviewed Nov 18, 2019

View reviewed changes

xush6528 mentioned this pull request Nov 18, 2019

Add docs for RpcAgentOptions #30032

Closed

xush6528 force-pushed the export-D18549919 branch from d64671f to c6bd871 Compare November 18, 2019 21:52

xush6528 requested a review from zhaojuanmao as a code owner November 18, 2019 21:52

pritamdamania87 reviewed Nov 18, 2019

View reviewed changes

xush6528 force-pushed the export-D18549919 branch from c6bd871 to b9fa82e Compare November 18, 2019 23:38

xush6528 force-pushed the export-D18549919 branch from b9fa82e to 59aa685 Compare November 18, 2019 23:39

mrshenli approved these changes Nov 19, 2019

View reviewed changes

facebook-github-bot reviewed Nov 19, 2019

View reviewed changes

xush6528 force-pushed the export-D18549919 branch from 59aa685 to 28a2e33 Compare November 19, 2019 03:06

facebook-github-bot closed this in 21dc1d4 Nov 19, 2019

facebook-github-bot added the merged label Nov 19, 2019

mruberry added the Merged label Oct 28, 2020

Add RpcAgentOptions struct type, which bundles different required arguments for different RpcAgents #29972

Add RpcAgentOptions struct type, which bundles different required arguments for different RpcAgents #29972

Uh oh!

Conversation

xush6528 commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Nov 16, 2019

Uh oh!

facebook-github-bot commented Nov 18, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xush6528 Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 18, 2019

Uh oh!

facebook-github-bot commented Nov 18, 2019

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 commented Nov 19, 2019

Uh oh!

facebook-github-bot commented Nov 19, 2019

Uh oh!

facebook-github-bot commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add `RpcAgentOptions` struct type, which bundles different required arguments for different `RpcAgent`s #29972

Add `RpcAgentOptions` struct type, which bundles different required arguments for different `RpcAgent`s #29972

xush6528 commented Nov 16, 2019 •

edited

Loading

xush6528 Nov 18, 2019 •

edited

Loading