[FIX] Average Pbar Metrics #4534

tchaton · 2020-11-05T15:48:25Z

What does this PR do?

PB: loss from progress bar appears to be sum of loss across all GPUs in Lightning 1.0.3

The sync_dist being an in-place operation, it was reducing across gpu and then it was changing behaviour.

Solution: Clone the value if sync_dist.

TODO: Find a way to make the sync_dist only once.

Closes #4395

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
[x ] 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? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Is this pull request ready for review? (if not, please submit in draft mode)

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 · 2020-11-05T15:48:28Z

Hello @tchaton! Thanks for updating this PR.

In the file tests/trainer/legacy_deprecate_flow_log_tests/test_trainer_steps_scalar_return.py:

Line 206:45: E231 missing whitespace after ':'

Comment last updated at 2020-11-12 14:03:57 UTC

tests/trainer/legacy_deprecate_flow_log_tests/test_trainer_steps_scalar_return.py

codecov · 2020-11-11T16:53:17Z

Codecov Report

Merging #4534 (7aa6a2e) into master (bd6c413) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4534   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         116     116           
  Lines        8879    8881    +2     
======================================
+ Hits         8261    8263    +2     
  Misses        618     618

….com/PyTorchLightning/pytorch-lightning into bug_fix/4395_sum_of_loss_across_gpus

pytorch_lightning/core/step_result.py

SeanNaren · 2020-11-12T12:45:10Z

Looks good! as mentioned I don't think its super priority to worry about a duplicate reduce call on a 1 item tensor, and for explicitness it's better to do it as a part of this func!

SkafteNicki

LGTM

pytorch_lightning/core/step_result.py

tests/trainer/legacy_deprecate_flow_log_tests/test_trainer_steps_scalar_return.py

* wip * update * normalize loss * update test * resolve bug * update test and add TODO * make sure it can be sync * add TODO * update sol (cherry picked from commit 4a01fd0)

* wip * update * normalize loss * update test * resolve bug * update test and add TODO * make sure it can be sync * add TODO * update sol

seannz · 2021-07-02T17:10:46Z

This fix seems to be broken in the latest master (commit 0e19d16) with the metrics summed across GPUs rather than averaged. Works correctly in 1.3.8.

tchaton added 2 commits November 5, 2020 11:01

wip

49e143d

update

8a43f35

tchaton added logger Related to the Loggers priority: 1 Medium priority task labels Nov 5, 2020

tchaton added this to the 1.0.x milestone Nov 5, 2020

tchaton self-assigned this Nov 5, 2020

tchaton added 2 commits November 9, 2020 15:04

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

9a29137

normalize loss

e566f81

s-rog reviewed Nov 10, 2020

View reviewed changes

tests/trainer/legacy_deprecate_flow_log_tests/test_trainer_steps_scalar_return.py Outdated Show resolved Hide resolved

edenlightning modified the milestones: 1.0.x, 1.0.7 Nov 10, 2020

tchaton added 2 commits November 11, 2020 14:36

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

712e018

update test

ddcbbc2

Borda modified the milestones: 1.0.7, 1.0.x Nov 11, 2020

tchaton added 2 commits November 11, 2020 16:21

resolve bug

1393a5e

update test and add TODO

8c66647

make sure it can be sync

b864a58

tchaton marked this pull request as ready for review November 11, 2020 17:15

tchaton requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, teddykoker and williamFalcon as code owners November 11, 2020 17:15

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

38062eb

tchaton added 2 commits November 11, 2020 17:36

add TODO

ca3709b

Merge branch 'bug_fix/4395_sum_of_loss_across_gpus' of https://github…

06b7e40

….com/PyTorchLightning/pytorch-lightning into bug_fix/4395_sum_of_loss_across_gpus

tchaton changed the title ~~[WIP] Average Pbar Metrics~~ [FIX] Average Pbar Metrics Nov 11, 2020

s-rog reviewed Nov 12, 2020

View reviewed changes

pytorch_lightning/core/step_result.py Outdated Show resolved Hide resolved

tchaton added 4 commits November 12, 2020 08:28

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

655f1ae

update sol

c6483f9

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

d273957

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

2277361

SeanNaren approved these changes Nov 12, 2020

View reviewed changes

SkafteNicki approved these changes Nov 12, 2020

View reviewed changes

Merge branch 'master' into bug_fix/4395_sum_of_loss_across_gpus

7aa6a2e

Borda reviewed Nov 12, 2020

View reviewed changes

s-rog approved these changes Nov 12, 2020

View reviewed changes

tchaton merged commit 4a01fd0 into master Nov 12, 2020

tchaton deleted the bug_fix/4395_sum_of_loss_across_gpus branch November 12, 2020 15:59

Borda pushed a commit that referenced this pull request Nov 12, 2020

[FIX] Average Pbar Metrics (#4534)

9408623

* wip * update * normalize loss * update test * resolve bug * update test and add TODO * make sure it can be sync * add TODO * update sol (cherry picked from commit 4a01fd0)

Borda pushed a commit that referenced this pull request Nov 12, 2020

[FIX] Average Pbar Metrics (#4534)

5eea67a

* wip * update * normalize loss * update test * resolve bug * update test and add TODO * make sure it can be sync * add TODO * update sol (cherry picked from commit 4a01fd0)

Borda pushed a commit that referenced this pull request Nov 12, 2020

[FIX] Average Pbar Metrics (#4534)

447fef2

* wip * update * normalize loss * update test * resolve bug * update test and add TODO * make sure it can be sync * add TODO * update sol (cherry picked from commit 4a01fd0)

rohitgr7 pushed a commit that referenced this pull request Nov 21, 2020

[FIX] Average Pbar Metrics (#4534)

e6857de

* wip * update * normalize loss * update test * resolve bug * update test and add TODO * make sure it can be sync * add TODO * update sol

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Average Pbar Metrics #4534

[FIX] Average Pbar Metrics #4534

tchaton commented Nov 5, 2020 •

edited

Loading

pep8speaks commented Nov 5, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

SeanNaren commented Nov 12, 2020

SkafteNicki left a comment

seannz commented Jul 2, 2021 •

edited

Loading

[FIX] Average Pbar Metrics #4534

[FIX] Average Pbar Metrics #4534

Conversation

tchaton commented Nov 5, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Nov 5, 2020 • edited Loading

Comment last updated at 2020-11-12 14:03:57 UTC

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

SeanNaren commented Nov 12, 2020

SkafteNicki left a comment

Choose a reason for hiding this comment

seannz commented Jul 2, 2021 • edited Loading

tchaton commented Nov 5, 2020 •

edited

Loading

pep8speaks commented Nov 5, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

seannz commented Jul 2, 2021 •

edited

Loading