Lightning-AI / Lightning-AI/pytorch-lightning

Confusing recommendation to use sync_dist=True even with TorchMetrics

Open
#20,153 17 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug help wanted logging ver: 2.2.x
Dominant language
Python
Stars
31.4k
Forks
3.8k
Avg merge
6d 7h
Merged PRs (30d)
6

Description

### Bug description

Hello!

When I train and validate a model in a multi-GPU setting (HPC, sbatch job that requests multiple GPUs on a single node), I use `self.log(..., sync_dist=True)` when logging PyTorch losses, and don't specify any value for `sync_dist` when logging metrics from TorchMetrics library. However, I still get warnings like
```
...
It is recommended to use `self.log('val_mean_recall', ..., sync_dist=True)` when logging on epoch level in distributed setting to accumulate the metric across devices.
...
It is recommended to use `self.log('val_bg_recall', ..., sync_dist=True)` when logging on epoch level in distributed setting to accumulate the metric across devices.
```
These specific messages correspond to logging `tmc.MulticlassRecall(len(self.task.class_names), average="macro", ignore_index=self.metric_ignore_index)` and individual components of `tmc.MulticlassRecall(len(self.task.class_names), average="none", ignore_index=self.metric_ignore_index)`.

Full code listing for metric object definitions and logging is provided in the "reproducing the bug" section.

As I understand from a note [here](https://lightning.ai/docs/torchmetrics/stable/pages/lightning.html#logging-torchmetrics), and from discussion [here](https://github.com/Lightning-AI/pytorch-lightning/discussions/6501#discussioncomment-553152), one doesn't typically need to explicitly use `sync_dist` when using TorchMetrics.

I wonder if I still need to enable `sync_dist=True` as advised in the warnings due to some special case that I am not aware about, or should I follow the docs and keep it as is? In any case, this is probably a bug, either in documentation, or in warning code.

Thank you!

### What version are you seeing the problem on?

2.3.0

### How to reproduce the bug

```python
self.val_metric_funs = tm.MetricCollection(
{
"cm_normalize_all": tmc.MulticlassConfusionMatrix(
len(self.task.class_names),
ignore_index=self.metric_ignore_index,
normalize="all",
),
"recall_average_macro": tmc.MulticlassRecall(
len(self.task.class_names),
average="macro",
ignore_index=self.metric_ignore_index,
),
"recall_average_none": tmc.MulticlassRecall(
len(self.task.class_names),
average="none",
ignore_index=self.metric_ignore_index,
),
"precision_average_macro": tmc.MulticlassPrecision(
len(self.task.class_names),
average="macro",
ignore_index=self.metric_ignore_index,
),
"precision_average_none": tmc.MulticlassPrecision(
len(self.task.class_names),
average="none",
ignore_index=self.metric_ignore_index,
),
"f1_average_macro": tmc.MulticlassF1Score(
len(self.task.class_names),
average="macro",
ignore_index=self.metric_ignore_index,
),
"f1_average_none": tmc.MulticlassF1Score(
len(self.task.class_names),
average="none",
ignore_index=self.metric_ignore_index,
),
}
)
```

```
if not sanity_check:
for metric_name, metric in metrics.items():
metric_fun = self.val_metric_funs[metric_name]
metric_name_ = metric_name.split("_")[0]
if isinstance(metric_fun, tmc.MulticlassConfusionMatrix):
for true_class_num in range(metric.shape[0]):
true_class_name = self.task.class_names[true_class_num]
for pred_class_num in range(metric.shape[1]):
pred_class_name = self.task.class_names[pred_class_num]
self.log(
f"val_true_{true_class_name}_pred_{pred_class_name}_cm",
metric[true_class_num, pred_class_num].item(),
on_step=False,
on_epoch=True,
logger=True,
)
elif isinstance(
metric_fun,
(
tmc.MulticlassRecall,
tmc.MulticlassPrecision,
tmc.MulticlassF1Score,
),
):
if metric_fun.average == "macro":
self.log(
f"val_mean_{metric_name_}",
metric.item(),
on_step=False,
on_epoch=True,
logger=True,
)
elif metric_fun.average == "none":
for class_num, metric_ in enumerate(metric):
class_name = self.task.class_names[class_num]
self.log(
f"val_{class_name}_{metric_name_}",
metric_.item(),
on_step=False,
on_epoch=True,
logger=True,
)
else:
raise NotImplementedError(
f"Code for logging metric {metric_name} is not implemented"
)
else:
raise NotImplementedError(
f"Code for logging metric {metric_name} is not implemented"
)
```

### Error messages and logs

```
...
It is recommended to use `self.log('val_mean_recall', ..., sync_dist=True)` when logging on epoch level in distributed setting to accumulate the metric across devices.
...
It is recommended to use `self.log('val_bg_recall', ..., sync_dist=True)` when logging on epoch level in distributed setting to accumulate the metric across devices.
```

### Environment

Current environment

```
#- PyTorch Lightning Version: 2.3.0
#- PyTorch Version: 2.3.1
#- Python version: 3.11.9
#- OS: Linux
#- CUDA/cuDNN version: 11.8
#- How you installed Lightning: conda-forge
```

### More info

_No response_

cc @carmocca

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the self.log calls and compare the warning behavior with the linked TorchMetrics logging guidance and discussion. Reproduce the distributed validation case with the shown MetricCollection, then determine whether the warning should account for TorchMetrics-managed synchronization; done means the warning and documentation consistently describe the required setting.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, pytorch
Domain
distributed-systems, machine-learning
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.