[bugfix] Changed CometLogger to stop modifying metrics in place by sohamtiwari3120 · Pull Request #9150 · Lightning-AI/pytorch-lightning

sohamtiwari3120 · 2021-08-26T19:23:25Z

What does this PR do?

modified log_metrics() in comet.py to make a copy of metrics dictionary before calling val.cpu().detach(), hence preventing metrics from being changed in place.

Fixes #7021

Does your PR introduce any breaking changes? If yes, please list them.

None

Before submitting

Was this discussed/approved 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 internal minor changes/refactorings)

PR review

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

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

Did you have fun?

Make sure you had fun coding 🙃

…ore calling val.cpu().detach()

awaelchli

@sohamtiwari3120 looking pretty good!
would you mind adding a changelog entry too in the file CHANGELOG.md?

codecov · 2021-08-28T22:17:20Z

Codecov Report

Merging #9150 (ea9fad7) into master (46b00a7) will decrease coverage by 4%.
The diff coverage is 100%.

❗ Current head ea9fad7 differs from pull request most recent head 37c429e. Consider uploading reports for the commit 37c429e to get more accurate results

@@           Coverage Diff           @@
##           master   #9150    +/-   ##
=======================================
- Coverage      92%     88%    -4%     
=======================================
  Files         176     176            
  Lines       14870   14825    -45     
=======================================
- Hits        13719   13059   -660     
- Misses       1151    1766   +615

…_place

sohamtiwari3120 · 2021-08-29T03:46:39Z

Hi @awaelchli!
I noticed that there were new commits on the master branch, hence I first merged those changes into my branch (where I wrote the bugfix), and then added a line about the bugfix in CHANGELOG.md, is that fine?

tchaton

LGTM !

akihironitta

LGTM once the test is added!

sohamtiwari3120 · 2021-08-30T03:06:05Z

LGTM once the test is added!

Hi @akihironitta
This is my first attempt at open source contribution. Can you please explain what exactly will the "test" test?

I modified the log_metrics function. So should I test whether the metrics dictionary is changing or not? Or you're talking about some other test.

Thanks!
Sincerely,
Soham

awaelchli · 2021-08-30T08:17:42Z

So you want to test that metrics are no longer changed inplace when we call .cpu(). Then I suggest a test like this:

metrics=dict(mymetric=Mock())
logger.log_metrics(metrics)
metrics["mymetric"].cpu.assert_not_called()

This would go into the file tests/loggers/test_comet.py

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

…tps://github.com/sohamtiwari3120/pytorch-lightning into bug/7021_comet_logger_modifying_metrics_in_place

sohamtiwari3120 · 2021-08-30T11:46:49Z

Hi @awaelchli @tchaton
Can you please review the test I have added in the test_comet.py file? Thank you

Sincerely,
Soham

awaelchli · 2021-08-30T11:55:49Z

looks pretty good! fingers crossed it passes :)

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

sohamtiwari3120 · 2021-08-30T16:45:44Z

@awaelchli it seems that the test I added worked finally! 😄

removed changes for tests from changelog, that should not be mentioned Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

sohamtiwari3120 · 2021-08-31T03:49:42Z

Hi! Do I need to make any further changes to the PR for it to auto-merge? Or should I leave it as it is now?

Thanks

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

modified log_metrics() in comet.py to make a copy of metrics dict bef…

bb7303e

…ore calling val.cpu().detach()

sohamtiwari3120 marked this pull request as ready for review August 28, 2021 08:56

sohamtiwari3120 requested review from Borda, awaelchli and tchaton as code owners August 28, 2021 08:56

awaelchli added bug Something isn't working logger Related to the Loggers labels Aug 28, 2021

awaelchli modified the milestones: v1.5, v1.4.x Aug 28, 2021

awaelchli approved these changes Aug 28, 2021

View reviewed changes

sohamtiwari3120 added 2 commits August 29, 2021 02:23

Merge branch 'master' into bug/7021_comet_logger_modifying_metrics_in…

9d2b3c0

…_place

modified changelog as well after merging commits from master

8d8420f

