Add allgather_base as per our discussion re: ProcessGroup interface. #31892

agolynski · 2020-01-06T19:42:14Z

Summary: Introduce ProcessGroup::allgather_base. No implementation yet: plan to add it one PG backend at a time in a follow up.

Test Plan: No functional changes, no tests yet.

Differential Revision: D19290739

facebook-github-bot · 2020-01-06T19:42:37Z

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

facebook-github-bot · 2020-01-06T19:49:55Z

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

facebook-github-bot · 2020-01-06T19:51:25Z

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

kostmo · 2020-01-06T20:12:53Z

💊 CircleCI build failures summary and remediations

As of commit 287e6a1:

None of the build failures appear to be your fault.

1/1 broken upstream at merge base 3d01e3d since Jan 15
Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch origin viable/strict
git rebase --onto viable/strict $(git merge-base origin/master HEAD)
```
If your commit is older than viable/strict:
```
git fetch origin viable/strict
git rebase viable/strict
```
Check out the recency history of this "viable master" tracking branch.

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

🚧 1 upstream failure recognized by patterns:

These builds matched patterns, but were probably caused by upstream breakages:

pytorch_xla_linux_xenial_py3_6_clang7_build from Jan 15

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 on the GitHub issue tracker.

This comment has been revised 24 times.

facebook-github-bot · 2020-01-06T20:16:08Z

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

torch/lib/c10d/ProcessGroup.cpp

torch/lib/c10d/ProcessGroup.hpp

pietern · 2020-01-07T08:54:31Z

torch/lib/c10d/ProcessGroup.hpp

@@ -128,6 +128,15 @@ class ProcessGroup {
      std::vector<at::Tensor>& inputTensors,
      const AllgatherOptions& opts = AllgatherOptions()) = 0;

+  // Q: Should we move it to protected or annotate with _ since it can be
+  // confusing to use and is intended for internal use only?


We could expose it for power users. I think it's fine to leave public. Btw, didn't we call it single before?

I think we agreed on _base during our meeting with @mrshenli

facebook-github-bot · 2020-01-08T17:25:04Z

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

facebook-github-bot · 2020-01-08T17:27:29Z

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

mrshenli

@pietern this looks OK to me. Is this sufficient to unblock #28068?

torch/lib/c10d/ProcessGroup.hpp

pietern · 2020-01-09T12:05:40Z

@mrshenli We need to remove the coalesced variant first as well.

mrshenli · 2020-01-09T16:52:33Z

Try to understand the minimum requirements to unblock #28068

The allgather_coalesced API is already in the public API (but not in doc yet), is it OK to remove it? Or do we have to replace it with an implementation in c10d/comm.h/cpp?
I guess this also applies to the allreduce_coalesced API. Do we need to replace that with allreduce_base too?

@pietern @agolynski

agolynski · 2020-01-09T17:36:46Z

Discussed with @mrshenli: If we just remove allgather_coalesced it'll break python API right now. How about we leave it and add comments not to touch it: "deprecated. do not use it. do not implement it".

Let's do similar exercise with allreduce_coalesced in a follow up?

facebook-github-bot · 2020-01-09T17:42:45Z

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

facebook-github-bot · 2020-01-09T20:20:27Z

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

agolynski · 2020-01-09T20:32:12Z

This pull request was exported from Phabricator. Differential Revision: [D19290739](https://phabricator.intern.facebook.com/D19290739)

Fixed broken build

pietern

CI failure is benign. Good to go. Thanks, @agolynski!

…ytorch#31892) Summary: Pull Request resolved: pytorch#31892 Introduce ProcessGroup::allgather_base. No implementation yet: plan to add it one PG backend at a time in a follow up. Test Plan: No functional changes, no tests yet. Differential Revision: D19290739 fbshipit-source-id: ec91720ef4f1f6aa445836d8153324ae769cdfd1

facebook-github-bot · 2020-01-15T20:16:19Z

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

facebook-github-bot · 2020-01-15T20:16:50Z

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

facebook-github-bot

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

facebook-github-bot · 2020-01-15T23:47:37Z

@agolynski merged this pull request in 74621ca.

…ytorch#31892) Summary: Introduce ProcessGroup::allgather_base. No implementation yet: plan to add it one PG backend at a time in a follow up. Pull Request resolved: pytorch#31892 Test Plan: No functional changes, no tests yet. Differential Revision: D19290739 Pulled By: agolynski fbshipit-source-id: c2f4947d2980995724c539de7c6d97618e1ba11a

agolynski requested review from mrshenli, pietern and zhaojuanmao as code owners January 6, 2020 19:42

facebook-github-bot added the fb-exported label Jan 6, 2020

agolynski force-pushed the export-D19290739 branch from 0e91a55 to 59263ef Compare January 6, 2020 19:43

agolynski force-pushed the export-D19290739 branch from 59263ef to 45252ca Compare January 6, 2020 19:49

agolynski force-pushed the export-D19290739 branch from 45252ca to 9e7d7ac Compare January 6, 2020 19:51

agolynski force-pushed the export-D19290739 branch from 9e7d7ac to 086bfb5 Compare January 6, 2020 20:16

pietern reviewed Jan 7, 2020

View reviewed changes

agolynski force-pushed the export-D19290739 branch from 086bfb5 to d586da2 Compare January 8, 2020 17:25

agolynski force-pushed the export-D19290739 branch from d586da2 to 34d7311 Compare January 8, 2020 17:27

mrshenli reviewed Jan 8, 2020

View reviewed changes

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

agolynski force-pushed the export-D19290739 branch from 34d7311 to 940e74f Compare January 9, 2020 17:42

agolynski force-pushed the export-D19290739 branch from 940e74f to 113cf3d Compare January 9, 2020 20:20

pietern approved these changes Jan 15, 2020

View reviewed changes

agolynski force-pushed the export-D19290739 branch from 113cf3d to d321a4e Compare January 15, 2020 20:16

agolynski force-pushed the export-D19290739 branch from d321a4e to 287e6a1 Compare January 15, 2020 20:16

facebook-github-bot reviewed Jan 15, 2020

View reviewed changes

facebook-github-bot closed this in 74621ca Jan 15, 2020

facebook-github-bot added the merged label Jan 15, 2020

ddkalamk mentioned this pull request Jan 17, 2020

Add alltoall collective communication support to torch.distributed module #32345

Closed

mrshenli mentioned this pull request Jan 17, 2020

add c10d dynamic loading mechanism and unit test #28068

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allgather_base as per our discussion re: ProcessGroup interface. #31892

Add allgather_base as per our discussion re: ProcessGroup interface. #31892

agolynski commented Jan 6, 2020

facebook-github-bot commented Jan 6, 2020

facebook-github-bot commented Jan 6, 2020

facebook-github-bot commented Jan 6, 2020

kostmo commented Jan 6, 2020 •

edited

facebook-github-bot commented Jan 6, 2020

pietern Jan 7, 2020

agolynski Jan 7, 2020

facebook-github-bot commented Jan 8, 2020

facebook-github-bot commented Jan 8, 2020

mrshenli left a comment

pietern commented Jan 9, 2020

mrshenli commented Jan 9, 2020 •

edited

agolynski commented Jan 9, 2020

facebook-github-bot commented Jan 9, 2020

facebook-github-bot commented Jan 9, 2020

agolynski commented Jan 9, 2020

pietern left a comment

facebook-github-bot commented Jan 15, 2020

facebook-github-bot commented Jan 15, 2020

facebook-github-bot left a comment

facebook-github-bot commented Jan 15, 2020

Add allgather_base as per our discussion re: ProcessGroup interface. #31892

Add allgather_base as per our discussion re: ProcessGroup interface. #31892

Conversation

agolynski commented Jan 6, 2020

facebook-github-bot commented Jan 6, 2020

facebook-github-bot commented Jan 6, 2020

facebook-github-bot commented Jan 6, 2020

kostmo commented Jan 6, 2020 • edited

💊 CircleCI build failures summary and remediations

Detailed failure analysis

🚧 1 upstream failure recognized by patterns:

facebook-github-bot commented Jan 6, 2020

pietern Jan 7, 2020

Choose a reason for hiding this comment

agolynski Jan 7, 2020

Choose a reason for hiding this comment

facebook-github-bot commented Jan 8, 2020

facebook-github-bot commented Jan 8, 2020

mrshenli left a comment

Choose a reason for hiding this comment

pietern commented Jan 9, 2020

mrshenli commented Jan 9, 2020 • edited

agolynski commented Jan 9, 2020

facebook-github-bot commented Jan 9, 2020

facebook-github-bot commented Jan 9, 2020

agolynski commented Jan 9, 2020

pietern left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 15, 2020

facebook-github-bot commented Jan 15, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 15, 2020

kostmo commented Jan 6, 2020 •

edited

mrshenli commented Jan 9, 2020 •

edited