Repository: pytorch/ignite

Issue:
Make sure that all exp tracking system loggers handle metric types in the same way
## 🚀 Feature

Related to the discussion from here: https://github.com/pytorch/ignite/issues/1476#issuecomment-826314243

We would like to ensure that all our supported experiment tracking loggers handle metric types in the same way as here:

For Neptune (correct):
https://github.com/pytorch/ignite/blob/9230a7319047b37ce19d956e024fa1b86030c30a/ignite/contrib/handlers/neptune_logger.py#L344-L351

In a previous issue we found that the code for WandB is different (incorrect):
https://github.com/pytorch/ignite/blob/9230a7319047b37ce19d956e024fa1b86030c30a/ignite/contrib/handlers/wandb_logger.py#L271-L275

ToDo:
- check `OutputHandler` implementation for all supported loggers: tensorboard_logger.py, clearml_logger.py, mlflow_logger.py, neptune_logger.py, polyaxon_logger.py, tqdm_logger.py, visdom_logger.py, wandb_logger.py
and find those where it is not done as in the "correct" example of Neptune.
- reimplement those loggers using above "correct" example of Neptune.


If interested in working on this issue, please comment out in here such that it could be assigned to you.

Discussion:
vfdev-5: @Muktan thanks for the quick PR ! 

> Checked all the output handler class but only wandb_logger had changes all other loggers check the instance types.
Most loggers added elif isinstance(value, torch.Tensor) and value.ndimension() == 1: condition and inside condition iterated over each item in value and logged it. But I have not added this for wandb_logger because it may change the initial logging behaviour.

I think the point of the issue #1965 is to change to previous behaviour and make it uniform for all loggers.


Also, how about tqdm logger, I think we have to rework _OutputHandler as well : https://github.com/pytorch/ignite/blob/be2bff832b88cfbc0a21828a38c664930f297409/ignite/contrib/handlers/tqdm_logger.py#L275-L286
Muktan: @vfdev-5 thanks for reply!

> I think the point of the issue #1965 is to change to previous behaviour and make it uniform for all loggers.
So do we have to change the actual behaviour.

For example for wandblogger it would be like:

```python
rendered_metrics = {}

if self.tag is not None:
     for name, value in metrics.items():
        if isinstance(value, numbers.Number) or isinstance(value, torch.Tensor) and value.ndimension() == 0: 
             rendered_metrics[f"{self.tag}/{name}"] = value
        elif isinstance(value, torch.Tensor) and value.ndimension() == 1: 
             for i, v in enumerate(value): 
                 rendered_metrics[f"{self.tag}/{name}/{i}"] = v
        else: 
             warnings.warn(f"WandBLogger output_handler can not log metrics value type {type(value)}") 

logger.log(rendered_metrics, step=global_step, sync=self.sync)
```
But the problem with that would occur when we run tests for the file. we have a test case (`test_output_handler_metric_names`) having `value` (`metrics={"a": 55.56, "c": "Some text"}`) as a string which in this case will raise the warning.
So, shall we update the test case? 


> Also, how about tqdm logger, I think we have to rework _OutputHandler as well :
https://github.com/pytorch/ignite/blob/be2bff832b88cfbc0a21828a38c664930f297409/ignite/contrib/handlers/tqdm_logger.py#L275-L286

Yes, for tqdm logger we have to rework _OutputHandler. 

I thought we have to make sure there is instance checking done. But it seems we need to follow the code structure of handling of metric types  as in neptune, for all other loggers, correct?
In that test cases won't fail?



vfdev-5: > But the problem with that would occur when we run tests for the file. we have a test case (test_output_handler_metric_names) having value (metrics={"a": 55.56, "c": "Some text"}) as a string which in this case will raise the warning.
So, shall we update the test case?

That's a good point. I think logging a metric as string is not a common usage. Yes, I think updating the test cases is a solution.
vfdev-5: > Okay, I will try to make a similar code structure for all the other loggers. Also, does the above wandb implementation look correct?

It looks like correct. I was also exploring mlflow logger implementation and explicit call `v.item()` probably could make sense here as well ?

Let me list all implementations and let's decide:
- ClearML
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/clearml_logger.py#L321-L330

- MLflow
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/mlflow_logger.py#L234-L244

- Neptune
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/neptune_logger.py#L342-L349

- Polyaxon
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/polyaxon_logger.py#L245-L255

- TensorBoard
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/tensorboard_logger.py#L287-L294

- TQDM
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/tqdm_logger.py#L274-L286

