Skip to content

[c10d] Move DDP broadcast coalesced to C++ #9729

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
goldsborough wants to merge 5 commits into from
Closed

[c10d] Move DDP broadcast coalesced to C++ #9729

goldsborough wants to merge 5 commits into from

Conversation

@goldsborough
Copy link
Contributor

@goldsborough goldsborough commented Jul 23, 2018

This PR depends on the tests added in #9670. It moves the first, tiny function from the c10d DDP to C++: dist_broadcast_coalesced. Let me know if torch/csrc/distributed/c10d/ddp.h will be a good place to put these rewritten functions.

@pietern @teng-li @apaszke

@goldsborough
Copy link
Contributor Author

goldsborough commented Jul 23, 2018

@pytorchbot retest this please

@goldsborough goldsborough force-pushed the ddp-broadcast-coalesced branch from 16fff4f to b23dd12 Compare July 24, 2018 21:16
teng-li
teng-li approved these changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

@teng-li teng-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please follow the c10d coding convention.

torch/csrc/distributed/c10d/ddp.h Outdated

This comment was marked as off-topic.

Sign in to view

@goldsborough
Copy link
Contributor Author

goldsborough commented Jul 24, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Jul 25, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@goldsborough
Copy link
Contributor Author

goldsborough commented Jul 25, 2018

@apaszke @ezyang can I get codeowner approval, thanks

This was referenced Jul 25, 2018
Closed
Closed
ezyang
ezyang reviewed Jul 25, 2018
View reviewed changes
torch/csrc/distributed/c10d/ddp.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke
apaszke approved these changes Jul 25, 2018
View reviewed changes
torch/csrc/distributed/c10d/ddp.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke
apaszke reviewed Jul 25, 2018
View reviewed changes
torch/csrc/distributed/c10d/init.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@goldsborough goldsborough force-pushed the ddp-broadcast-coalesced branch from b1bf41c to 23c6713 Compare July 30, 2018 17:48
@goldsborough
Copy link
Contributor Author

goldsborough commented Jul 30, 2018

@apaszke can you take a look at the new version?

@ailzhang
Copy link
Contributor

ailzhang commented Jul 31, 2018

@pytorchbot retest this please

teng-li
teng-li reviewed Jul 31, 2018
View reviewed changes
Copy link
Contributor

@teng-li teng-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks good to me.

Please run clang-format-7 -i on both files before landing

torch/csrc/distributed/c10d/ddp.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@goldsborough goldsborough force-pushed the ddp-broadcast-coalesced branch from 2cfa2a1 to 5108147 Compare July 31, 2018 23:08
@goldsborough
Copy link
Contributor Author

goldsborough commented Aug 1, 2018

clang-formatted and re-ran tests on FAIR cluster. Landing

facebook-github-bot
facebook-github-bot reviewed Aug 1, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@goldsborough goldsborough deleted the ddp-broadcast-coalesced branch August 1, 2018 16:18
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@goldsborough
Move DDP broadcast coalesced to C++ (pytorch#9729)
838a2ed 
Summary:
This PR depends on the tests added in pytorch#9670. It moves the first, tiny function from the c10d DDP to C++: `dist_broadcast_coalesced`. Let me know if ` torch/csrc/distributed/c10d/ddp.h` will be a good place to put these rewritten functions.

pietern The controller you requested could not be found. apaszke
Pull Request resolved: pytorch#9729

Differential Revision: D8985308

Pulled By: goldsborough

fbshipit-source-id: dc459fe9040273714044152063585e746974752f
facebook-github-bot pushed a commit that referenced this pull request Sep 7, 2018
@goldsborough @facebook-github-bot
Move sync_params to C++ (#9805)
01930a3 
Summary:
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 `.h` and `.cpp` file for better code organization.

The controller you requested could not be found. pietern apaszke
Pull Request resolved: #9805

Differential Revision: D9688604

Pulled By: goldsborough

fbshipit-source-id: 4467104d3f9e2354425503b9e4edbd59603e20a8
PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
@goldsborough @PenghuiCheng
Move sync_params to C++ (pytorch#9805)
4f97613 
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
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@ezyang ezyang ezyang approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@pietern pietern Awaiting requested review from pietern

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+2 more reviewers

@apaszke apaszke apaszke approved these changes

@teng-li teng-li teng-li approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@goldsborough @ailzhang @ezyang @apaszke @facebook-github-bot @teng-li