Lightning-AI / Lightning-AI/pytorch-lightning

Deprecate `save` from the Logger Base API

Open
#11,676 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

deprecation logger refactor
Dominant language
Python
Stars
31.4k
Forks
3.8k
Avg merge
6d 7h
Merged PRs (30d)
6

Description

## Proposed refactor

Deprecate `save` from `LightningLoggerBase`:
https://github.com/PyTorchLightning/pytorch-lightning/blob/a8ee5cacb7b31064c8b73372ca944d43f7caadc6/pytorch_lightning/loggers/base.py#L173-L175

### Motivation

After https://github.com/PyTorchLightning/pytorch-lightning/issues/9145 `save` on the `LightningLoggerBase` does nothing, and only 2 loggers have implementations for it: `CSVLogger` and `TensorboardLogger`.

After https://github.com/PyTorchLightning/pytorch-lightning/issues/8991 we now consider flushing an internal implementation detail of the loggers, so it follows that `save` should also be an internal implementation detail of the loggers.

Currently the Trainer calls `log_metrics` and `save` together here:
https://github.com/PyTorchLightning/pytorch-lightning/blob/6bc0e1da6329c42ec3b7c6fbf91eb059b9124207/pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py#L131-L132

Instead, the Trainer can just call `log_metrics` and the actual saving can be delegated completely to the logger, including buffering & flush frequency.

### Pitch
in v1.6:
* Deprecate `save` from `LightningLoggerBase`
* For the following three `save` calls, only call if `save` is overridden in the Logger:
https://github.com/PyTorchLightning/pytorch-lightning/blob/6bc0e1da6329c42ec3b7c6fbf91eb059b9124207/pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py#L132
https://github.com/PyTorchLightning/pytorch-lightning/blob/6bc0e1da6329c42ec3b7c6fbf91eb059b9124207/pytorch_lightning/trainer/trainer.py#L1241
https://github.com/PyTorchLightning/pytorch-lightning/blob/6bc0e1da6329c42ec3b7c6fbf91eb059b9124207/pytorch_lightning/loops/epoch/training_epoch_loop.py#L508
(Note: the final `save` call above will be removed in v1.7 following the removal of `flush_logs_every_n_steps`)

in v1.8:
* For `CSVLogger` and `TensorboardLogger` their `log_metrics` and `log_graph` implementations should call `save`
* Remove `save` from `LightningLoggerBase`

### Additional Context
This was discussed in https://github.com/PyTorchLightning/pytorch-lightning/discussions/9004

cc @justusschock @awaelchli @akihironitta @rohitgr7 @edward-io @borda @ananthsub @kamil-kaczmarek @Raalsky @Blaizzy @tchaton

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 with pytorch_lightning/loggers/base.py and the three save call sites in logger_connector.py, trainer.py, and training_epoch_loop.py. Compare the Logger implementations for CSVLogger and TensorboardLogger, then review the linked discussions and issues for the intended deprecation sequence. Done means the v1.6 save-call behavior is updated without breaking logger tests, with the later removal work clearly separated.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, pytorch
Domain
backend, tooling
Issue type
Refactor
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.