- Visdom
https://github.com/pytorch/ignite/blob/8cc474644504b2afaceec74790a8b3b43c352832/ignite/contrib/handlers/visdom_logger.py#L367-L382

- W&B
https://github.com/pytorch/ignite/blob/be2bff832b88cfbc0a21828a38c664930f297409/ignite/contrib/handlers/wandb_logger.py#L279-L283

Actually, I think polyaxon and mlflow and tqdm do the data type dispatch in the correct way:
```
if value is a number:
    log or append it to a dict as is: value
elif value is a 0-d Tensor:
    log or append it to a dict using: value.item() 
elif value is a 1-d Tensor:
    loop over values and
        log or append it to a dict using: v.item() 
else:
    warn about unhandled dtype
```

Let's implement this logic to all OutputHandlers. What do you think ?
Muktan: Yes, `value.item()` for type tensor seems more apt.
vfdev-5: @Muktan could you please fix mypy issues ?
Muktan: @vfdev-5 shall I add the test for missing lines as indicated by codecov?
vfdev-5: > @vfdev-5 shall I add the test for missing lines as indicated by codecov?

@Muktan yes, please
vfdev-5: Thanks for the update @Muktan ! Looks good, let's what CI and codecov says.

Patch:
diff --git a/ignite/contrib/handlers/clearml_logger.py b/ignite/contrib/handlers/clearml_logger.py
index a66a4847..c6043797 100644
--- a/ignite/contrib/handlers/clearml_logger.py
+++ b/ignite/contrib/handlers/clearml_logger.py
@@ -319,8 +319,12 @@ class OutputHandler(BaseOutputHandler):
             )
 
         for key, value in metrics.items():
-            if isinstance(value, numbers.Number) or isinstance(value, torch.Tensor) and value.ndimension() == 0:
+            if isinstance(value, numbers.Number):
                 logger.clearml_logger.report_scalar(title=self.tag, series=key, iteration=global_step, value=value)
+            elif isinstance(value, torch.Tensor) and value.ndimension() == 0:
+                logger.clearml_logger.report_scalar(
+                    title=self.tag, series=key, iteration=global_step, value=value.item()
+                )
             elif isinstance(value, torch.Tensor) and value.ndimension() == 1:
                 for i, v in enumerate(value):
                     logger.clearml_logger.report_scalar(
diff --git a/ignite/contrib/handlers/neptune_logger.py b/ignite/contrib/handlers/neptune_logger.py
index a8146569..7d37006e 100644
--- a/ignite/contrib/handlers/neptune_logger.py
+++ b/ignite/contrib/handlers/neptune_logger.py
@@ -340,8 +340,10 @@ class OutputHandler(BaseOutputHandler):
             )
 
         for key, value in metrics.items():
-            if isinstance(value, numbers.Number) or isinstance(value, torch.Tensor) and value.ndimension() == 0:
+            if isinstance(value, numbers.Number):
                 logger.log_metric(f"{self.tag}/{key}", x=global_step, y=value)
+            elif isinstance(value, torch.Tensor) and value.ndimension() == 0:
+                logger.log_metric(f"{self.tag}/{key}", x=global_step, y=value.item())
             elif isinstance(value, torch.Tensor) and value.ndimension() == 1:
                 for i, v in enumerate(value):
                     logger.log_metric(f"{self.tag}/{key}/{i}", x=global_step, y=v.item())
diff --git a/ignite/contrib/handlers/tensorboard_logger.py b/ignite/contrib/handlers/tensorboard_logger.py
index db6cb8ed..9588e210 100644
--- a/ignite/contrib/handlers/tensorboard_logger.py
+++ b/ignite/contrib/handlers/tensorboard_logger.py
@@ -285,8 +285,10 @@ class OutputHandler(BaseOutputHandler):
             )
 
         for key, value in metrics.items():
-            if isinstance(value, numbers.Number) or isinstance(value, torch.Tensor) and value.ndimension() == 0:
+            if isinstance(value, numbers.Number):
                 logger.writer.add_scalar(f"{self.tag}/{key}", value, global_step)
+            elif isinstance(value, torch.Tensor) and value.ndimension() == 0:
+                logger.writer.add_scalar(f"{self.tag}/{key}", value.item(), global_step)
             elif isinstance(value, torch.Tensor) and value.ndimension() == 1:
                 for i, v in enumerate(value):
                     logger.writer.add_scalar(f"{self.tag}/{key}/{i}", v.item(), global_step)
