Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix gather_all_tensors #220

Merged
merged 16 commits into from
May 4, 2021
Merged

Fix gather_all_tensors #220

merged 16 commits into from
May 4, 2021

Conversation

maximsch2
Copy link
Contributor

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

gather_all_tensors doesn't work correctly when tensors are different shape, fix that

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Apr 30, 2021 •
edited
Loading

Hello @maximsch2! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-04 13:00:54 UTC

@codecov
Copy link

codecov bot commented Apr 30, 2021 •
edited
Loading

Codecov Report

Merging #220 (0822aef) into master (67ce961) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   96.76%   96.78%   +0.02%     
==========================================
  Files          92       92              
  Lines        2965     2988      +23     
==========================================
+ Hits         2869     2892      +23     
  Misses         96       96
Flag Coverage Δ
Linux 79.11% <14.81%> (-0.55%) ⬇️
Windows 79.11% <14.81%> (-0.55%) ⬇️
cpu 96.78% <100.00%> (+0.02%) ⬆️
macOS 96.78% <100.00%> (+0.02%) ⬆️
pytest 96.78% <100.00%> (+0.02%) ⬆️
python3.6 95.72% <100.00%> (+0.03%) ⬆️
python3.8 96.65% <100.00%> (-0.08%) ⬇️
python3.9 ?
torch1.3.1 95.72% <100.00%> (+0.03%) ⬆️
torch1.4.0 ?
torch1.8.1 96.65% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchmetrics/utilities/distributed.py 98.27% <100.00%> (+1.13%) ⬆️

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 67ce961...0822aef. Read the comment docs.

@maximsch2
Copy link
Contributor Author

@ananthsub, I think it can be great to bring this over to LightningModule.all_gather as well to make it easier to use.

@maximsch2 maximsch2 mentioned this pull request May 3, 2021
Closed
@Borda Borda added the bug / fix Something isn't working label May 3, 2021
Borda
Borda approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

SkafteNicki
SkafteNicki reviewed May 3, 2021
View reviewed changes
tests/bases/test_ddp.py Outdated Show resolved Hide resolved
tests/bases/test_ddp.py Outdated Show resolved Hide resolved
torchmetrics/utilities/distributed.py Outdated Show resolved Hide resolved
torchmetrics/utilities/distributed.py Outdated Show resolved Hide resolved
torchmetrics/utilities/distributed.py Outdated Show resolved Hide resolved
@Borda Borda requested a review from SkafteNicki May 3, 2021 20:22
tchaton
tchaton approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@Borda Borda enabled auto-merge (squash) May 4, 2021 07:58
@ananthsub
Copy link
Contributor

@tchaton : @maximsch2 pointed out we should bring this to Lightning to replace the existing all_gather_tensors utility here: https://github.com/PyTorchLightning/pytorch-lightning/blob/597b309f2e3b0f5fb618f4326e8700df399917fb/pytorch_lightning/utilities/distributed.py#L86-L113

SkafteNicki
SkafteNicki approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

torchmetrics/utilities/distributed.py Outdated Show resolved Hide resolved
@Borda Borda merged commit cb6899b into Lightning-AI:master May 4, 2021
@maximsch2 maximsch2 deleted the fix_gather_tensors branch May 4, 2021 15:19
Borda pushed a commit that referenced this pull request May 10, 2021
@maximsch2 @Borda
Fix gather_all_tensors (#220)
09874d1 
* fix gather_all_tensors

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

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

* pep

* shortcut for simple cases

* add better test and fix gather

* format

* add changelog

* address comments

* pep8

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: jirka <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit cb6899b)
@ananthsub ananthsub mentioned this pull request Apr 6, 2022
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@tchaton tchaton tchaton approved these changes

@Borda Borda Borda approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

Assignees
No one assigned
Labels
bug / fix Something isn't working ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maximsch2 @pep8speaks @ananthsub @Borda @tchaton @SkafteNicki