Skip allreducing `local_used_maps_dev_` when `find_unused_param=False` #40407

sinannasir · 2020-06-22T23:57:08Z

Stack from ghstack:

Skip allreducing local_used_maps_dev_ when find_unused_param=False #40407 Skip allreducing local_used_maps_dev_ when find_unused_param=False

Summary:

In reducer.cpp, we have a new boolean find_unused_param_ and its value is set in Reducer::prepare_for_backward.
If !find_unused_param_, then it avoids allreduce(local_used_maps_dev_).
Solves issue 38942.
Fixes incorrect find_unused_parameters_ passing like checking outputs.empty() or unused_parameters_.empty().

Test Plan:

Run test/distributed/test_c10d.py and make sure all tests pass.
A new test case test_find_unused_parameters_when_unused_parameters_empty is included. Old reducer.cpp was failing in that unit test because it was checking find_unused_parameters_ by unused_parameters_.empty(). Current reducer.cpp passes this unit test.
Two test cases were failing test_forward_backward_unused_parameters and test_forward_backward_optimizer , because find_unused_parameter_ of their reducer object was not set properly. I fixed that as well.

Tasks: T68705534

Tags: DDP

Differential Revision: D22176231

Summary: Solve issue #38942. In reducer.cpp, I check whether `find_unused_param` is set to False by !unused_parameters_.empty(). If !unused_parameters_.empty(), then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li Tasks: T68705534 Tags: DDP [ghstack-poisoned]

Summary: Solve issue #38942. In reducer.cpp, I check whether `find_unused_param` is set to False by !unused_parameters_.empty(). If !unused_parameters_.empty(), then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li Tasks: T68705534 Tags: DDP ghstack-source-id: 3da54f3 Pull Request resolved: #40407

dr-ci · 2020-06-23T00:41:54Z

💊 CI failures summary and remediations

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

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

Extra GitHub checks: 2 failed

Failed: GitHub Actions - clang-tidy
Failed: GitHub Actions - clang-tidy

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 27 times.

torch/csrc/distributed/c10d/reducer.cpp

mrshenli · 2020-06-23T17:48:25Z

And since the unused parameters from different processes can be different, let's also add a test for that case.

…param=False`" Summary: 1. Solves issue [38942](#38942). 2. In reducer.cpp, I check whether `find_unused_param` is set to `false` by `!unused_parameters_.empty()`. If `!unused_parameters_.empty()`, then it avoids `allreduce(local_used_maps_dev_)`. 3. `unused_parameters_()` is always empty, as we set `find_unused_param = false`. Further, there may be additional cases with an empty `unused_parameters_`, but `find_unused_param = true`. Therefore, in addition to the issue [38942](#38942), this diff also avoids `allreduce(local_used_maps_dev_)` for that case. I think that since we never call `unused_parameters_.clear()` in separate processes, this will not cause any issue and will not lead to DDP comm hang. In fact, it will be more beneficial, because if all parameters are used, then each should already be reduced. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

Pull Request resolved: #40407 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. Differential Revision: [D22176231](https://our.internmc.facebook.com/intern/diff/D22176231/) ghstack-source-id: 106447051

torch/csrc/distributed/c10d/reducer.cpp

…param=False`" Summary: 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li, Yanli Zhao Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

Pull Request resolved: #40407 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. ghstack-source-id: 106478491 Differential Revision: [D22176231](https://our.internmc.facebook.com/intern/diff/D22176231/)

pritamdamania87

And since the unused parameters from different processes can be different, let's also add a test for that case.

@mrshenli Could you elaborate on the test case we need here? Looks like we have unit tests where we test both cases for find_unused_parameters_. Is there something missing in our current tests?

torch/csrc/distributed/c10d/reducer.cpp

…param=False`" Summary: 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li, Yanli Zhao Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

Pull Request resolved: #40407 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. ghstack-source-id: 106487672 Differential Revision: [D22176231](https://our.internmc.facebook.com/intern/diff/D22176231/)

torch/csrc/distributed/c10d/reducer.cpp

mrshenli · 2020-06-24T15:02:23Z

@mrshenli Could you elaborate on the test case we need here? Looks like we have unit tests where we test both cases for find_unused_parameters_. Is there something missing in our current tests?

Sure. The first two attempts in this PR reminds us that we might want to add tests for the following cases:

unused_parameters_.empty() does not imply find_unused_parameters: we can set find_unused_parameters to true, and then let some some process use all parameters but other don't.
outputs in prepare_for_backward does not imply find_unused_parameters: the code today actually implies find_unused_parameters using outputs. However, as we know explicitly passing find_unused_parameters, we can modify the following code accordingly and add tests for it, where the forward pass in some process produces empty outputs.:

pytorch/torch/csrc/distributed/c10d/reducer.cpp

Lines 751 to 756 in e490352

    
           // If no outputs are specified, we assume that autograd hooks for ALL 
        
           // variables will be called, and we don't have to search the autograd graph 
        
           // for presence of these hooks. 
        
           if (outputs.empty()) { 
        
             return; 
        
           }

…param=False`" Summary: 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li, Yanli Zhao Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