diff --git a/ignite/contrib/handlers/tqdm_logger.py b/ignite/contrib/handlers/tqdm_logger.py
index 4607bf38..b8f0259e 100644
--- a/ignite/contrib/handlers/tqdm_logger.py
+++ b/ignite/contrib/handlers/tqdm_logger.py
@@ -1,8 +1,9 @@
 # -*- coding: utf-8 -*-
 """TQDM logger."""
+import numbers
 import warnings
 from collections import OrderedDict
-from typing import Any, Callable, List, Optional, Union
+from typing import Any, Callable, Dict, List, Optional, Union
 
 import torch
 
@@ -271,19 +272,18 @@ class _OutputHandler(BaseOutputHandler):
 
         metrics = self._setup_output_metrics(engine)
 
-        rendered_metrics = OrderedDict()
+        rendered_metrics = OrderedDict()  # type: Dict[str, Union[str, float, numbers.Number]]
         for key, value in metrics.items():
-            if isinstance(value, torch.Tensor):
-                if value.ndimension() == 0:
-                    rendered_metrics[key] = value.item()
-                elif value.ndimension() == 1:
-                    for i, v in enumerate(value):
-                        k = f"{key}_{i}"
-                        rendered_metrics[k] = v.item()
-                else:
-                    warnings.warn(f"ProgressBar can not log tensor with {value.ndimension()} dimensions")
-            else:
+            if isinstance(value, numbers.Number) or isinstance(value, str):
                 rendered_metrics[key] = value
+            elif isinstance(value, torch.Tensor) and value.ndimension() == 0:
+                rendered_metrics[key] = value.item()
+            elif isinstance(value, torch.Tensor) and value.ndimension() == 1:
+                for i, v in enumerate(value):
+                    k = f"{key}_{i}"
+                    rendered_metrics[k] = v.item()
+            else:
+                warnings.warn(f"ProgressBar can not log tensor with {value.ndimension()} dimensions")
 
         if rendered_metrics:
             logger.pbar.set_postfix(rendered_metrics)  # type: ignore[attr-defined]
diff --git a/ignite/contrib/handlers/visdom_logger.py b/ignite/contrib/handlers/visdom_logger.py
index aab8cfd7..07582390 100644
--- a/ignite/contrib/handlers/visdom_logger.py
+++ b/ignite/contrib/handlers/visdom_logger.py
@@ -368,9 +368,12 @@ class OutputHandler(BaseOutputHandler, _BaseVisDrawer):
 
             values = []  # type: List[Union[float, torch.Tensor]]
             keys = []
-            if isinstance(value, numbers.Number) or isinstance(value, torch.Tensor) and value.ndimension() == 0:
+            if isinstance(value, numbers.Number):
                 values.append(value)  # type: ignore[arg-type]
                 keys.append(key)
+            elif isinstance(value, torch.Tensor) and value.ndimension() == 0:
+                values.append(value.item())
+                keys.append(key)
             elif isinstance(value, torch.Tensor) and value.ndimension() == 1:
                 values = value  # type: ignore[assignment]
                 keys = [f"{key}/{i}" for i in range(len(value))]
diff --git a/ignite/contrib/handlers/wandb_logger.py b/ignite/contrib/handlers/wandb_logger.py
index c59fcbfe..d9dfabdf 100644
--- a/ignite/contrib/handlers/wandb_logger.py
+++ b/ignite/contrib/handlers/wandb_logger.py
@@ -1,6 +1,9 @@
 """WandB logger and its helper handlers."""
-from typing import Any, Callable, List, Optional, Union
+import numbers
+import warnings
+from typing import Any, Callable, Dict, List, Optional, Union
 
+import torch
 from torch.optim import Optimizer
 
 from ignite.contrib.handlers.base_logger import BaseLogger, BaseOptimizerParamsHandler, BaseOutputHandler
@@ -277,10 +280,20 @@ class OutputHandler(BaseOutputHandler):
             )
 
         metrics = self._setup_output_metrics(engine)
+        rendered_metrics = {}  # type: Dict[str, Union[str, float, numbers.Number]]
         if self.tag is not None:
-            metrics = {f"{self.tag}/{name}": value for name, value in metrics.items()}
-
-        logger.log(metrics, step=global_step, sync=self.sync)
+            for name, value in metrics.items():
+                if isinstance(value, numbers.Number) or isinstance(value, str):
+                    rendered_metrics[f"{self.tag}/{name}"] = value
+                elif isinstance(value, torch.Tensor) and value.ndimension() == 0:
+                    rendered_metrics[f"{self.tag}/{name}"] = value.item()
+                elif isinstance(value, torch.Tensor) and value.ndimension() == 1:
+                    for i, v in enumerate(value):
+                        rendered_metrics[f"{self.tag}/{name}/{i}"] = v.item()
+                else:
+                    warnings.warn(f"WandBLogger output_handler can not log metrics value type {type(value)}")
+
+        logger.log(rendered_metrics, step=global_step, sync=self.sync)
 
 
 class OptimizerParamsHandler(BaseOptimizerParamsHandler):


