Repository: Lightning-AI/pytorch-lightning
Pull request: [bugfix] Changed CometLogger to stop modifying metrics in place
## 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.
<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

The following links the related issue to the PR (https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
-->

Fixes #7021

### Does your PR introduce any breaking changes? If yes, please list them.

None

## Before submitting

- [x] Was this **discussed/approved** via a GitHub issue? (not for typos and docs)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [x] Did you make sure your **PR does only one thing**, instead of bundling different changes together?
- [x] Did you make sure to **update the documentation** with your changes? (if necessary)
- [x] Did you write any **new necessary tests**? (not for typos and docs)
- [x] Did you verify new and **existing tests pass** locally with your changes?
- [x] Did you list all the **breaking changes** introduced by this pull request?
- [x] Did you **update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)**? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- In the CHANGELOG, separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

- [x] Is this pull request ready for review? (if not, please submit in draft mode)
- [x] Check that all items from **Before submitting** are resolved
- [x] Make sure the title is self-explanatory and the description concisely explains the PR
- [x] 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 🙃


Problem statement:
CometLogger can modify logged metrics in-place 
When `logger.log_metrics(metrics)` is called with a `CometLogger`, `metrics` may be modified in-place. This can lead to confusing errors. E.g. if the user does

```python
def training_step(self, batch, batch_idx):
    losses = self._get_losses(batch)
    self.logger.log_metrics(losses)
    return losses
```

then `losses` will have all the tensors moved to the CPU and their gradients detached, leading to an error like `RuntimeError: element 0 of tensors does not require grad and does not have a grad_fn` when backprop is attempted.

None of the other loggers change `metrics` in-place when `log_metrics` is called. All of them except neptune say that they just accept `metrics: Dict[str, float]`, though some others (e.g. the tensorboard logger) have code to handle `torch.Tensor`s or other types as well.

The `CSVLogger` uses the following for handling tensors:
```python
def _handle_value(value):
    if isinstance(value, torch.Tensor):
        return value.item()
    return value
...
metrics = {k: _handle_value(v) for k, v in metrics_dict.items()}
```

The `TensorBoardLogger` similarly has

```python
for k, v in metrics.items():
    if isinstance(v, torch.Tensor):
        v = v.item()
    ...
    self.experiment.add_scalar(k, v, step)
```

In the `CometLogger`, the current tensor conversion code is

```python
for key, val in metrics.items():
  if is_tensor(val):
    metrics[key] = val.cpu().detach()
```

but then the entire `metrics` dictionary is copied later in the function anyway, so it doesn't really make sense to do in-place modification then copy everything.

I'm happy to submit a PR to fix this so that the `CometLogger` doesn't modify the original `metrics` dictionary. I just wanted to ask for a couple of opinions before changing things:

1. Should I keep the current tensor conversion behavior for `CometLogger` (`val.cpu().detach()`) or switch to using `val.item()`? My preference would be the latter, though this does change the behavior (see at the end).
2. Should I update the other loggers to all accept `metrics: Dict[str, Union[float, torch.Tensor]]` and have them all use the same method (probably imported from `loggers/base.py`) to convert to a `Dict[str, float]`?
3. * I don't know the other loggers, so I'm not sure if tensors are actually not supported or if the type annotation isn't precise and the conversion is happening in third-party code

---

`val.cpu().detach()` vs `val.item()`
* Comet sort of has support for tensors with >1 element, so using the first method will make logging such tensors valid while the second method would throw an error. However, I don't think anybody would be using this behavior on purpose. If you do `logger.log_metrics({"test": torch.tensor([1.0, 10.0])})`, you get `COMET WARNING: Cannot safely convert array([ 1., 10.], dtype=float32) object to a scalar value, using its string representation for logging`. The metric itself doesn't even appear in the web interface for CometML, so I assume you can only access it if you query for it directly through their API.

Hints from issue discussion:
PR on this is more than welcome! Great observation. Btw I believe we don't expect users to directly call `self.logger.log_metrics`, but we should still fix it :) 


> val.cpu().detach() vs val.item()