sinannasir · 2020-06-24T18:36:45Z

All tests pass and we don't allocate memory for local_used_maps and local_used_maps_dev_ if find_unused_parameters is false.
Fixed declaring global_unused twice when find_unused_parameters_ = true.
Correctly passing find_unused_parameters for test case test_forward_backward_unused_parameters.
Included Note [Skip allreducing local_used_maps_dev_] for reference.

…param=False`" Summary: 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li, Yanli Zhao Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

Pull Request resolved: #40407 1. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in the Reducer's Python API. 2. Solves issue [38942](#38942). When find_unused_parameters_ is set to false, there is no need to allreduce local_used_maps_dev_, because all parameters will be reduced anyway. Therefore, we can avoid allocating memory for local_used_maps and local_used_maps_dev_ if find_unused_parameters_ is false. ghstack-source-id: 106525355 Differential Revision: [D22176231](https://our.internmc.facebook.com/intern/diff/D22176231/)

sinannasir · 2020-06-24T19:13:11Z

Avoided some VS Code editorial changes.

sinannasir · 2020-06-24T21:03:06Z

@mrshenli Could you elaborate on the test case we need here? Looks like we have unit tests where we test both cases for find_unused_parameters_. Is there something missing in our current tests?

Sure. The first two attempts in this PR reminds us that we might want to add tests for the following cases:

unused_parameters_.empty() does not imply find_unused_parameters: we can set find_unused_parameters to true, and then let some some process use all parameters but other don't.

outputs in prepare_for_backward does not imply find_unused_parameters: the code today actually implies find_unused_parameters using outputs. However, as we know explicitly passing find_unused_parameters, we can modify the following code accordingly and add tests for it, where the forward pass in some process produces empty outputs.:

pytorch/torch/csrc/distributed/c10d/reducer.cpp

Lines 751 to 756 in e490352

// If no outputs are specified, we assume that autograd hooks for ALL

// variables will be called, and we don't have to search the autograd graph

// for presence of these hooks.

if (outputs.empty()) {

return;

}

I am looking into it.

…param=False`" Summary: 1. Solves issue [38942](#38942). 2. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. Test Plan: Run `test/distributed/test_c10d.py` and make sure all tests pass. Reviewers: Pritam Damania Subscribers: Pritam Damania, Shen Li, Yanli Zhao Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

Pull Request resolved: #40407 1. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in the Reducer's Python API. 2. Solves issue [38942](#38942). When find_unused_parameters_ is set to false, there is no need to allreduce local_used_maps_dev_, because all parameters will be reduced anyway. Therefore, we can avoid allocating memory for local_used_maps and local_used_maps_dev_ if find_unused_parameters_ is false. ghstack-source-id: 106644409 Differential Revision: [D22176231](https://our.internmc.facebook.com/intern/diff/D22176231/)

sinannasir · 2020-06-26T00:23:04Z

The new changes in the final commit:

A new test case test_find_unused_parameters_when_unused_parameters_empty is included. Today's reducer.cpp is failing in that unit test because it is checking find_unused_parameters_ by using unused_parameters_:

pytorch/torch/csrc/distributed/c10d/reducer.cpp

Lines 380 to 396 in e490352

    
           if (!has_rebuilt_bucket_ && unused_parameters_.empty() && 
        
               index.replica_index == 0) { 
        
             rebuilt_params_.push_back( 
        
                 replicas_[index.replica_index][index.variable_index]); 
        
             rebuilt_param_indices_.push_back(index.variable_index); 
        
           } 
        
           // If there are model parameters that went unused when computing the model 
        
           // output, they won't be part of the autograd graph, and won't receive 
        
           // gradients. These parameters are discovered in the `prepare_for_backward` 
        
           // function and their indexes stored in the `unused_parameters_` vector. 
        
           if (!has_marked_unused_parameters_ && !unused_parameters_.empty()) { 
        
             has_marked_unused_parameters_ = true; 
        
             for (const auto& unused_index : unused_parameters_) { 
        
               mark_variable_ready(unused_index); 
        
             } 
        
           }