Test patch:
diff --git a/tests/ignite/contrib/handlers/test_clearml_logger.py b/tests/ignite/contrib/handlers/test_clearml_logger.py
index 9ac30f7c..58e4dc79 100644
--- a/tests/ignite/contrib/handlers/test_clearml_logger.py
+++ b/tests/ignite/contrib/handlers/test_clearml_logger.py
@@ -183,6 +183,27 @@ def test_output_handler_metric_names(dirname):
         any_order=True,
     )
 
+    # log a torch tensor (ndimension = 0)
+    wrapper = OutputHandler("tag", metric_names="all")
+    mock_logger = MagicMock(spec=ClearMLLogger)
+    mock_logger.clearml_logger = MagicMock()
+
+    mock_engine = MagicMock()
+    mock_engine.state = State(metrics={"a": torch.tensor(12.23), "b": torch.tensor(23.45), "c": torch.tensor(5.01)})
+    mock_engine.state.iteration = 5
+
+    wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
+
+    assert mock_logger.clearml_logger.report_scalar.call_count == 3
+    mock_logger.clearml_logger.report_scalar.assert_has_calls(
+        [
+            call(title="tag", series="a", iteration=5, value=torch.tensor(12.23).item()),
+            call(title="tag", series="b", iteration=5, value=torch.tensor(23.45).item()),
+            call(title="tag", series="c", iteration=5, value=torch.tensor(5.01).item()),
+        ],
+        any_order=True,
+    )
+
 
 def test_output_handler_both(dirname):
 
diff --git a/tests/ignite/contrib/handlers/test_neptune_logger.py b/tests/ignite/contrib/handlers/test_neptune_logger.py
index c50ac64b..e75f65fe 100644
--- a/tests/ignite/contrib/handlers/test_neptune_logger.py
+++ b/tests/ignite/contrib/handlers/test_neptune_logger.py
@@ -94,9 +94,11 @@ def test_output_handler_metric_names():
     wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
 
     assert mock_logger.log_metric.call_count == 2
-    mock_logger.log_metric.assert_has_calls([call("tag/a", y=12.23, x=5), call("tag/b", y=23.45, x=5),], any_order=True)
+    mock_logger.log_metric.assert_has_calls(
+        [call("tag/a", y=12.23, x=5), call("tag/b", y=23.45, x=5)], any_order=True,
+    )
 
-    wrapper = OutputHandler("tag", metric_names=["a",])
+    wrapper = OutputHandler("tag", metric_names=["a",],)
 
     mock_engine = MagicMock()
     mock_logger.log_metric = MagicMock()
@@ -132,7 +134,9 @@ def test_output_handler_metric_names():
         wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
 
     assert mock_logger.log_metric.call_count == 1
-    mock_logger.log_metric.assert_has_calls([call("tag/a", y=55.56, x=7),], any_order=True)
+    mock_logger.log_metric.assert_has_calls(
+        [call("tag/a", y=55.56, x=7),], any_order=True,
+    )
 
     # all metrics
     wrapper = OutputHandler("tag", metric_names="all")
@@ -145,7 +149,25 @@ def test_output_handler_metric_names():
     wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
 
     assert mock_logger.log_metric.call_count == 2
-    mock_logger.log_metric.assert_has_calls([call("tag/a", y=12.23, x=5), call("tag/b", y=23.45, x=5),], any_order=True)
+    mock_logger.log_metric.assert_has_calls(
+        [call("tag/a", y=12.23, x=5), call("tag/b", y=23.45, x=5),], any_order=True,
+    )
+
+    # log a torch tensor (ndimension = 0)
+    wrapper = OutputHandler("tag", metric_names="all")
+    mock_logger = MagicMock(spec=NeptuneLogger)
+    mock_logger.log_metric = MagicMock()
+    mock_engine = MagicMock()
+    mock_engine.state = State(metrics={"a": torch.tensor(12.23), "b": torch.tensor(23.45)})
+    mock_engine.state.iteration = 5
+
+    wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
+
+    assert mock_logger.log_metric.call_count == 2
+    mock_logger.log_metric.assert_has_calls(
+        [call("tag/a", y=torch.tensor(12.23).item(), x=5), call("tag/b", y=torch.tensor(23.45).item(), x=5),],
+        any_order=True,
+    )
 
 
 def test_output_handler_both():
