Fix distributed autograd initialization. #29069
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Distributed autograd was initialized after RPC and this would cause a race in some scenarios where one node might have initialized distributed autograd, calls backward() but other nodes have not initialized distributed autograd yet. Moving this before `_init_rpc` fixes the problem since `_init_rpc` implicitly has a sync between processes via the store. Differential Revision: [D18280875](https://our.internmc.facebook.com/intern/diff/D18280875/) [ghstack-poisoned]
pritamdamania87
requested review from
apaszke,
mrshenli and
pietern
as code owners
pritamdamania87
pushed a commit
that referenced
this pull request
Nov 1, 2019
Distributed autograd was initialized after RPC and this would cause a race in some scenarios where one node might have initialized distributed autograd, calls backward() but other nodes have not initialized distributed autograd yet. Moving this before `_init_rpc` fixes the problem since `_init_rpc` implicitly has a sync between processes via the store. Differential Revision: [D18280875](https://our.internmc.facebook.com/intern/diff/D18280875/) ghstack-source-id: 93117986 Pull Request resolved: #29069
pietern
approved these changes
Nov 4, 2019
torch/distributed/rpc/__init__.py
Outdated
| store, _, _ = next(rendezvous_iterator) | ||
|
|
||
| # Initialize autograd before RPC since _init_rpc guarantees all | ||
| # processes sync via the store. If we initialize autograd after rpc, |
pietern
suggested changes
Nov 4, 2019
Contributor
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
CI is failing.
The RPC initialization tests may crash because you now end up initializing distributed autograd twice.
Contributor
Contributor
|
If #29157 is merged before this one, could you enable those two tests? |
Contributor
Contributor
|
it will close #28932 as well |
Contributor
|
Shall we get this landed soon? |
Contributor
|
This will also close #29410 |
Distributed autograd was initialized after RPC and this would cause a race in some scenarios where one node might have initialized distributed autograd, calls backward() but other nodes have not initialized distributed autograd yet. Moving this before `_init_rpc` fixes the problem since `_init_rpc` implicitly has a sync between processes via the store. Differential Revision: [D18280875](https://our.internmc.facebook.com/intern/diff/D18280875/) [ghstack-poisoned]
pritamdamania87
pushed a commit
that referenced
this pull request
Nov 8, 2019
Pull Request resolved: #29069 Distributed autograd was initialized after RPC and this would cause a race in some scenarios where one node might have initialized distributed autograd, calls backward() but other nodes have not initialized distributed autograd yet. Moving this before `_init_rpc` fixes the problem since `_init_rpc` implicitly has a sync between processes via the store. ghstack-source-id: 93505965 Differential Revision: [D18280875](https://our.internmc.facebook.com/intern/diff/D18280875/)
mrshenli
approved these changes
Nov 8, 2019
Contributor
mrshenli
left a comment
mrshenli
left a comment
There was a problem hiding this comment.
This LGTM. Just had a question regarding init_rpc.
| with self.assertRaisesRegex(RuntimeError, "is not unique"): | ||
| rpc.init_model_parallel( | ||
| self_name="duplicate_name", | ||
| store, _, _ = next(torch.distributed.rendezvous( |
Contributor
There was a problem hiding this comment.
why calling the lower level APIs instead of init_model_parallel?
Contributor
Author
There was a problem hiding this comment.
With the current changes init_model_parallel throws a KeyError because the worker_name_to_id[self_name] lookup fails for invalid strings. I call the lower level API here to reproduce the errors we're looking for.
Contributor
There was a problem hiding this comment.
I see, we will need to update these tests after we move worker_name_to_id to backend_config dict.
Distributed autograd was initialized after RPC and this would cause a race in some scenarios where one node might have initialized distributed autograd, calls backward() but other nodes have not initialized distributed autograd yet. Moving this before `_init_rpc` fixes the problem since `_init_rpc` implicitly has a sync between processes via the store. Differential Revision: [D18280875](https://our.internmc.facebook.com/intern/diff/D18280875/) [ghstack-poisoned]
pritamdamania87
pushed a commit
that referenced
this pull request
Nov 8, 2019
Pull Request resolved: #29069 Distributed autograd was initialized after RPC and this would cause a race in some scenarios where one node might have initialized distributed autograd, calls backward() but other nodes have not initialized distributed autograd yet. Moving this before `_init_rpc` fixes the problem since `_init_rpc` implicitly has a sync between processes via the store. ghstack-source-id: 93535922 Differential Revision: [D18280875](https://our.internmc.facebook.com/intern/diff/D18280875/)
Contributor
|
This pull request has been merged in 5e1983f. |
This was referenced Nov 11, 2019
This was referenced Nov 18, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Distributed autograd was initialized after RPC and this would cause a
race in some scenarios where one node might have initialized distributed
autograd, calls backward() but other nodes have not initialized distributed
autograd yet.
Moving this before
_init_rpcfixes the problem since_init_rpcimplicitlyhas a sync between processes via the store.
Differential Revision: D18280875