Refactor core DistributedDataParallel tests #20235
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
Differential Revision: D15245429 Differential Version: 81320055
pytorchbot
added
the
oncall: distributed
This was referenced May 7, 2019
mrshenli
approved these changes
May 7, 2019
Contributor
mrshenli
left a comment
mrshenli
left a comment
There was a problem hiding this comment.
If device_ids is indeed unnecessary, shall we consolidate?
| copy.deepcopy(model).cuda(gpus[0]), | ||
| device_ids=gpus, | ||
| copy.deepcopy(model).to(devices[0]), | ||
| device_ids=device_ids, |
Contributor
There was a problem hiding this comment.
device_ids in DDP should be able to directly handle devices, as we wrote:
device_ids (list of int or torch.device): CUDA devices. This should ...
Contributor
Author
There was a problem hiding this comment.
The existing tests exercised both and I didn't want to change that. We should test both the list of integers and list of torch.device cases in the suite (it could be broken out as a separate lightweight test instead of performing a full end to end tests).
Differential Revision: D15245429 Differential Version: 81326667
Differential Revision: D15245429 Differential Version: 81330801
Differential Revision: D15245429 Differential Version: 81461821
Contributor
|
@pytorchbot retest this please |
Contributor
|
This pull request has been merged in f32c9bd. |
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:
:white_circle: #20236 Make DistributedDataParallel usable with CPU models 💚
:black_circle: #20235 Refactor core DistributedDataParallel tests 💚
:white_circle: #20234 Add c10d::broadcast_coalesced and tests 💚
The tests expected to only run for CUDA models. In a future commit we
need to update this to work for CPU models as well. Therefore, we can
no longer rely on only integers being passed for device identifiers.
With this change we pass both the materialized list of devices to use
(as
torch.Deviceobjects), as well as an optional list of integers.The latter is specified to exercise the code in the
DistributedDataParallel constructor that turns a list of integers into
CUDA devices, IFF it is used to wrap a single-device CUDA module.
This commit also groups together the 'str' and non-'str' tests. These
used to test passing the list of devices as integers or as
torch.Deviceinstances. These are now executed from the same test.Differential Revision: D15245429