Skip to content

Fix BackendType repr in doc #30243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mrshenli wants to merge 4 commits into from
Closed

Fix BackendType repr in doc #30243

mrshenli wants to merge 4 commits into from

Conversation

@mrshenli
Copy link
Contributor

@mrshenli mrshenli commented Nov 21, 2019 •
edited
Loading

Stack from ghstack:

Before this commit, rpc docs shows init_rpc as the following:

torch.distributed.rpc.init_rpc(
   name,
   backend=<BackendType.PROCESS_GROUP: BackendValue(
     construct_rpc_agent_options_handler=<function _process_group_construct_rpc_agent_options_handler>,
     init_backend_handler=<function _process_group_init_backend_handler>)>,
   init_method=None,
   rank=-1,
   world_size=None,
   rpc_agent_options=None
)

It unnecessarily leaks implementation details. This commit adds a
repr function to BackendType Enum class to address this problem.

closes #29905

Differential Revision: D18641559

@mrshenli
Fix BackendType repr in doc
cca2d87 
[ghstack-poisoned]
This was referenced Nov 21, 2019
Closed
Closed
@mrshenli
Update on "Fix BackendType repr in doc"
eafd287 
Before this commit, rpc docs shows init_rpc as the following:

> torch.distributed.rpc.init_rpc(
>   name,
>   backend=<BackendType.PROCESS_GROUP: BackendValue(
>     construct_rpc_agent_options_handler=<function _process_group_construct_rpc_agent_options_handler>,
>     init_backend_handler=<function _process_group_init_backend_handler>)>,
>   init_method=None,
>   rank=-1,
>   world_size=None,
>   rpc_agent_options=None
> )

It unnecessarily leaks implementation details. This commit adds a
__repr__ function to BackendType Enum class to address this problem.

[ghstack-poisoned]
mrshenli added a commit that referenced this pull request Nov 21, 2019
@mrshenli
Fix BackendType repr in doc
4282207 
Before this commit, rpc docs shows init_rpc as the following:

> torch.distributed.rpc.init_rpc(
>   name,
>   backend=<BackendType.PROCESS_GROUP: BackendValue(
>     construct_rpc_agent_options_handler=<function _process_group_construct_rpc_agent_options_handler>,
>     init_backend_handler=<function _process_group_init_backend_handler>)>,
>   init_method=None,
>   rank=-1,
>   world_size=None,
>   rpc_agent_options=None
> )

It unnecessarily leaks implementation details. This commit adds a
__repr__ function to BackendType Enum class to address this problem.

ghstack-source-id: 71a95e7
Pull Request resolved: #30243
@mrshenli
Copy link
Contributor Author

mrshenli commented Nov 21, 2019

Screen Shot 2019-11-21 at 1 54 45 PM

@mrshenli mrshenli mentioned this pull request Nov 21, 2019
Closed
rohan-varma
rohan-varma approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@rohan-varma rohan-varma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Does BackendType show up in docs? Would we need it to show up so users know about BackendType.ProcessGroup?

@mrshenli
Copy link
Contributor Author

mrshenli commented Nov 21, 2019

Looks good. Does BackendType show up in docs? Would we need it to show up so users know about BackendType.ProcessGroup?

Good point, it does not yet, and we should have it. Let me try in a followup PR.

