[Metrics] Detach bugfix #4313

SkafteNicki · 2020-10-22T20:25:33Z

What does this PR do?

Fixes #4098
Fixes #4266
Make sure that the metrics states gets detached from the computational graph to prevent memory consumption from growing.
Also make sure that results gets detached for logging.
Also added note on how metrics can be backproped.

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 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 🙃

teddykoker

Good catch nice work :)

nathanpainchaud

Just a few typos in the added documentation.

docs/source/metrics.rst

pytorch_lightning/metrics/metric.py

docs/source/metrics.rst

Co-authored-by: Teddy Koker <[email protected]>

…i/pytorch-lightning into metrics/detach_bugfix

codecov · 2020-10-23T11:56:19Z

Codecov Report

Merging #4313 into master will increase coverage by 0%.
The diff coverage is 80%.

@@          Coverage Diff           @@
##           master   #4313   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         113     113           
  Lines        8222    8198   -24     
======================================
- Hits         7637    7622   -15     
+ Misses        585     576    -9

nateraw

this is a huge catch, thank you @SkafteNicki ...I was goin OOM on GPU when training on large dataset last night, and this is definitely the culprit.

mergify · 2020-10-24T20:57:01Z

This pull request is now in conflict... :(

docs/source/metrics.rst

Co-authored-by: Adrian Wälchli <[email protected]>

SeanNaren · 2020-11-02T17:18:48Z

Looks good! I was contemplating if the detach() should move within the compute() function, but then realised there maybe be a case where you wouldn't want to detach computed metrics.

* detach on buffer * doc update * remove file * changelog * suggestions * Update docs/source/metrics.rst Co-authored-by: Teddy Koker <[email protected]> * fix for 4266 * Update docs/source/metrics.rst Co-authored-by: Adrian Wälchli <[email protected]> * Update CHANGELOG.md Co-authored-by: Teddy Koker <[email protected]> Co-authored-by: chaton <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Ananya Harsh Jha <[email protected]> Co-authored-by: Roger Shieh <[email protected]> Co-authored-by: Sean Naren <[email protected]> Co-authored-by: Rohit Gupta <[email protected]> (cherry picked from commit 19187d3)

* detach on buffer * doc update * remove file * changelog * suggestions * Update docs/source/metrics.rst Co-authored-by: Teddy Koker <[email protected]> * fix for 4266 * Update docs/source/metrics.rst Co-authored-by: Adrian Wälchli <[email protected]> * Update CHANGELOG.md Co-authored-by: Teddy Koker <[email protected]> Co-authored-by: chaton <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Ananya Harsh Jha <[email protected]> Co-authored-by: Roger Shieh <[email protected]> Co-authored-by: Sean Naren <[email protected]> Co-authored-by: Rohit Gupta <[email protected]>

SkafteNicki added 3 commits October 22, 2020 22:14

detach on buffer

c767560

doc update

158dc39

remove file

c174170

SkafteNicki added bug Something isn't working Metrics labels Oct 22, 2020

SkafteNicki added this to the 1.0.x milestone Oct 22, 2020

SkafteNicki requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, tchaton, teddykoker and williamFalcon as code owners October 22, 2020 20:25

mergify bot requested a review from a team October 22, 2020 20:26

teddykoker approved these changes Oct 22, 2020

View reviewed changes

nathanpainchaud reviewed Oct 22, 2020

View reviewed changes

docs/source/metrics.rst Outdated Show resolved Hide resolved

docs/source/metrics.rst Outdated Show resolved Hide resolved

SkafteNicki added 2 commits October 22, 2020 22:35

changelog

81c1a26

suggestions

f313bd2

ananyahjha93 suggested changes Oct 22, 2020

View reviewed changes

pytorch_lightning/metrics/metric.py Show resolved Hide resolved

mergify bot requested a review from a team October 22, 2020 21:14

teddykoker reviewed Oct 22, 2020

View reviewed changes

docs/source/metrics.rst Show resolved Hide resolved

SkafteNicki and others added 3 commits October 23, 2020 11:59

Update docs/source/metrics.rst

fea7a12

Co-authored-by: Teddy Koker <[email protected]>

fix for 4266

b153967

Merge branch 'metrics/detach_bugfix' of https://github.com/SkafteNick…

08c966c

…i/pytorch-lightning into metrics/detach_bugfix

SkafteNicki mentioned this pull request Oct 23, 2020

Error in logging of Regression Metrics #4266

Closed

Merge branch 'master' into metrics/detach_bugfix

9f11214

nateraw reviewed Oct 23, 2020

View reviewed changes

awaelchli approved these changes Oct 30, 2020

View reviewed changes

docs/source/metrics.rst Outdated Show resolved Hide resolved

Update docs/source/metrics.rst

74470df

Co-authored-by: Adrian Wälchli <[email protected]>

justusschock approved these changes Oct 30, 2020

View reviewed changes

Merge branch 'master' into metrics/detach_bugfix

1296b66

SkafteNicki requested a review from ananyahjha93 October 30, 2020 10:54

Merge branch 'master' into metrics/detach_bugfix

46e3377

ananyahjha93 approved these changes Nov 2, 2020

View reviewed changes

ananyahjha93 and others added 2 commits November 2, 2020 09:33

Merge branch 'master' into metrics/detach_bugfix

92170d7

Update CHANGELOG.md

c88b511

s-rog approved these changes Nov 2, 2020

View reviewed changes

s-rog and others added 2 commits November 3, 2020 00:17

Merge branch 'master' into metrics/detach_bugfix

c62ccb8

Merge branch 'master' into metrics/detach_bugfix

3bf5cb9

Merge branch 'master' into metrics/detach_bugfix

beaec83

SeanNaren merged commit 19187d3 into Lightning-AI:master Nov 2, 2020

SkafteNicki deleted the metrics/detach_bugfix branch November 2, 2020 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics] Detach bugfix #4313

[Metrics] Detach bugfix #4313

SkafteNicki commented Oct 22, 2020 •

edited

Loading

teddykoker left a comment

nathanpainchaud left a comment

codecov bot commented Oct 23, 2020 •

edited

Loading

nateraw left a comment

mergify bot commented Oct 24, 2020

SeanNaren commented Nov 2, 2020

[Metrics] Detach bugfix #4313

[Metrics] Detach bugfix #4313

Conversation

SkafteNicki commented Oct 22, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

teddykoker left a comment

Choose a reason for hiding this comment

nathanpainchaud left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2020 • edited Loading

Codecov Report

nateraw left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 24, 2020

SeanNaren commented Nov 2, 2020

SkafteNicki commented Oct 22, 2020 •

edited

Loading

codecov bot commented Oct 23, 2020 •

edited

Loading