convert output_device at data_parallel from torch.device to index #10189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

weiyangfb wants to merge 1 commit into pytorch:master from weiyangfb:parallel_device

Contributor

weiyangfb commented Aug 2, 2018

fixes torch.device and torch.nn.parallel.data_parallel compatibility #9984

weiyangfb requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners

August 2, 2018 23:14

Contributor Author

weiyangfb commented Aug 3, 2018

@pytorchbot retest this please

Contributor

vishwakftw commented Aug 3, 2018

I believe there are some instances of the same case in nn/parallel/distributed.py and nn/parallel/distributed_c10d.py. Could those be changed too?

Contributor Author

weiyangfb commented Aug 3, 2018

@vishwakftw I see, I will change them as well

ailzhang reviewed

View reviewed changes

test/test_nn.py Outdated

    
                      # test output_device

                      l = nn.Linear(10, 5).float().cuda()

                      i = Variable(torch.randn(20, 10).float().cuda())

                      out = dp.data_parallel(l, i, (0, 1), torch.device('cuda'))

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

weiyangfb added the ready for review (this tag is deprecated) label

weiyangfb force-pushed the parallel_device branch from 6606483 to d7c16b6 Compare

August 15, 2018 18:27

li-roy approved these changes

View reviewed changes

Contributor

li-roy left a comment

This looks good to me.

Contributor

li-roy commented Aug 16, 2018

actually, can we add a test for the other two code paths as well?

ssnl requested changes

View reviewed changes

Collaborator

ssnl left a comment

The better fix is to make scatter, gather, parallel_apply, etc. to accept device objects (vs. converting to idx in DP). You can also make device_ids to support device objects this way.

Member

fmassa commented Aug 28, 2018

ping @weiyangfb on @ssnl suggestion.

Collaborator

ssnl commented Aug 28, 2018

you can probably use/adapt torch.cuda._get_device_index now to do that after #10833 .

weiyangfb force-pushed the parallel_device branch from d7c16b6 to c70b9d2 Compare

August 28, 2018 20:00

weiyangfb requested review from pietern and teng-li as code owners

August 28, 2018 20:00

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Contributor Author

weiyangfb commented Aug 29, 2018

is this good to go? @ssnl

weiyangfb force-pushed the parallel_device branch from c60a6a6 to 9e500da Compare

September 4, 2018 20:12

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Contributor Author

weiyangfb commented Sep 5, 2018

is this good to go? @ssnl @teng-li @ailzhang

teng-li reviewed

View reviewed changes

torch/nn/parallel/parallel_apply.py Outdated

This comment was marked as off-topic.

Sign in to view

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ssnl reviewed

View reviewed changes

test/test_nn.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/nn/parallel/data_parallel.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/nn/parallel/replicate.py Outdated

This comment was marked as off-topic.

Sign in to view

weiyangfb force-pushed the parallel_device branch from 25b90d1 to 4ed0d8f Compare

September 6, 2018 21:26

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Contributor Author

weiyangfb commented Sep 10, 2018

@ssnl would you like to take quick pass on this? The updates are separated test function and doc fixes. Thanks!

ssnl approved these changes

View reviewed changes

Collaborator

ssnl left a comment

lgtm, but there is one remaining nit to be addressed

weiyangfb force-pushed the parallel_device branch from 4ed0d8f to 3a4e211 Compare

September 11, 2018 05:11

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Contributor Author

weiyangfb commented Sep 11, 2018

@ssnl I see, fixed more places with Device -> torch.device

weiyangfb mentioned this pull request

[wip] allow torch.device as input for device args in data_parallel and distributed APIs #11530

Closed

weiyangfb force-pushed the parallel_device branch from 3a4e211 to f360f36 Compare

September 11, 2018 18:16

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.


          allow torch.device as devices at args of data_parallel and distribute…

d5721ff

…d APIs

1. convert torch.device to device.index in APIs
2. docs fixes

weiyangfb force-pushed the parallel_device branch from f360f36 to d5721ff Compare

September 11, 2018 22:48

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

weiyangfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot closed this in

54107ae

ezyang added the merged label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ailzhang ailzhang left review comments

facebook-github-bot facebook-github-bot left review comments

ssnl ssnl approved these changes

apaszke Awaiting requested review from apaszke

colesbury Awaiting requested review from colesbury

ezyang Awaiting requested review from ezyang

gchanan Awaiting requested review from gchanan

soumith Awaiting requested review from soumith

zdevito Awaiting requested review from zdevito

pietern Awaiting requested review from pietern

+2 more reviewers

teng-li teng-li left review comments

li-roy li-roy approved these changes

Labels

ready for review (this tag is deprecated)