@@ -369,12 +391,12 @@ def test_grads_scalar_handler_frozen_layers(dummy_model_factory, norm_mock):
     wrapper(mock_engine, mock_logger, Events.EPOCH_STARTED)
 
     mock_logger.log_metric.assert_has_calls(
-        [call("grads_norm/fc2/weight", y=ANY, x=5), call("grads_norm/fc2/bias", y=ANY, x=5),], any_order=True
+        [call("grads_norm/fc2/weight", y=ANY, x=5), call("grads_norm/fc2/bias", y=ANY, x=5),], any_order=True,
     )
 
     with pytest.raises(AssertionError):
         mock_logger.log_metric.assert_has_calls(
-            [call("grads_norm/fc1/weight", y=ANY, x=5), call("grads_norm/fc1/bias", y=ANY, x=5),], any_order=True
+            [call("grads_norm/fc1/weight", y=ANY, x=5), call("grads_norm/fc1/bias", y=ANY, x=5),], any_order=True,
         )
     assert mock_logger.log_metric.call_count == 2
     assert norm_mock.call_count == 2
diff --git a/tests/ignite/contrib/handlers/test_tensorboard_logger.py b/tests/ignite/contrib/handlers/test_tensorboard_logger.py
index bdc0418a..ad063a19 100644
--- a/tests/ignite/contrib/handlers/test_tensorboard_logger.py
+++ b/tests/ignite/contrib/handlers/test_tensorboard_logger.py
@@ -147,6 +147,22 @@ def test_output_handler_metric_names():
     assert mock_logger.writer.add_scalar.call_count == 2
     mock_logger.writer.add_scalar.assert_has_calls([call("tag/a", 12.23, 5), call("tag/b", 23.45, 5),], any_order=True)
 
+    # log a torch tensor (ndimension = 0)
+    wrapper = OutputHandler("tag", metric_names="all")
+    mock_logger = MagicMock(spec=TensorboardLogger)
+    mock_logger.writer = MagicMock()
+
+    mock_engine = MagicMock()
+    mock_engine.state = State(metrics={"a": torch.tensor(12.23), "b": torch.tensor(23.45)})
+    mock_engine.state.iteration = 5
+
+    wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
+
+    assert mock_logger.writer.add_scalar.call_count == 2
+    mock_logger.writer.add_scalar.assert_has_calls(
+        [call("tag/a", torch.tensor(12.23).item(), 5), call("tag/b", torch.tensor(23.45).item(), 5),], any_order=True
+    )
+
 
 def test_output_handler_both():
 
diff --git a/tests/ignite/contrib/handlers/test_wandb_logger.py b/tests/ignite/contrib/handlers/test_wandb_logger.py
index 628bfae4..2ca558c8 100644
--- a/tests/ignite/contrib/handlers/test_wandb_logger.py
+++ b/tests/ignite/contrib/handlers/test_wandb_logger.py
@@ -133,6 +133,30 @@ def test_output_handler_metric_names():
     wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
     mock_logger.log.assert_called_once_with({"tag/a": 12.23, "tag/b": 23.45}, step=5, sync=None)
 
+    # log a torch vector
+    wrapper = OutputHandler("tag", metric_names="all")
+    mock_logger = MagicMock(spec=WandBLogger)
+    mock_logger.log = MagicMock()
+    vector = torch.tensor([0.1, 0.2, 0.1, 0.2, 0.33])
+    mock_engine = MagicMock()
+    mock_engine.state = State(metrics={"a": vector})
+    mock_engine.state.iteration = 5
+
+    wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
+    mock_logger.log.assert_called_once_with({f"tag/a/{i}": vector[i].item() for i in range(5)}, step=5, sync=None)
+
+    # log warning
+    wrapper = OutputHandler("tag", metric_names=["a"])
+    mock_engine = MagicMock()
+    mock_engine.state = State(metrics={"a": [1, 2, 3, 4]})
+    mock_engine.state.iteration = 7
+
+    mock_logger = MagicMock(spec=WandBLogger)
+    mock_logger.log = MagicMock()
+
+    with pytest.warns(UserWarning, match=r"WandBLogger output_handler can not log metrics value type"):
+        wrapper(mock_engine, mock_logger, Events.ITERATION_STARTED)
+
 
 def test_output_handler_both():
     wrapper = OutputHandler("tag", metric_names=["a", "b"], output_transform=lambda x: {"loss": x})