[torch distributed] Implementing all_gather_base (#56304) by liangluofb · Pull Request #56315 · pytorch/pytorch

liangluofb · 2021-04-17T01:21:57Z

Summary:
Pull Request resolved: #56304

This diff implements the all_gather_base in pytorch distributed.

Test Plan: dist.all_gather_base(output, input)...

Differential Revision: D27488999

facebook-github-bot · 2021-04-17T01:22:03Z

💊 CI failures summary and remediations

As of commit 98c7966 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

facebook-github-bot · 2021-04-17T01:22:11Z

This pull request was exported from Phabricator. Differential Revision: D27488999

facebook-github-bot · 2021-04-17T01:50:54Z

This pull request was exported from Phabricator. Differential Revision: D27488999

codecov · 2021-04-17T06:31:10Z

Codecov Report

Merging #56315 (59f4e8f) into master (ffdecc1) will decrease coverage by 0.00%.
The diff coverage is 21.05%.

❗ Current head 59f4e8f differs from pull request most recent head 19a8630. Consider uploading reports for the commit 19a8630 to get more accurate results

@@            Coverage Diff             @@
##           master   #56315      +/-   ##
==========================================
- Coverage   77.02%   77.02%   -0.01%     
==========================================
  Files        1924     1923       -1     
  Lines      190590   190521      -69     
==========================================
- Hits       146803   146748      -55     
+ Misses      43787    43773      -14

zhaojuanmao

@liangluofb thanks for your contribution! the codes overall look good, I left some minor comments.

Also we've discussed internally, we would like to make 'all_gather_base' to be private APIs and named as '_all_gather_base' in both python and C++. As @agolynski had a C10d API cleanup plans to merge all_gather and all_gather_base. Before that cleanup is done, we would like not to introduce a new set of APIs.

So would you please help changing the 'all_gather_base' to be '_all_gather_base' in both python and C++? thanks

torch/_C/_distributed_c10d.pyi

torch/distributed/distributed_c10d.py

torch/lib/c10d/ProcessGroupNCCL.cpp

facebook-github-bot · 2021-04-20T00:42:04Z

This pull request was exported from Phabricator. Differential Revision: D27488999

facebook-github-bot · 2021-04-20T02:23:42Z

This pull request was exported from Phabricator. Differential Revision: D27488999

torch/lib/c10d/ProcessGroup.hpp

torch/lib/c10d/ProcessGroupNCCL.cpp

torch/lib/c10d/ProcessGroup.hpp

agolynski

Mostly LG, just some minor comments thank you!

facebook-github-bot · 2021-04-21T18:42:47Z

This pull request was exported from Phabricator. Differential Revision: D27488999

torch/lib/c10d/ProcessGroupNCCL.cpp

facebook-github-bot · 2021-04-22T00:10:25Z

This pull request was exported from Phabricator. Differential Revision: D27488999

facebook-github-bot · 2021-04-22T19:57:00Z

This pull request was exported from Phabricator. Differential Revision: D27488999

facebook-github-bot · 2021-04-23T03:33:19Z

This pull request was exported from Phabricator. Differential Revision: D27488999

Summary: Pull Request resolved: pytorch#56315 This diff implements the all_gather_base in pytorch distributed. Test Plan: dist.all_gather_base(output, input)... Reviewed By: agolynski, amylittleyang Differential Revision: D27488999 fbshipit-source-id: 4b286563301324f9212c0e31f8540712453441e2

facebook-github-bot · 2021-04-23T20:16:59Z

This pull request was exported from Phabricator. Differential Revision: D27488999

facebook-github-bot · 2021-04-23T21:18:17Z

This pull request has been merged in c370957.

Summary: Pull Request resolved: pytorch#56315 This diff implements the all_gather_base in pytorch distributed. Test Plan: dist.all_gather_base(output, input)... Reviewed By: agolynski, amylittleyang Differential Revision: D27488999 fbshipit-source-id: 937ec8bddf9527fa4d114f984d1d0f6a5b8c3936

zarzen · 2021-07-16T16:42:16Z

Hi,
would you mind to change the function signature of _allgather_base to accept two list tensors? so that multiple tensor allgather can be launched in a single ncclGroup? and the current collective interface requires std::vector<at::Tensor> as inputs anyway.
I can submit a PR if you would agree on the change.

c10::intrusive_ptr<ProcessGroup::Work> ProcessGroupNCCL::_allgather_base(
     std::vector<at::Tensor>& outputs,
    std::vector<at::Tensor>& inputs,
    const AllgatherOptions& /*unused */)

At the python front end we can do a type checking, if the input is a single tensor then wrap it with the brackets.

def allgather_base(self, outputs, inputs, ... ):
    if type(output) != list:
        outputs = [outputs]
        inputs = [inputs]
    ...

liangluofb requested review from H-Huang, mingzhe09088 and mrshenli as code owners April 17, 2021 01:21

liangluofb force-pushed the export-D27488999 branch from aea915a to 2e81eb8 Compare April 17, 2021 01:21

liangluofb requested review from pritamdamania87, rohan-varma, wayi1 and zhaojuanmao as code owners April 17, 2021 01:21

facebook-github-bot added the cla signed label Apr 17, 2021

facebook-github-bot added oncall: distributed Add this issue/PR to distributed oncall triage queue fb-exported labels Apr 17, 2021

liangluofb force-pushed the export-D27488999 branch from 2e81eb8 to 59f4e8f Compare April 17, 2021 01:50

zhaojuanmao requested a review from agolynski April 19, 2021 18:04

zhaojuanmao reviewed Apr 19, 2021

View reviewed changes

torch/_C/_distributed_c10d.pyi Outdated Show resolved Hide resolved

torch/distributed/distributed_c10d.py Outdated Show resolved Hide resolved

torch/lib/c10d/ProcessGroupNCCL.cpp Outdated Show resolved Hide resolved

liangluofb force-pushed the export-D27488999 branch from 59f4e8f to 19a8630 Compare April 20, 2021 00:42

liangluofb force-pushed the export-D27488999 branch from 19a8630 to dbacc5e Compare April 20, 2021 02:23

agolynski reviewed Apr 20, 2021

View reviewed changes

torch/lib/c10d/ProcessGroup.hpp Outdated Show resolved Hide resolved

agolynski reviewed Apr 21, 2021

View reviewed changes

torch/lib/c10d/ProcessGroupNCCL.cpp Outdated Show resolved Hide resolved

agolynski reviewed Apr 21, 2021

View reviewed changes

torch/lib/c10d/ProcessGroup.hpp Outdated Show resolved Hide resolved

agolynski reviewed Apr 21, 2021

View reviewed changes

liangluofb force-pushed the export-D27488999 branch from dbacc5e to 9af3fcf Compare April 21, 2021 18:42

agolynski reviewed Apr 21, 2021

View reviewed changes

torch/lib/c10d/ProcessGroupNCCL.cpp Outdated Show resolved Hide resolved

agolynski approved these changes Apr 21, 2021

View reviewed changes

liangluofb force-pushed the export-D27488999 branch from 9af3fcf to d357231 Compare April 22, 2021 00:10

liangluofb force-pushed the export-D27488999 branch from d357231 to e7081bc Compare April 22, 2021 19:57

liangluofb force-pushed the export-D27488999 branch from e7081bc to 77fe978 Compare April 23, 2021 03:33

liangluofb force-pushed the export-D27488999 branch from 77fe978 to 98c7966 Compare April 23, 2021 20:17

agolynski approved these changes Apr 23, 2021

View reviewed changes

facebook-github-bot closed this in c370957 Apr 23, 2021

facebook-github-bot added the Merged label Apr 23, 2021

zarzen mentioned this pull request Jul 23, 2021

ZeRO3, improved parameter all-gather operation deepspeedai/DeepSpeed#1188

Merged

kwen2501 mentioned this pull request Aug 4, 2022

all_gather supporting single tensor output #82639

Closed

Conversation

liangluofb commented Apr 17, 2021

Uh oh!

facebook-github-bot commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

facebook-github-bot commented Apr 17, 2021

Uh oh!

facebook-github-bot commented Apr 17, 2021

Uh oh!

codecov bot commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zhaojuanmao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Apr 20, 2021

Uh oh!

facebook-github-bot commented Apr 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agolynski left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 21, 2021

Uh oh!

Uh oh!

facebook-github-bot commented Apr 22, 2021

Uh oh!

facebook-github-bot commented Apr 22, 2021

Uh oh!

facebook-github-bot commented Apr 23, 2021

Uh oh!

facebook-github-bot commented Apr 23, 2021

Uh oh!

facebook-github-bot commented Apr 23, 2021

Uh oh!

zarzen commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

facebook-github-bot commented Apr 17, 2021 •

edited

Loading

codecov bot commented Apr 17, 2021 •

edited

Loading