[c10d] Move sync_params to C++ #9805
Conversation
goldsborough
requested review from
apaszke,
colesbury,
ezyang,
gchanan,
pietern,
soumith,
teng-li and
zdevito
as code owners
|
@pytorchbot retest this please |
|
cc @teng-li would you like to review this PR too? |
|
@goldsborough We do have a test covering this change right? |
|
@teng-li I ran the c10d DDP test that I wrote. Should we also write some unit tests for these individual functions? |
goldsborough
force-pushed
the
ddp-sync-params
branch
3 times, most recently
from
e115f43 to
3523820
Compare
teng-li
left a comment
teng-li
left a comment
There was a problem hiding this comment.
Look great, thank you for adding those test cases.
Please see my comments.
test/test_c10d.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
test/test_c10d.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@pytorchbot retest this please |
goldsborough
force-pushed
the
ddp-sync-params
branch
2 times, most recently
from
c3851ae to
c031d29
Compare
|
@pytorchbot retest this please |
|
might possibly need a rebase. cc: @teng-li |
|
@pytorchbot retest this please |
1 similar comment
|
@pytorchbot retest this please |
goldsborough
force-pushed
the
ddp-sync-params
branch
3 times, most recently
from
21cd574 to
46bf4f1
Compare
Rebase fixes Rebase Wrote unit test for dist_broadcast_coalesced Wrote unit test for sync_params Make the bucke size 20 bytes and increase test timeout to 10s 5 tensors per device Increase timeout
goldsborough
force-pushed
the
ddp-sync-params
branch
from
56474d2 to
ec7d381
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: The next function I'm moving to C++ is `sync_params`. It is stacked on top of pytorch#9729, so some changes will go away when it lands and I rebase. I also split code into a `.h` and `.cpp` file for better code organization. The controller you requested could not be found. pietern apaszke Pull Request resolved: pytorch#9805 Differential Revision: D9688604 Pulled By: goldsborough fbshipit-source-id: 4467104d3f9e2354425503b9e4edbd59603e20a8
7 participants
The next function I'm moving to C++ is
sync_params. It is stacked on top of #9729, so some changes will go away when it lands and I rebase.I also split code into a
.hand.cppfile for better code organization.@teng-li @pietern @apaszke