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

[Metrics] Detach bugfix #4313

Merged
merged 17 commits into from
Nov 2, 2020
Merged

[Metrics] Detach bugfix #4313

merged 17 commits into from
Nov 2, 2020

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Oct 22, 2020 •
edited
Loading

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 🙃

@SkafteNicki SkafteNicki added bug Something isn't working Metrics labels Oct 22, 2020
@SkafteNicki SkafteNicki added this to the 1.0.x milestone Oct 22, 2020
@mergify mergify bot requested a review from a team October 22, 2020 20:26
teddykoker
teddykoker approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@teddykoker teddykoker left a comment

Choose a reason for hiding this comment

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

Good catch nice work :)

nathanpainchaud
nathanpainchaud reviewed Oct 22, 2020
View reviewed changes
Copy link
Contributor

@nathanpainchaud nathanpainchaud left a comment

Choose a reason for hiding this comment

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

Just a few typos in the added documentation.

docs/source/metrics.rst Outdated Show resolved Hide resolved
docs/source/metrics.rst Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team October 22, 2020 21:14
@SkafteNicki SkafteNicki mentioned this pull request Oct 23, 2020
Closed
@codecov
Copy link

codecov bot commented Oct 23, 2020 •
edited
Loading

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
nateraw reviewed Oct 23, 2020
View reviewed changes
Copy link
Contributor

@nateraw nateraw left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mergify bot commented Oct 24, 2020

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

awaelchli
awaelchli approved these changes Oct 30, 2020
View reviewed changes
docs/source/metrics.rst Outdated Show resolved Hide resolved
@tchaton @awaelchli
Update docs/source/metrics.rst
74470df 
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
@SeanNaren
Copy link
Contributor

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.

@SeanNaren SeanNaren merged commit 19187d3 into Lightning-AI:master Nov 2, 2020
@SkafteNicki SkafteNicki deleted the metrics/detach_bugfix branch November 2, 2020 21:13
Borda pushed a commit that referenced this pull request Nov 4, 2020
@SkafteNicki @Borda
[Metrics] Detach bugfix (#4313)
e95a1b2 
* detach on buffer

* doc update

* remove file

* changelog

* suggestions

* Update docs/source/metrics.rst

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>

* fix for 4266

* Update docs/source/metrics.rst

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

* Update CHANGELOG.md

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Co-authored-by: Ananya Harsh Jha <ananya@pytorchlightning.ai>
Co-authored-by: Roger Shieh <sh.rog@protonmail.ch>
Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
(cherry picked from commit 19187d3)
rohitgr7 added a commit that referenced this pull request Nov 21, 2020
@SkafteNicki @teddykoker
@tchaton @awaelchli @ananyahjha93 @s-rog @SeanNaren @rohitgr7
[Metrics] Detach bugfix (#4313)
f2a5747 
* detach on buffer

* doc update

* remove file

* changelog

* suggestions

* Update docs/source/metrics.rst

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>

* fix for 4266

* Update docs/source/metrics.rst

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

* Update CHANGELOG.md

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Co-authored-by: Ananya Harsh Jha <ananya@pytorchlightning.ai>
Co-authored-by: Roger Shieh <sh.rog@protonmail.ch>
Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nateraw nateraw nateraw left review comments

@nathanpainchaud nathanpainchaud nathanpainchaud left review comments

@teddykoker teddykoker teddykoker approved these changes

@s-rog s-rog s-rog approved these changes

@justusschock justusschock justusschock approved these changes

@ananyahjha93 ananyahjha93 ananyahjha93 approved these changes

@awaelchli awaelchli awaelchli approved these changes

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

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

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

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
pl:1.0.x
Development

Successfully merging this pull request may close these issues.

Error in logging of Regression Metrics Memory leak when using Metric with list state