Support work.result() to get result tensors for allreduce for Gloo, NCCL backends (resubmission of #43386) #43970

mrzzd · 2020-09-01T18:37:29Z

Stack from ghstack:

Support work.result() to get result tensors for allreduce for Gloo, NCCL backends (resubmission of #43386) #43970 Support work.result() to get result tensors for allreduce for Gloo, NCCL backends (resubmission of Support work.result() to get result tensors for allreduce for Gloo, NCCL backends #43386)

Original commit changeset: 27fbeb161706

Differential Revision: D23455047

…rs for allreduce for Gloo, NCCL backends" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

dr-ci · 2020-09-01T18:57:28Z

💊 CI failures summary and remediations

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

3/3 failures possibly* introduced in this PR
- 1/3 non-CircleCI failure(s)

3 jobs timed out:

binary_linux_libtorch_3_7m_cu101_gcc5_4_cxx11-abi_nightly_shared-with-deps_build
binary_linux_libtorch_3_7m_cu101_gcc5_4_cxx11-abi_nightly_static-with-deps_build
binary_linux_libtorch_3_7m_cu101_gcc5_4_cxx11-abi_nightly_shared-without-deps_build

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.7-py3.6

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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 85 times.

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…rs for allreduce for Gloo, NCCL backends" Pull Request resolved: #43970 Original commit changeset: 27fbeb161706 ghstack-source-id: 111190323 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

pritamdamania87 · 2020-09-02T20:44:58Z

We can close out this PR since the original diff has been reverted?

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…rs for allreduce for Gloo, NCCL backends" Pull Request resolved: #43970 Original commit changeset: 27fbeb161706 ghstack-source-id: 111397109 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

pritamdamania87

If this is a resubmission of #43386, can we update the PR title and summary to reflect this appropriately?

.circleci/config.yml

pritamdamania87 · 2020-09-03T23:40:39Z

test/distributed/test_distributed.py

+
+            result = work.result()
+            expected_value = 2 + (10 * (len(group) - 1))
+            self.assertEqual(result, [_build_tensor(src + 1, expected_value)])


nit: Since allreduce is in place we can just check if result is equal to tensor?

nit: Since allreduce is in place we can just check if result is equal to tensor?

Being equal to tensor does not mean result is ready, it could be both of them have non-finalized values. I see that's being tested in other unitttests, but still, better to not depend on them.

We are calling work.wait() before the check though right? That should synchronize the appropriate streams to ensure this is safe to do.

torch/lib/c10d/ProcessGroupGloo.cpp

pritamdamania87 · 2020-09-03T23:48:24Z

torch/lib/c10d/ProcessGroupGloo.cpp

+  std::vector<at::Tensor> outputs_;
 };

 class AsyncAllreduceCoalescedWork : public AsyncAllreduceWork {


Apart from allreduce, there are other ops like barrier, scatter, gather, reduce etc. can we add this functionality to those as well? Can be in a separate PR on top of this.

Let's do it in separate PR

pritamdamania87 · 2020-09-03T23:48:53Z

torch/lib/c10d/ProcessGroupNCCL.cpp

+  bool result = exception() || finishedGPUExecutionInternal();
+  return result;


Why not have this in a single line as before?

It was a temporary change to add some log lines. Reverted.

pritamdamania87 · 2020-09-03T23:50:00Z

torch/lib/c10d/ProcessGroupNCCL.cpp

 }

+std::vector<at::Tensor> ProcessGroupNCCL::WorkNCCL::result() {
+  TORCH_CHECK(isCompleted());


This is not necessarily true for NCCL. isCompleted() returns true only when the work has completed successfully on the GPU. But we can still retrieve the result for the Work and perform async processing. I think we shouldn't have this check for NCCL.

Good point, removed.

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…rs for allreduce for Gloo, NCCL backends" Pull Request resolved: #43970 Original commit changeset: 27fbeb161706 ghstack-source-id: 111453596 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

mrzzd · 2020-09-04T23:23:22Z

Verified that failing test also passed: https://app.circleci.com/pipelines/github/pytorch/pytorch/210229/workflows/86bde47b-f2da-48e3-a618-566ae2713102/jobs/7253683

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…CCL backends Pull Request resolved: #43970 It is resubmition of #43386 Original commit changeset: 27fbeb161706 ghstack-source-id: 111494058 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

pritamdamania87

Looks good to me! Can we also update the PR summary appropriate? It currently still mentions "#43970 Back out "Revert D23216393: Support work.result() to get result tensors for allreduce for Gloo, NCCL backends"". Also, please check all the builds are clean before landing.

torch/lib/c10d/ProcessGroupGloo.cpp

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…CCL backends Pull Request resolved: #43970 It is resubmition of #43386 Original commit changeset: 27fbeb161706 ghstack-source-id: 111541355 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…CCL backends Pull Request resolved: #43970 It is resubmition of #43386 Original commit changeset: 27fbeb161706 ghstack-source-id: 111742817 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

…CCL backends Pull Request resolved: #43970 It is resubmition of #43386 Original commit changeset: 27fbeb161706 ghstack-source-id: 111775070 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/)

mrzzd · 2020-09-10T17:00:48Z

We can close out this PR since the original diff has been reverted?

I just rebased with latest changes in trunk. I am going to submit this as soon as testing finishes.

facebook-github-bot · 2020-09-11T02:14:36Z

This pull request has been merged in 2e744b1.

…CCL backends (#43970) Summary: Pull Request resolved: #43970 It is resubmition of #43386 Original commit changeset: 27fbeb161706 ghstack-source-id: 111775070 Test Plan: Added checks to existing unit test and ran it on gpu devserver. Verified the test that was failing in original diff also passes: https://app.circleci.com/pipelines/github/pytorch/pytorch/210229/workflows/86bde47b-f2da-48e3-a618-566ae2713102/jobs/7253683 Reviewed By: pritamdamania87 Differential Revision: D23455047 fbshipit-source-id: b8dc4a30b95570d68a482c19131674fff2a3bc7c

Back out "Revert D23216393: Support work.result() to get result tenso…

dfbccd8

…rs for allreduce for Gloo, NCCL backends" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

mrzzd requested review from mrshenli, pietern, pritamdamania87 and zhaojuanmao as code owners September 1, 2020 18:37

mrzzd added the ci/all label Sep 1, 2020

Update on "Back out "Revert D23216393: Support work.result() to get r…

2073d48

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

mrzzd added the ci-all/ label Sep 3, 2020

Update on "Back out "Revert D23216393: Support work.result() to get r…

2977dd7

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

pritamdamania87 suggested changes Sep 3, 2020

View reviewed changes

Update on "Back out "Revert D23216393: Support work.result() to get r…

b06c971

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Back out "Revert D23216393: Support work.result() to get r…

0ecf0ca

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Back out "Revert D23216393: Support work.result() to get r…

02149c7

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Back out "Revert D23216393: Support work.result() to get r…

b3cdf07

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Back out "Revert D23216393: Support work.result() to get r…

1973b17

…esult tensors for allreduce for Gloo, NCCL backends"" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

mrzzd requested a review from pritamdamania87 September 4, 2020 23:38

mrzzd changed the title ~~Back out "Revert D23216393: Support work.result() to get result tensors for allreduce for Gloo, NCCL backends"~~ Support work.result() to get result tensors for allreduce for Gloo, NCCL backends (resubmission of #43386) Sep 4, 2020

pritamdamania87 approved these changes Sep 5, 2020

View reviewed changes

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

Update on "Support work.result() to get result tensors for allreduce …

003a573

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Support work.result() to get result tensors for allreduce …

d0ced03

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Support work.result() to get result tensors for allreduce …

4170e60

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Support work.result() to get result tensors for allreduce …

347bd36

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

Update on "Support work.result() to get result tensors for allreduce …

faac5ee

…for Gloo, NCCL backends (resubmission of #43386)" Original commit changeset: 27fbeb161706 Differential Revision: [D23455047](https://our.internmc.facebook.com/intern/diff/D23455047/) [ghstack-poisoned]

facebook-github-bot closed this in 2e744b1 Sep 11, 2020

facebook-github-bot added the merged label Sep 11, 2020

facebook-github-bot deleted the gh/mrzzd/2/head branch September 14, 2020 14:17

mruberry added the Merged label Oct 28, 2020

		bool result = exception() \|\| finishedGPUExecutionInternal();
		return result;

Support work.result() to get result tensors for allreduce for Gloo, NCCL backends (resubmission of #43386) #43970

Support work.result() to get result tensors for allreduce for Gloo, NCCL backends (resubmission of #43386) #43970

Uh oh!

Conversation

mrzzd commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

pritamdamania87 commented Sep 2, 2020

Uh oh!

pritamdamania87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrzzd commented Sep 4, 2020

Uh oh!

pritamdamania87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrzzd commented Sep 10, 2020

Uh oh!

facebook-github-bot commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mrzzd commented Sep 1, 2020 •

edited

Loading

dr-ci bot commented Sep 1, 2020 •

edited

Loading