Make `all_reduce` consistent for both NCCL and GLOO #18235

awaelchli · 2023-08-05T17:23:56Z

What does this PR do?

Fixes #18228
specifically the last part #18228 (comment).

Using the "avg" reduce op would not be carried out in-place, while all other operations did (e.g. sum).
This PR makes this consistent, enabling the inplace behavior for all operations.

Before submitting

Was this discussed/agreed via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Reviewer checklist

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

cc @carmocca @justusschock @awaelchli

for more information, see https://pre-commit.ci

tests/tests_pytorch/core/test_metric_result_integration.py

github-actions · 2023-08-05T21:54:14Z

⚡ Required checks status: All passing 🟢

Groups summary

🟢 pytorch_lightning: Tests workflow

Check ID	Status
pl-cpu (macOS-11, lightning, 3.8, 1.11)	success	✅
pl-cpu (macOS-11, lightning, 3.9, 1.12)	success	✅
pl-cpu (macOS-11, lightning, 3.10, 1.13)	success	✅
pl-cpu (macOS-11, lightning, 3.10, 2.0)	success	✅
pl-cpu (macOS-11, lightning, 3.8, 1.11, oldest)	success	✅
pl-cpu (ubuntu-20.04, lightning, 3.8, 1.11)	success	✅
pl-cpu (ubuntu-20.04, lightning, 3.9, 1.12)	success	✅
pl-cpu (ubuntu-20.04, lightning, 3.10, 1.13)	success	✅
pl-cpu (ubuntu-20.04, lightning, 3.10, 2.0)	success	✅
pl-cpu (ubuntu-20.04, lightning, 3.8, 1.11, oldest)	success	✅
pl-cpu (windows-2022, lightning, 3.8, 1.11)	success	✅
pl-cpu (windows-2022, lightning, 3.9, 1.12)	success	✅
pl-cpu (windows-2022, lightning, 3.10, 1.13)	success	✅
pl-cpu (windows-2022, lightning, 3.10, 2.0)	success	✅
pl-cpu (windows-2022, lightning, 3.8, 1.11, oldest)	success	✅
pl-cpu (macOS-11, pytorch, 3.8, 1.13)	success	✅
pl-cpu (ubuntu-20.04, pytorch, 3.8, 1.13)	success	✅
pl-cpu (windows-2022, pytorch, 3.8, 1.13)	success	✅

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py, src/lightning/pytorch/trainer/connectors/logger_connector/result.py, tests/tests_pytorch/core/test_metric_result_integration.py.

🟢 pytorch_lightning: Azure GPU