xush6528
xush6528 reviewed Nov 21, 2019
View reviewed changes
torch/distributed/rpc/backend_registry.py Outdated
BackendType = enum.Enum(
value="BackendType",
names={},
__repr__=_backend_type_repr
Copy link
Contributor

@xush6528 xush6528 Nov 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work?

It looks to me unexpeted keyword args are dropped by this asterisk.

https://github.com/python/cpython/blob/293dd23477eef6e7c1b1e26b5bb2c1e0d79ac3c2/Lib/enum.py#L278

Copy link
Contributor Author

@mrshenli mrshenli Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes you are right! I was doing BackendType .__repr__ = _backend_type_repr when generating the figure and then switched to this afterward. It was showing the same result, but I should have make clean the docs, after that I started to see errors. Let me update.

@mrshenli
Update on "Fix BackendType repr in doc"
1c4d414 
Before this commit, rpc docs shows init_rpc as the following:

```python
torch.distributed.rpc.init_rpc(
   name,
   backend=<BackendType.PROCESS_GROUP: BackendValue(
     construct_rpc_agent_options_handler=<function _process_group_construct_rpc_agent_options_handler>,
     init_backend_handler=<function _process_group_init_backend_handler>)>,
   init_method=None,
   rank=-1,
   world_size=None,
   rpc_agent_options=None
)
```

It unnecessarily leaks implementation details. This commit adds a
__repr__ function to BackendType Enum class to address this problem.

closes #29905

Differential Revision: [D18641559](https://our.internmc.facebook.com/intern/diff/D18641559)

[ghstack-poisoned]
xush6528
xush6528 approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@xush6528 xush6528 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel certain that this works!

@mrshenli
Update on "Fix BackendType repr in doc"
0a10fec 
Before this commit, rpc docs shows init_rpc as the following:

```python
torch.distributed.rpc.init_rpc(
   name,
   backend=<BackendType.PROCESS_GROUP: BackendValue(
     construct_rpc_agent_options_handler=<function _process_group_construct_rpc_agent_options_handler>,
     init_backend_handler=<function _process_group_init_backend_handler>)>,
   init_method=None,
   rank=-1,
   world_size=None,
   rpc_agent_options=None
)
```

It unnecessarily leaks implementation details. This commit adds a
__repr__ function to BackendType Enum class to address this problem.

closes #29905

Differential Revision: [D18641559](https://our.internmc.facebook.com/intern/diff/D18641559)

[ghstack-poisoned]
mrshenli added a commit that referenced this pull request Nov 21, 2019
@mrshenli
Fix BackendType repr in doc
1102d90 
Before this commit, rpc docs shows init_rpc as the following:

> torch.distributed.rpc.init_rpc(
>   name,
>   backend=<BackendType.PROCESS_GROUP: BackendValue(
>     construct_rpc_agent_options_handler=<function _process_group_construct_rpc_agent_options_handler>,
>     init_backend_handler=<function _process_group_init_backend_handler>)>,
>   init_method=None,
>   rank=-1,
>   world_size=None,
>   rpc_agent_options=None
> )

It unnecessarily leaks implementation details. This commit adds a
__repr__ function to BackendType Enum class to address this problem.

ghstack-source-id: 33cdabc
Pull Request resolved: #30243
@pritamdamania87
Copy link
Contributor

pritamdamania87 commented Nov 21, 2019

@mrshenli I see a couple of issues with the docs:

  1. init_rpc used to be a link to the method, now the link is lost.
  2. The following line doesn't seem right: "By default, this will also initialize the ProcessGroup (torch.distributed.Backend()) backend for RPC communication". We shouldn't link to torch.distributed.Backend (since this backend refers to gloo,nccl and mpi) for ProcessGroup. We should link to it when we specify that we use gloo for communication. If we do want a link for ProcessGroup, maybe we can link to https://pytorch.org/docs/master/distributed.html#torch.distributed.init_process_group for now?

@mrshenli
Copy link
Contributor Author

mrshenli commented Nov 21, 2019

@pritamdamania87

  1. init_rpc used to be a link to the method, now the link is lost.

Where is that link?

@mrshenli
Copy link
Contributor Author

mrshenli commented Nov 21, 2019

The following line doesn't seem right: "By default, this will also initialize the ProcessGroup (torch.distributed.Backend()) backend for RPC communication". We shouldn't link to torch.distributed.Backend (since this backend refers to gloo,nccl and mpi) for ProcessGroup. We should link to it when we specify that we use gloo for communication. If we do want a link for ProcessGroup, maybe we can link to https://pytorch.org/docs/master/distributed.html#torch.distributed.init_process_group for now?

I agree. This is irrelevant to this PR though, add fix in a followup PR

@mrshenli mrshenli mentioned this pull request Nov 21, 2019
Closed
@mrshenli
Copy link
Contributor Author

mrshenli commented Nov 21, 2019

Addressed @pritamdamania87's comments in #30259

@facebook-github-bot facebook-github-bot deleted the gh/mrshenli/65/head branch December 10, 2019 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xush6528 xush6528 xush6528 approved these changes

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

+1 more reviewer

@rohan-varma rohan-varma rohan-varma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mrshenli @pritamdamania87 @xush6528 @rohan-varma