Add test case for init_rpc_backend#26997
Add test case for init_rpc_backend#26997xush6528 wants to merge 14 commits intogh/xush6528/2/basefrom
Conversation
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
| elif is_rpc_backend_registered(backend): | ||
| _agent = init_rpc_backend( | ||
| backend, | ||
| self_rank=self_rank, |
There was a problem hiding this comment.
Can you add a test case in test_rpc.py where you register a backend and call init ?
There was a problem hiding this comment.
@satgera working on it. It requires me to patch init_rref_context
mrshenli
left a comment
There was a problem hiding this comment.
Would I be correct if I assume this is not needed for now as init_rpc_backend takes *args and **args, and the current registered backend we have does not really use self_rank? So this is only to make sure we do pass all args from _init_rpc to init_rpc_backend?
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 90955970 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
|
@mrshenli open-source tests are still good. I broke fb internal init_rpc_backend_handler. |
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 90998862 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
|
disabled rpc test in py2. |
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 91003067 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 91033024 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
|
Checked. failed tests are test_sparse_allreduce_checks, test_cpp_cuda. not related to this PR. |
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 91071519 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 91086987 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
|
|
||
| from torch.distributed.rpc import RpcBackend | ||
| from common_utils import load_tests | ||
| from torch.distributed.rpc_api import RpcBackend |
There was a problem hiding this comment.
I haven't seen this module before, is it new? If so, why was it added?
There was a problem hiding this comment.
Nevermind, I see it is added here. Why the rename?
There was a problem hiding this comment.
"rpc" module was shadowed by "rpc" method imported to "torch.distributed".
This caused the patch replacing the wrong target.
Fixed it by renaming the module to "rpc_api".
There was a problem hiding this comment.
I see, thanks. I believe @rohan-varma is working on a PR to rename those to rpc_sync and rpc_async, right? Then this will no longer be necessary.
There was a problem hiding this comment.
@pietern @xush6528 , that change is landed in #26570. But it doesn't remove the rpc function, it instead adds a warning to callers who use it and encourage them to call rpc_sync or rpc_async. We eventually want to entirely remove the rpc function as you mentioned, but wasn't sure if we could do this yet due to backwards compatibility issues, so after a discussion with @mrshenli (comments on the PR) we decided to go with this approach.
There was a problem hiding this comment.
@rohan-varma When we do, can you make sure to revert the rename? If it's an internal only module, it shouldn't be named _api, but rather use an underscore prefix, like all other private API like things in PyTorch.
There was a problem hiding this comment.
@pietern, the rpc_api.py module is part of our external-facing API though, right? It contains things like get_worker_id, remote, rpc_sync, etc. Should we define __all__ to make it clear what our external facing APIs are?
There was a problem hiding this comment.
I feel ideally we want to put all external api names in __init__.py and has a module implementing these api names.
It's a mess at the moment.
In __init__.py defines def init_model_parallel
In rpc_api.py, other API is defined (as Rohan listed).
There was a problem hiding this comment.
Looks like this is being further tracked in #27229
Reverting accidental change in #26919 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/) [ghstack-poisoned]
Pull Request resolved: #26997 Reverting accidental change in #26919 ghstack-source-id: 91126906 Differential Revision: [D17637468](https://our.internmc.facebook.com/intern/diff/D17637468/)
|
This pull request has been merged in 00e5882. |
Summary: Pull Request resolved: pytorch#26997 Reverting accidental change in pytorch#26919 ghstack-source-id: 91126906 Reviewed By: zhaojuanmao Differential Revision: D17637468 fbshipit-source-id: 9ffcf4b15b37effe6b5d5f82338ff89298c82a52
Summary: Pull Request resolved: pytorch#26997 Reverting accidental change in pytorch#26919 ghstack-source-id: 91126906 Reviewed By: zhaojuanmao Differential Revision: D17637468 fbshipit-source-id: 9ffcf4b15b37effe6b5d5f82338ff89298c82a52
Summary: Pull Request resolved: pytorch#26997 Reverting accidental change in pytorch#26919 ghstack-source-id: 91126906 Reviewed By: zhaojuanmao Differential Revision: D17637468 fbshipit-source-id: 9ffcf4b15b37effe6b5d5f82338ff89298c82a52
Stack from ghstack:
Differential Revision: D17637468