Does Comet accept scalar tensors? If it can do the tensor->Python conversion (why wouldn't it), I would go with `val.cpu().detach()` as in the other loggers.
@neighthan still interested to send a fix for this? 
This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

Hi @awaelchli! I am new to open source contribution and since this is a good first issue, I would like to try my hand at it!
Dear @sohamtiwari3120,

Yes, feel free to take on this one and open a PR.

Best,
T.C
Hi @tchaton,

Can you please review my PR. There are a few checks that failed and I am unable to determine the exact cause for the same.

Sincerely,
Soham
Hey @ sohamtiwari3120,

Approved. Mind adding a test to prevent regression ?

Best,
T.C
Hi @tchaton 

I would love to try! However, it would be my first time writing tests. Therefore could you please help me with the following:
- can you explain how will the test to prevent regression look like,
- also could you provide any references useful for beginners in writing tests.

Sincerely,
Soham
Dear @sohamtiwari3120,

Check out this document: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md

In this case, the test should ensure the values aren't modified the logged metrics owned by the trainer.

Best,
T.C

Commit message: Update CHANGELOG.md

removed changes for tests from changelog, that should not be mentioned

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

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8d3dcedbe003a..7155c85e20691 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -253,6 +253,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 - Fixed not setting a default value for `max_epochs` if `max_time` was specified on the `Trainer` constructor ([#9072](https://github.com/PyTorchLightning/pytorch-lightning/pull/9072))
 
+- Fixed the CometLogger, no longer modifies the metrics in place. Instead creates a copy of metrics before performing any operations ([#9150](https://github.com/PyTorchLightning/pytorch-lightning/pull/9150))
+
 
 ## [1.4.3] - 2021-08-17
 
diff --git a/pytorch_lightning/loggers/comet.py b/pytorch_lightning/loggers/comet.py
index 81084d61b6bd2..9bdafbd265352 100644
--- a/pytorch_lightning/loggers/comet.py
+++ b/pytorch_lightning/loggers/comet.py
@@ -240,11 +240,11 @@ def log_hyperparams(self, params: Union[Dict[str, Any], Namespace]) -> None:
     def log_metrics(self, metrics: Dict[str, Union[torch.Tensor, float]], step: Optional[int] = None) -> None:
         assert rank_zero_only.rank == 0, "experiment tried to log from global_rank != 0"
         # Comet.ml expects metrics to be a dictionary of detached tensors on CPU
-        for key, val in metrics.items():
+        metrics_without_epoch = metrics.copy()
+        for key, val in metrics_without_epoch.items():
             if is_tensor(val):
-                metrics[key] = val.cpu().detach()
+                metrics_without_epoch[key] = val.cpu().detach()
 
-        metrics_without_epoch = metrics.copy()
         epoch = metrics_without_epoch.pop("epoch", None)
         metrics_without_epoch = self._add_prefix(metrics_without_epoch)
         self.experiment.log_metrics(metrics_without_epoch, step=step, epoch=epoch)
diff --git a/tests/loggers/test_comet.py b/tests/loggers/test_comet.py
index cb3641e669d50..4a9121bc01abc 100644
--- a/tests/loggers/test_comet.py
+++ b/tests/loggers/test_comet.py
@@ -15,6 +15,7 @@
 from unittest.mock import DEFAULT, patch
 
 import pytest
+from torch import tensor
 
 from pytorch_lightning import Trainer
 from pytorch_lightning.loggers import CometLogger
@@ -220,3 +221,14 @@ def test_comet_epoch_logging(comet, comet_experiment, tmpdir, monkeypatch):
     logger = CometLogger(project_name="test", save_dir=tmpdir)
     logger.log_metrics({"test": 1, "epoch": 1}, step=123)
     logger.experiment.log_metrics.assert_called_once_with({"test": 1}, epoch=1, step=123)
+
+
+@patch("pytorch_lightning.loggers.comet.CometExperiment")
+@patch("pytorch_lightning.loggers.comet.comet_ml")
+def test_comet_metrics_safe(comet, tmpdir, monkeypatch):
+    """Test that CometLogger.log_metrics doesn't do inplace modification of metrics."""
+    _patch_comet_atexit(monkeypatch)
+    logger = CometLogger(project_name="test", save_dir=tmpdir)
+    metrics = {"tensor": tensor([[1.0, 0.0], [0.0, 1.0]], requires_grad=True), "epoch": 1}
+    logger.log_metrics(metrics)
+    assert metrics["tensor"].requires_grad == True


Code review comments:
diff_hunk: @@ -220,3 +221,14 @@ def test_comet_epoch_logging(comet, comet_experiment, tmpdir, monkeypatch):
     logger = CometLogger(project_name="test", save_dir=tmpdir)
     logger.log_metrics({"test": 1, "epoch": 1}, step=123)
     logger.experiment.log_metrics.assert_called_once_with({"test": 1}, epoch=1, step=123)
+
+
+@patch("pytorch_lightning.loggers.comet.CometExperiment")
+@patch("pytorch_lightning.loggers.comet.comet_ml")
+def test_comet_metrics_safe(comet, tmpdir, monkeypatch):
+    """Test that CometLogger.log_metrics doesn't do inplace modification of metrics."""
+    _patch_comet_atexit(monkeypatch)
+    logger = CometLogger(project_name="test", save_dir=tmpdir)
+    metrics = {"tensor": tensor([[1.0, 0.0], [0.0, 1.0]], requires_grad=True), "epoch": 1}
+    logger.log_metrics(metrics)
+    assert metrics["tensor"].requires_grad == True
path: tests/loggers/test_comet.py
text: @user1:
CI is failing because of this

```suggestion
    assert metrics["tensor"].requires_grad
```