sohamtiwari3120 requested review from SeanNaren, carmocca, justusschock, kaushikb11 and williamFalcon as code owners August 29, 2021 03:43

tchaton approved these changes Aug 29, 2021

View reviewed changes

mergify Bot added the ready to be merged PRs ready to be merged label Aug 29, 2021

akihironitta reviewed Aug 30, 2021

View reviewed changes

tchaton enabled auto-merge (squash) August 30, 2021 09:24

added tests for Lightning-AI#9150 and mentioned the same in changelog

681eb41

auto-merge was automatically disabled August 30, 2021 11:42
Head branch was pushed to by a user without write access

pre-commit-ci Bot and others added 2 commits August 30, 2021 11:43

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

c46d3cb

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

added new line at the end of test_comet.py

dadfe39

Merge branch 'bug/7021_comet_logger_modifying_metrics_in_place' of ht…

18b5b49

…tps://github.com/sohamtiwari3120/pytorch-lightning into bug/7021_comet_logger_modifying_metrics_in_place

carmocca reviewed Aug 30, 2021

View reviewed changes

Comment thread pytorch_lightning/loggers/comet.py

sohamtiwari3120 and others added 2 commits August 30, 2021 16:23

fixed test_comet_metrics_safe, added import for cometexperiment

be96bec

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

ea9fad7

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

awaelchli reviewed Aug 30, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

Update CHANGELOG.md

37c429e

removed changes for tests from changelog, that should not be mentioned Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

carmocca approved these changes Aug 30, 2021

View reviewed changes

carmocca reviewed Aug 30, 2021

View reviewed changes

Comment thread tests/loggers/test_comet.py Outdated

Update tests/loggers/test_comet.py

420d37c

carmocca enabled auto-merge (squash) August 30, 2021 22:27

carmocca merged commit 861f8af into Lightning-AI:master Aug 31, 2021

ethanwharris pushed a commit that referenced this pull request Aug 31, 2021

[bugfix] Changed CometLogger to stop modifying metrics in place (#9150)

3552a27

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

lexierule pushed a commit that referenced this pull request Sep 1, 2021

[bugfix] Changed CometLogger to stop modifying metrics in place (#9150)

4b007e6

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Changed CometLogger to stop modifying metrics in place#9150

[bugfix] Changed CometLogger to stop modifying metrics in place#9150
carmocca merged 11 commits intoLightning-AI:masterfrom
sohamtiwari3120:bug/7021_comet_logger_modifying_metrics_in_place

sohamtiwari3120 commented Aug 26, 2021 •

edited by awaelchli

Loading

Uh oh!

awaelchli left a comment

Uh oh!

codecov Bot commented Aug 28, 2021 •

edited

Loading

Uh oh!

sohamtiwari3120 commented Aug 29, 2021

Uh oh!

tchaton left a comment

Uh oh!

akihironitta left a comment

Uh oh!

sohamtiwari3120 commented Aug 30, 2021

Uh oh!

awaelchli commented Aug 30, 2021

Uh oh!

sohamtiwari3120 commented Aug 30, 2021

Uh oh!

awaelchli commented Aug 30, 2021

Uh oh!

Uh oh!

sohamtiwari3120 commented Aug 30, 2021

Uh oh!

Uh oh!

Uh oh!

sohamtiwari3120 commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

sohamtiwari3120 commented Aug 26, 2021 • edited by awaelchli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Uh oh!

awaelchli left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sohamtiwari3120 commented Aug 29, 2021

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

akihironitta left a comment

Choose a reason for hiding this comment

Uh oh!

sohamtiwari3120 commented Aug 30, 2021

Uh oh!

awaelchli commented Aug 30, 2021

Uh oh!

sohamtiwari3120 commented Aug 30, 2021

Uh oh!

awaelchli commented Aug 30, 2021

Uh oh!

Uh oh!

sohamtiwari3120 commented Aug 30, 2021

Uh oh!

Uh oh!

Uh oh!

sohamtiwari3120 commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sohamtiwari3120 commented Aug 26, 2021 •

edited by awaelchli

Loading

codecov Bot commented Aug 28, 2021 •

edited

Loading