[bugfix] Changed CometLogger to stop modifying metrics in place

[sohamtiwari3120]

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 🙃

[awaelchli]

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

[codecov]

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     

[sohamtiwari3120]

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]

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]

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

[sohamtiwari3120]

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

Sincerely,
Soham

[awaelchli]

looks pretty good! fingers crossed it passes :)

[sohamtiwari3120]

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

[sohamtiwari3120]

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