Check ID	Status
[pytorch-lightning (GPUs) (testing Lightning	latest)](https://dev.azure.com/Lightning-AI/72ab7ed8-b00f-4b6e-b131-3388f7ffafa7/_build/results?buildId=169272&view=logs&jobId=47e66f3c-897a-5428-da11-bf5c7745762e)	success
[pytorch-lightning (GPUs) (testing PyTorch	latest)](https://dev.azure.com/Lightning-AI/72ab7ed8-b00f-4b6e-b131-3388f7ffafa7/_build/results?buildId=169272&view=logs&jobId=3f274fac-2e11-54ca-487e-194c91f3ae9f)	success

These checks are required after the changes to src/lightning/pytorch/trainer/connectors/logger_connector/result.py, tests/tests_pytorch/core/test_metric_result_integration.py, src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py.

🟢 pytorch_lightning: Benchmarks

Check ID	Status
lightning.Benchmarks	success	✅

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py, src/lightning/pytorch/trainer/connectors/logger_connector/result.py.

🟢 fabric: Docs

Check ID	Status
make-doctest (fabric)	success	✅
make-html (fabric)	success	✅

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py.

🟢 pytorch_lightning: Docs

Check ID	Status
make-doctest (pytorch)	success	✅
make-html (pytorch)	success	✅

These checks are required after the changes to src/lightning/pytorch/trainer/connectors/logger_connector/result.py.

🟢 lightning_fabric: CPU workflow

Check ID	Status
fabric-cpu (macOS-11, lightning, 3.8, 1.11)	success	✅
fabric-cpu (macOS-11, lightning, 3.9, 1.12)	success	✅
fabric-cpu (macOS-11, lightning, 3.10, 1.13)	success	✅
fabric-cpu (macOS-11, lightning, 3.10, 2.0)	success	✅
fabric-cpu (macOS-11, lightning, 3.8, 1.11, oldest)	success	✅
fabric-cpu (ubuntu-20.04, lightning, 3.8, 1.11)	success	✅
fabric-cpu (ubuntu-20.04, lightning, 3.9, 1.12)	success	✅
fabric-cpu (ubuntu-20.04, lightning, 3.10, 1.13)	success	✅
fabric-cpu (ubuntu-20.04, lightning, 3.10, 2.0)	success	✅
fabric-cpu (ubuntu-20.04, lightning, 3.8, 1.11, oldest)	success	✅
fabric-cpu (windows-2022, lightning, 3.8, 1.11)	success	✅
fabric-cpu (windows-2022, lightning, 3.9, 1.12)	success	✅
fabric-cpu (windows-2022, lightning, 3.10, 1.13)	success	✅
fabric-cpu (windows-2022, lightning, 3.10, 2.0)	success	✅
fabric-cpu (windows-2022, lightning, 3.8, 1.11, oldest)	success	✅
fabric-cpu (macOS-11, fabric, 3.8, 1.13)	success	✅
fabric-cpu (ubuntu-20.04, fabric, 3.8, 1.13)	success	✅
fabric-cpu (windows-2022, fabric, 3.8, 1.13)	success	✅

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py, tests/tests_fabric/utilities/test_distributed.py.

🟢 lightning_fabric: Azure GPU

Check ID	Status
[lightning-fabric (GPUs) (testing Fabric	latest)](https://dev.azure.com/Lightning-AI/72ab7ed8-b00f-4b6e-b131-3388f7ffafa7/_build/results?buildId=169274&view=logs&jobId=3f274fac-2e11-54ca-487e-194c91f3ae9f)	success
[lightning-fabric (GPUs) (testing Lightning	latest)](https://dev.azure.com/Lightning-AI/72ab7ed8-b00f-4b6e-b131-3388f7ffafa7/_build/results?buildId=169274&view=logs&jobId=47e66f3c-897a-5428-da11-bf5c7745762e)	success

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py, tests/tests_fabric/utilities/test_distributed.py.

🟢 mypy

Check ID	Status
mypy	success	✅

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py, src/lightning/pytorch/trainer/connectors/logger_connector/result.py.

🟢 install

Check ID	Status
install-pkg (ubuntu-22.04, app, 3.8)	success	✅
install-pkg (ubuntu-22.04, app, 3.10)	success	✅
install-pkg (ubuntu-22.04, fabric, 3.8)	success	✅
install-pkg (ubuntu-22.04, fabric, 3.10)	success	✅
install-pkg (ubuntu-22.04, pytorch, 3.8)	success	✅
install-pkg (ubuntu-22.04, pytorch, 3.10)	success	✅
install-pkg (ubuntu-22.04, lightning, 3.8)	success	✅
install-pkg (ubuntu-22.04, lightning, 3.10)	success	✅
install-pkg (ubuntu-22.04, notset, 3.8)	success	✅
install-pkg (ubuntu-22.04, notset, 3.10)	success	✅
install-pkg (macOS-12, app, 3.8)	success	✅
install-pkg (macOS-12, app, 3.10)	success	✅
install-pkg (macOS-12, fabric, 3.8)	success	✅
install-pkg (macOS-12, fabric, 3.10)	success	✅
install-pkg (macOS-12, pytorch, 3.8)	success	✅
install-pkg (macOS-12, pytorch, 3.10)	success	✅
install-pkg (macOS-12, lightning, 3.8)	success	✅
install-pkg (macOS-12, lightning, 3.10)	success	✅
install-pkg (macOS-12, notset, 3.8)	success	✅
install-pkg (macOS-12, notset, 3.10)	success	✅
install-pkg (windows-2022, app, 3.8)	success	✅
install-pkg (windows-2022, app, 3.10)	success	✅
install-pkg (windows-2022, fabric, 3.8)	success	✅
install-pkg (windows-2022, fabric, 3.10)	success	✅
install-pkg (windows-2022, pytorch, 3.8)	success	✅
install-pkg (windows-2022, pytorch, 3.10)	success	✅
install-pkg (windows-2022, lightning, 3.8)	success	✅
install-pkg (windows-2022, lightning, 3.10)	success	✅
install-pkg (windows-2022, notset, 3.8)	success	✅
install-pkg (windows-2022, notset, 3.10)	success	✅

These checks are required after the changes to src/lightning/fabric/fabric.py, src/lightning/fabric/utilities/distributed.py, src/lightning/pytorch/trainer/connectors/logger_connector/result.py.

🟢 link-check

Check ID	Status
check-md-links / markdown-link-check	success	✅

These checks are required after the changes to src/lightning/fabric/CHANGELOG.md.

Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

tests/tests_fabric/utilities/test_distributed.py

codecov · 2023-08-08T07:48:03Z

Codecov Report

Merging #18235 (e2dad6e) into master (7e13eb7) will decrease coverage by 24%.
Report is 3 commits behind head on master.
The diff coverage is 70%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #18235     +/-   ##
=========================================
- Coverage      83%      59%    -24%     
=========================================
  Files         431      426      -5     
  Lines       33320    33234     -86     
=========================================
- Hits        27821    19715   -8106     
- Misses       5499    13519   +8020

src/lightning/fabric/utilities/distributed.py

carmocca

LGTM

src/lightning/fabric/utilities/distributed.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> (cherry picked from commit 70e31b6)

Standardize reduce()

29883c3

awaelchli added bug Something isn't working strategy: ddp DistributedDataParallel labels Aug 5, 2023

awaelchli added this to the 2.0.x milestone Aug 5, 2023

awaelchli added the fun Staff contributions outside working hours - to differentiate from the "community" label label Aug 5, 2023

github-actions bot added the fabric lightning.fabric.Fabric label Aug 5, 2023

awaelchli and others added 12 commits August 5, 2023 20:01

test

1413bf3

[pre-commit.ci] auto fixes from pre-commit.com hooks

10db1a2

for more information, see https://pre-commit.ci

changelog

64e086f

Merge remote-tracking branch 'origin/bugfix/reduce' into bugfix/reduce

ef3ef53

update test

5d2f3b3

fix

3127bbe

[pre-commit.ci] auto fixes from pre-commit.com hooks

0f7095e

for more information, see https://pre-commit.ci

fix

0b73f7c

tests

a4e4262

tests

84c7c10

Merge remote-tracking branch 'origin/bugfix/reduce' into bugfix/reduce

87bee97

fix test

85f5401

github-actions bot added the pl Generic label for PyTorch Lightning package label Aug 5, 2023

pre-commit-ci bot and others added 5 commits August 5, 2023 21:10

[pre-commit.ci] auto fixes from pre-commit.com hooks

9eeb434

for more information, see https://pre-commit.ci

fix

73c037a

Merge remote-tracking branch 'origin/bugfix/reduce' into bugfix/reduce

6456281

documentation

2beabef

update

2c60b9f

awaelchli commented Aug 5, 2023

View reviewed changes

tests/tests_pytorch/core/test_metric_result_integration.py Show resolved Hide resolved

awaelchli marked this pull request as ready for review August 5, 2023 21:53

awaelchli requested review from carmocca, justusschock, Borda and williamFalcon as code owners August 5, 2023 21:53

awaelchli commented Aug 5, 2023

View reviewed changes

tests/tests_fabric/utilities/test_distributed.py Show resolved Hide resolved

Borda approved these changes Aug 6, 2023

View reviewed changes

awaelchli and others added 2 commits August 7, 2023 20:44

fix

472588a

Merge branch 'master' into bugfix/reduce

e2dad6e

Merge branch 'master' into bugfix/reduce

f772630

mergify bot added the has conflicts label Aug 9, 2023

carmocca reviewed Aug 9, 2023

View reviewed changes

src/lightning/fabric/utilities/distributed.py Show resolved Hide resolved

carmocca approved these changes Aug 9, 2023

View reviewed changes

src/lightning/fabric/utilities/distributed.py Show resolved Hide resolved

awaelchli and others added 2 commits August 9, 2023 14:17

Update src/lightning/fabric/utilities/distributed.py

b91cbce

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Merge branch 'master' into bugfix/reduce

397be6a

mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Aug 9, 2023

line too long

24771f8

awaelchli merged commit 70e31b6 into master Aug 9, 2023
102 checks passed

awaelchli deleted the bugfix/reduce branch August 9, 2023 21:39

Borda pushed a commit that referenced this pull request Aug 14, 2023

Make all_reduce consistent for both NCCL and GLOO (#18235)

34d9bc9

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> (cherry picked from commit 70e31b6)

lexierule pushed a commit that referenced this pull request Aug 14, 2023

Make all_reduce consistent for both NCCL and GLOO (#18235)

cfefd09

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> (cherry picked from commit 70e31b6)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `all_reduce` consistent for both NCCL and GLOO #18235

Make `all_reduce` consistent for both NCCL and GLOO #18235

awaelchli commented Aug 5, 2023 •

edited by github-actions bot

github-actions bot commented Aug 5, 2023 •

edited

codecov bot commented Aug 8, 2023 •

edited

carmocca left a comment

Make all_reduce consistent for both NCCL and GLOO #18235

Make all_reduce consistent for both NCCL and GLOO #18235

Conversation

awaelchli commented Aug 5, 2023 • edited by github-actions bot

What does this PR do?

PR review

github-actions bot commented Aug 5, 2023 • edited

⚡ Required checks status: All passing 🟢

Groups summary

codecov bot commented Aug 8, 2023 • edited

Codecov Report

carmocca left a comment

Choose a reason for hiding this comment

Make `all_reduce` consistent for both NCCL and GLOO #18235

Make `all_reduce` consistent for both NCCL and GLOO #18235

awaelchli commented Aug 5, 2023 •

edited by github-actions bot

github-actions bot commented Aug 5, 2023 •

edited

codecov bot commented Aug 8, 2023 •

edited