[rpc] add the worker IDs outside of addSendRpcBackward to ensure they are recorded in all cases #30914
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
… 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 D18496709, 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
requested review from
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
pritamdamania87
approved these changes
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/)
…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]
…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/)
…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/)
…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/)
Contributor
|
This pull request has been merged in 4f342a6. |
xush6528
added a commit
that referenced
this pull request
Dec 12, 2019
Cleanup after #30914. In #30914, `autogradContext->addKnownWorkerId(dst);` was moved out of `addSendRpcBackward()`. So `addSendRpcBackward()` does not need `dstId` as it's argument anymore. Differential Revision: [D5742365](https://our.internmc.facebook.com/intern/diff/D5742365/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Dec 12, 2019
Cleanup after #30914. In #30914, `autogradContext->addKnownWorkerId(dst);` was moved out of `addSendRpcBackward()`. So `addSendRpcBackward()` does not need `dstId` as it's argument anymore. Differential Revision: [D5742365](https://our.internmc.facebook.com/intern/diff/D5742365/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Dec 12, 2019
Pull Request resolved: #31207 Cleanup after #30914. In #30914, `autogradContext->addKnownWorkerId(dst);` was moved out of `addSendRpcBackward()`. So `addSendRpcBackward()` does not need `dstId` as it's argument anymore. ghstack-source-id: 95509218 Differential Revision: [D5742365](https://our.internmc.facebook.com/intern/diff/D5742365/)
facebook-github-bot
pushed a commit
that referenced
this pull request
Dec 15, 2019
Summary: Pull Request resolved: #31207 Cleanup after #30914. In #30914, `autogradContext->addKnownWorkerId(dst);` was moved out of `addSendRpcBackward()`. So `addSendRpcBackward()` does not need `dstId` as it's argument anymore. ghstack-source-id: 95509218 Test Plan: # Unit tests ``` buck test mode/dev-nosan //caffe2/test:dist_autograd_fork -- test_context_cleanup_tensor_no_grad ``` Differential Revision: D5742365 fbshipit-source-id: accd041a594ec18d369231f5590289828d87baa7
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
wuhuikx
pushed a commit
to wuhuikx/pytorch
that referenced
this pull request
Jan 30, 2020
Summary: Pull Request resolved: pytorch#31207 Cleanup after pytorch#30914. In pytorch#30914, `autogradContext->addKnownWorkerId(dst);` was moved out of `addSendRpcBackward()`. So `addSendRpcBackward()` does not need `dstId` as it's argument anymore. ghstack-source-id: 95509218 Test Plan: # Unit tests ``` buck test mode/dev-nosan //caffe2/test:dist_autograd_fork -- test_context_cleanup_tensor_no_grad ``` Differential Revision: D5742365 fbshipit-source-id: accd041a594ec18d369231f5590289828d87baa7
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:
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
addKnownWorkerIdscall to thegetMessageWithAutogradfunction.Differential Revision: D18869191