Prioritize raising error message about unused parameters when rebuild_buckets fails #45933

rohan-varma · 2020-10-06T23:36:46Z

Stack from ghstack:

[NCCL] Provide additional information about NCCL error codes. #45950 [NCCL] Provide additional information about NCCL error codes.
Only populate grad accumulator to var mapping for find_unused_parameters=True in DDP #45942 Only populate grad accumulator to var mapping for find_unused_parameters=True in DDP
Prioritize raising error message about unused parameters when rebuild_buckets fails #45933 Prioritize raising error message about unused parameters when rebuild_buckets fails

Occasionally users run DDP with models with unused params, in this
case we would like to surface an error message telling them to run with
find_unused_params=True. However, a recent change to rebuild_buckets logic (#44798) made
it so that we raise a size mismatch error when this happens, but the
information about unused parameters is likely to be more useful and likely to
be the most common case of failure. Prefer raising this error over the
subsequent size mismatch errors.

Differential Revision: D24151256

…_buckets fails Occasionally users run DDP with models with unused params, in this case we would like to surface an error message telling them to run with find_unused_params=True. However, a recent change to rebuild_buckets logic (#44798) made it so that we raise a size mismatch error when this happens, but the information about unused parameters is likely to be more useful and likely to be the most common case of failure. Prefer raising this error over the subsequent size mismatch errors. Differential Revision: [D24151256](https://our.internmc.facebook.com/intern/diff/D24151256/) [ghstack-poisoned]

…hen rebuild_buckets fails" Occasionally users run DDP with models with unused params, in this case we would like to surface an error message telling them to run with find_unused_params=True. However, a recent change to rebuild_buckets logic (#44798) made it so that we raise a size mismatch error when this happens, but the information about unused parameters is likely to be more useful and likely to be the most common case of failure. Prefer raising this error over the subsequent size mismatch errors. Differential Revision: [D24151256](https://our.internmc.facebook.com/intern/diff/D24151256/) [ghstack-poisoned]

…_buckets fails Pull Request resolved: #45933 Occasionally users run DDP with models with unused params, in this case we would like to surface an error message telling them to run with find_unused_params=True. However, a recent change to rebuild_buckets logic (#44798) made it so that we raise a size mismatch error when this happens, but the information about unused parameters is likely to be more useful and likely to be the most common case of failure. Prefer raising this error over the subsequent size mismatch errors. ghstack-source-id: 113713050 Differential Revision: [D24151256](https://our.internmc.facebook.com/intern/diff/D24151256/)

mrshenli

LGTM!

mrshenli · 2020-10-07T02:10:30Z

torch/csrc/distributed/c10d/reducer.cpp

+  // has unused parameters for example, this will raise an error recommending to
+  // run with find_unused_parameters=True, instead of the size mismatch exception
+  // below.
+  ensure_prior_reduction_finished();


looks like self.reducer._rebuild_buckets is always invoked. Will it be sufficient if we move this to the front of the above if clause and then remove the other ensure_prior_reduction_finished invocation in prepare_for_backward?

Yes, I believe this should be okay. Originally I was concerned about require_backward_grad_sync flag, but I don't think its an issue. If we don't require_backward_grad_sync this iteration, we should still have the previous iteration reduction finished, where "previous iteration" here means the previous iteration that required grad sync, or the initial state of require_finalize_, which is false.

codecov · 2020-10-07T04:09:58Z

Codecov Report

❗ No coverage uploaded for pull request base (gh/rohan-varma/180/base@8a1e100). Click here to learn what that means.
The diff coverage is 26.31%.

@@                    Coverage Diff                     @@
##             gh/rohan-varma/180/base   #45933   +/-   ##
==========================================================
  Coverage                           ?   68.25%           
==========================================================
  Files                              ?      410           
  Lines                              ?    53251           
  Branches                           ?        0           
==========================================================
  Hits                               ?    36349           
  Misses                             ?    16902           
  Partials                           ?        0

Impacted Files	Coverage Δ
.../testing/_internal/distributed/distributed_test.py	`29.52% <26.31%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a1e100...f650e34. Read the comment docs.

…hen rebuild_buckets fails" Occasionally users run DDP with models with unused params, in this case we would like to surface an error message telling them to run with find_unused_params=True. However, a recent change to rebuild_buckets logic (#44798) made it so that we raise a size mismatch error when this happens, but the information about unused parameters is likely to be more useful and likely to be the most common case of failure. Prefer raising this error over the subsequent size mismatch errors. Differential Revision: [D24151256](https://our.internmc.facebook.com/intern/diff/D24151256/) [ghstack-poisoned]

dr-ci · 2020-10-08T23:50:06Z

💊 CI failures summary and remediations

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

✅ None of the CI failures appear to be your fault 💚

1/1 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_macos_10_13_py3_test (1/1)

Step: "Update Homebrew" (full log | diagnosis details | 🔁 rerun) ❄️

fatal: Could not read from remote repository.

remote: Total 60 (delta 51), reused 21 (delta 17), pack-reused 0         
Unpacking objects:  96% (58/60) Unpacking objects:  98% (59/60) Unpacking objects: 100% (60/60) Unpacking objects: 100% (60/60), 12.98 KiB | 309.00 KiB/s, done. 
From ssh://github.com/Homebrew/homebrew-cask-versions 
 + f127a2be3...882d4c8f3 master     -> origin/master  (forced update) 
+ git reset --hard origin/master 
HEAD is now at 882d4c8f3 vlc-nightly: update url do (#9707) 
+ for path in '$(find /usr/local/Homebrew -type d -name .git)' 
+ cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/.git/.. 
+ git fetch --depth=1 origin 
Connection to github.com closed by remote host.  
fatal: Could not read from remote repository. 
 
Please make sure you have the correct access rights 
and the repository exists.

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 1 time.

facebook-github-bot · 2020-10-09T18:30:39Z

This pull request has been merged in 62554a3.

facebook-github-bot · 2020-10-09T18:30:48Z

This pull request has been merged in 62554a3.

rohan-varma requested review from mingzhe09088, mrshenli, pietern, pritamdamania87 and zhaojuanmao as code owners October 6, 2020 23:36

facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Oct 6, 2020

rohan-varma mentioned this pull request Oct 7, 2020

Only populate grad accumulator to var mapping for find_unused_parameters=True in DDP #45942

Closed

mrshenli approved these changes Oct 7, 2020

View reviewed changes

rohan-varma mentioned this pull request Oct 7, 2020

[NCCL] Provide additional information about NCCL error codes. #45950

Closed

rohan-varma added 2 commits October 7, 2020 12:46

facebook-github-bot closed this in 62554a3 Oct 9, 2020

This was referenced Oct 9, 2020

[v1.7 patch] Prioritize raising error message about unused parameters when rebuild_buckets fails #46099

Merged

[v1.7.0] Release Tracker #45592

Closed

facebook-github-bot added the Merged label Oct 9, 2020

facebook-github-bot deleted the gh/rohan-varma/180/head branch October 13, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize raising error message about unused parameters when rebuild_buckets fails #45933

Prioritize raising error message about unused parameters when rebuild_buckets fails #45933

rohan-varma commented Oct 6, 2020 •

edited

mrshenli left a comment

mrshenli Oct 7, 2020

rohan-varma Oct 7, 2020

codecov bot commented Oct 7, 2020 •

edited

dr-ci bot commented Oct 8, 2020 •

edited

facebook-github-bot commented Oct 9, 2020

facebook-github-bot commented Oct 9, 2020

Prioritize raising error message about unused parameters when rebuild_buckets fails #45933

Prioritize raising error message about unused parameters when rebuild_buckets fails #45933

Conversation

rohan-varma commented Oct 6, 2020 • edited

mrshenli left a comment

Choose a reason for hiding this comment

mrshenli Oct 7, 2020

Choose a reason for hiding this comment

rohan-varma Oct 7, 2020

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2020 • edited

Codecov Report

dr-ci bot commented Oct 8, 2020 • edited

💊 CI failures summary and remediations

❄️ 1 failure tentatively classified as flaky

pytorch_macos_10_13_py3_test (1/1)

facebook-github-bot commented Oct 9, 2020

facebook-github-bot commented Oct 9, 2020

rohan-varma commented Oct 6, 2020 •

edited

codecov bot commented Oct 7, 2020 •

edited

dr-ci bot commented Oct 8, 2020 •

edited