The problem is with backward, but the exception is happening in the constructor of the next ddp_model. This is because the broadcast process_group_->broadcast(vec)->wait(); starts to pass weird control_accessor entries after the first problematic backward call. This may have something to do with gloo process group. The test now passes after new modifications.

Writing a test scenario for the following statement is not possible because of If all parameters are unused by forward pass in a process, backward will not work with DDP. #40566

outputs in prepare_for_backward does not imply find_unused_parameters: the code today actually implies find_unused_parameters using outputs.
I still changed the way find_parameters_unused_ is checked in the prepare_for_backward function.

Two test cases were failing test_forward_backward_unused_parameters and test_forward_backward_optimizer , because find_unused_parameter_ of their reducer object was not set properly. I fixed that as well.

pritamdamania87

Looks like there are some merge conflicts in CI you probably need to rebase and resubmit the PR.

pritamdamania87 · 2020-06-26T04:47:56Z

torch/csrc/distributed/c10d/reducer.cpp

+  if (!has_rebuilt_bucket_ && !find_unused_parameters_ &&
      index.replica_index == 0) {
    rebuilt_params_.push_back(
        replicas_[index.replica_index][index.variable_index]);
    rebuilt_param_indices_.push_back(index.variable_index);
  }

-  // If there are model parameters that went unused when computing the model
-  // output, they won't be part of the autograd graph, and won't receive
-  // gradients. These parameters are discovered in the `prepare_for_backward`
-  // function and their indexes stored in the `unused_parameters_` vector.
-  if (!has_marked_unused_parameters_ && !unused_parameters_.empty()) {
+  // If `find_unused_parameters_` is true there may be model parameters that
+  // went unused when computing the model output, they won't be part of the
+  // autograd graph, and won't receive gradients. These parameters are discovered
+  // in the `prepare_for_backward` function and their indexes stored in
+  // the `unused_parameters_` vector.
+  if (!has_marked_unused_parameters_ && find_unused_parameters_) {


These changes look good to me, although I'll let Shen stamp the PR.

Thanks for letting me know about the merge conflict :) Just rebased and resolved the conflict at init.cpp.

…param=False`" Summary: 1. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. 2. Solves issue [38942](#38942). 3. Fixes incorrect `find_unused_parameters_` passing like checking `outputs.empty()` or `unused_parameters_.empty()`. Test Plan: 1. Run `test/distributed/test_c10d.py` and make sure all tests pass. 2. A new test case `test_find_unused_parameters_when_unused_parameters_empty` is included. Old `reducer.cpp` was failing in that unit test because it was checking `find_unused_parameters_` by `unused_parameters_.empty()`. Current `reducer.cpp` passes this unit test. 3. Two test cases were failing `test_forward_backward_unused_parameters` and `test_forward_backward_optimizer` , because `find_unused_parameter_` of their `reducer` object was not set properly. I fixed that as well. Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

zhaojuanmao

overall lgtm

torch/csrc/distributed/c10d/init.cpp

…param=False`" Summary: 1. In reducer.cpp, we have a new boolean `find_unused_param_` and its value is set in `Reducer::prepare_for_backward`. If `!find_unused_param_`, then it avoids `allreduce(local_used_maps_dev_)`. 2. Solves issue [38942](#38942). 3. Fixes incorrect `find_unused_parameters_` passing like checking `outputs.empty()` or `unused_parameters_.empty()`. Test Plan: 1. Run `test/distributed/test_c10d.py` and make sure all tests pass. 2. A new test case `test_find_unused_parameters_when_unused_parameters_empty` is included. Old `reducer.cpp` was failing in that unit test because it was checking `find_unused_parameters_` by `unused_parameters_.empty()`. Current `reducer.cpp` passes this unit test. 3. Two test cases were failing `test_forward_backward_unused_parameters` and `test_forward_backward_optimizer` , because `find_unused_parameter_` of their `reducer` object was not set properly. I fixed that as well. Tasks: T68705534 Tags: DDP Differential Revision: [D22176231](https://www.internalfb.com/intern/diff/D22176231/) [ghstack-poisoned]

mrshenli

LGTM! Thanks for working on this!

mrshenli · 2020-06-26T23:16:25Z

torch/csrc/distributed/c10d/reducer.cpp

  }
 }

