Always include autograd context id in rpc/remote requests #29781
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
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. [ghstack-poisoned]
This was referenced Nov 14, 2019
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. closes #28819 [ghstack-poisoned]
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. closes #28819 Differential Revision: [D18496709](https://our.internmc.facebook.com/intern/diff/D18496709) [ghstack-poisoned]
test/dist_autograd_test.py
Outdated
Comment on lines
619
to
623
| ctx = dist_autograd._current_context() | ||
| send_functions = ctx._send_functions() | ||
| self.assertEqual(len(send_functions), 0) | ||
| recv_functions = ctx._recv_functions() | ||
| self.assertEqual(len(recv_functions), 1) |
Contributor
There was a problem hiding this comment.
Instead of checking the autograd functions, can we run the backward pass and verify the gradient is accumulated correctly on the remote tensor? We probably might need to return a global tensor from ret_requires_grad to validate the grads are accumulated for ExecMode.RPC_SYNC
Contributor
Author
There was a problem hiding this comment.
Yes, let me add that.
pritamdamania87
suggested changes
Nov 14, 2019
Contributor
pritamdamania87
left a comment
pritamdamania87
left a comment
There was a problem hiding this comment.
Thanks for fixing this! Just have one comment inline for the unit test.
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. closes #28819 Differential Revision: [D18496709](https://our.internmc.facebook.com/intern/diff/D18496709) [ghstack-poisoned]
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. closes #28819 Differential Revision: [D18496709](https://our.internmc.facebook.com/intern/diff/D18496709) [ghstack-poisoned]
pritamdamania87
approved these changes
Nov 14, 2019
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. closes #28819 Differential Revision: [D18496709](https://our.internmc.facebook.com/intern/diff/D18496709) [ghstack-poisoned]
Contributor
Author
|
The lint failure is on a file that I didn't touch: |
Contributor
csarofeen
pushed a commit
to mruberry/pytorch
that referenced
this pull request
Nov 18, 2019
) Summary: Pull Request resolved: pytorch#29781 Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. closes pytorch#28819 Test Plan: Imported from OSS Differential Revision: D18496709 Pulled By: mrshenli fbshipit-source-id: 2f870c410291a1300952895b7488ea07e5574228
rohan-varma
added a commit
that referenced
this pull request
Dec 7, 2019
…ensure they are recorded in all cases" When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Dec 7, 2019
… are recorded in all cases Pull Request resolved: #30914 When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. ghstack-source-id: 95144641 Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/)
rohan-varma
added a commit
that referenced
this pull request
Dec 7, 2019
…ensure they are recorded in all cases" recorded in all cases** When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Dec 7, 2019
…ensure they are recorded in all cases" recorded in all cases** recorded in all cases** When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Dec 7, 2019
… are recorded in all cases Pull Request resolved: #30914 When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. ghstack-source-id: 95158324 Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/)
rohan-varma
added a commit
that referenced
this pull request
Dec 8, 2019
…ensure they are recorded in all cases" recorded in all cases** When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Dec 8, 2019
… are recorded in all cases Pull Request resolved: #30914 When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. ghstack-source-id: 95166853 Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/)
rohan-varma
added a commit
that referenced
this pull request
Dec 9, 2019
…ensure they are recorded in all cases" recorded in all cases** recorded in all cases** When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Dec 9, 2019
… are recorded in all cases Pull Request resolved: #30914 When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. ghstack-source-id: 95178561 Differential Revision: [D18869191](https://our.internmc.facebook.com/intern/diff/D18869191/)
facebook-github-bot
pushed a commit
that referenced
this pull request
Dec 9, 2019
…30914) Summary: Pull Request resolved: #30914 When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since #29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. ghstack-source-id: 95178561 Test Plan: Added a unit test: `test_context_cleanup_tensor_no_grad` Differential Revision: D18869191 fbshipit-source-id: b80f66bfd0dd7d01960abe1691d3f44095bb1b2b
xxtEchjovs44
pushed a commit
to xxtEchjovs44/pytorch
that referenced
this pull request
Jan 29, 2020
Even though the request might not contain any requires_grad tensor, the return value could. Therefore, we should always include the autograd context id in the request. ghstack-source-id: 75cd6bf Pull Request resolved: pytorch/pytorch#29781
wuhuikx
pushed a commit
to wuhuikx/pytorch
that referenced
this pull request
Jan 30, 2020
…ytorch#30914) Summary: Pull Request resolved: pytorch#30914 When tensors don't require grad, we don't call `addSendRpcBackward`, where we record known workerIDs to clean up the dist autograd context later. But since pytorch#29781, we always include the autograd context ID in RPCs, even if tensors do not require grad. So, it could be possible that we don't release the contexts on some nodes. This can contribute to OOMs since the contexts will not be cleaned up in this case, which can be checking by running the unit test without this patch. We can fix this issue by moving the `addKnownWorkerIds` call to the `getMessageWithAutograd` function. ghstack-source-id: 95178561 Test Plan: Added a unit test: `test_context_cleanup_tensor_no_grad` Differential Revision: D18869191 fbshipit-source-id: b80f66bfd0dd7d01960abe1691d3f44095bb1b2b
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:
Even though the request might not contain any requires_grad tensor,
the return value could. Therefore, we should always include the
autograd context id in the request.
closes #28819
Differential Revision: D18496709