Lightning-AI / Lightning-AI/pytorch-lightning
Deprecate `save` from the Logger Base API
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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