+// Note [Skip allreducing local_used_maps_dev]


Just curious, any reason for adding this note to the dtor?

In Pritam's example, the notes were placed after the constructor. Thats why I put it there.
https://github.com/pytorch/pytorch/blob/master/torch/csrc/autograd/engine.cpp#L100. Example of referring to a note: https://github.com/pytorch/pytorch/blob/master/torch/csrc/autograd/engine.cpp#L64.

mrshenli · 2020-06-26T23:24:03Z

test/distributed/test_c10d.py

+            output.mean().backward()
+
+            # Now locally unused parameter should have grad updated on all ranks.
+            [self.assertIsNotNone(t_p.grad) for t_p in model.module.task_parameters()]


nit: I wonder if we should also verify that the grad values are as expected? We can do this by

creating two local models, one's forward uses both t0 and t1, and the other one's forward only uses t1

run forward/backward on them, and then compute the mean grads of these two models

Compare the mean grad with DDP's grad.

I noticed the test above (test_global_local_unused_params_grad) didn't test the accuracy either. So feel free to do this in a followup PR.

I noted this and do a followup soon.

rohan-varma · 2020-06-29T18:14:02Z

test/distributed/test_c10d.py

+        process_group = c10d.ProcessGroupGloo(store, self.rank, self.world_size)
+
+        # Test on CPU
+        cpu_model = DistributedDataParallel(


nit: In a follow up PR, It may be worth separating the GPU and CPU test so that (a portion of) this test can be run in CPU-only builds?

Yes, I noted that.

sinannasir requested review from mrshenli, pietern and zhaojuanmao as code owners June 22, 2020 23:57

sinannasir requested a review from pritamdamania87 June 23, 2020 00:45

mrshenli requested changes Jun 23, 2020

View reviewed changes

torch/csrc/distributed/c10d/reducer.cpp Outdated Show resolved Hide resolved

sinannasir requested a review from mrshenli June 23, 2020 20:54

mrshenli reviewed Jun 23, 2020

View reviewed changes

torch/csrc/distributed/c10d/reducer.cpp Outdated Show resolved Hide resolved

sinannasir requested a review from apaszke as a code owner June 24, 2020 02:05

pritamdamania87 reviewed Jun 24, 2020

View reviewed changes

torch/csrc/distributed/c10d/reducer.cpp Show resolved Hide resolved

mrshenli reviewed Jun 24, 2020

View reviewed changes

torch/csrc/distributed/c10d/reducer.cpp Outdated Show resolved Hide resolved

sinannasir requested review from mrshenli, pritamdamania87 and zhaojuanmao and removed request for zhaojuanmao June 24, 2020 15:53

sinannasir mentioned this pull request Jun 25, 2020

If all parameters are unused by forward pass in a process, backward will not work with DDP. #40566

Open

pritamdamania87 reviewed Jun 26, 2020

View reviewed changes

zhaojuanmao reviewed Jun 26, 2020

View reviewed changes

torch/csrc/distributed/c10d/init.cpp Outdated Show resolved Hide resolved

mrshenli approved these changes Jun 26, 2020

View reviewed changes

sinannasir closed this Jun 29, 2020

sinannasir added the merged label Jun 29, 2020

rohan-varma reviewed Jun 29, 2020

View reviewed changes

facebook-github-bot deleted the gh/sinannasir/1/head branch July 30, 2020 14:17

Skip allreducing local_used_maps_dev_ when find_unused_param=False #40407

Skip allreducing local_used_maps_dev_ when find_unused_param=False #40407

Uh oh!

Conversation

sinannasir commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Extra GitHub checks: 2 failed

Uh oh!

Uh oh!

mrshenli commented Jun 23, 2020

Uh oh!

Uh oh!

pritamdamania87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrshenli commented Jun 24, 2020

Uh oh!

sinannasir commented Jun 24, 2020

Uh oh!

sinannasir commented Jun 24, 2020

Uh oh!

sinannasir commented Jun 24, 2020

Uh oh!

sinannasir commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pritamdamania87 left a comment

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!

zhaojuanmao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrshenli left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Skip allreducing `local_used_maps_dev_` when `find_unused_param=False` #40407

Skip allreducing `local_used_maps_dev_` when `find_unused_param=False` #40407

sinannasir commented Jun 22, 2020 •

edited

Loading

dr-ci bot commented Jun 23, 2020 •

edited

Loading

sinannasir commented Jun 26, 2020 •

edited

Loading