Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate save from the Logger Base API #11676

Open
daniellepintz opened this issue Jan 31, 2022 · 5 comments
Open

Deprecate save from the Logger Base API #11676

daniellepintz opened this issue Jan 31, 2022 · 5 comments
Labels
deprecation Includes a deprecation logger Related to the Loggers refactor
Milestone

Comments

@daniellepintz
Copy link
Contributor

daniellepintz commented Jan 31, 2022

Proposed refactor

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

Motivation

After #9145 save on the LightningLoggerBase does nothing, and only 2 loggers have implementations for it: CSVLogger and TensorboardLogger.

After #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:

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 #9004

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

@daniellepintz daniellepintz added refactor logger Related to the Loggers deprecation Includes a deprecation labels Jan 31, 2022
@ananthsub ananthsub added this to the 1.7 milestone Feb 9, 2022
@ananthsub
Copy link
Contributor

Removing save from LightningLoggerBase makes sense to do only after v1.6 is released because flush_logs_every_n_steps will be removed in v1.7.
So the deprecation of save and removal of flush_logs_every_n_steps would happen in v1.7, with save targeted to be removed in v1.9.

@daniellepintz
Copy link
Contributor Author

Hmm, sorry I'm not quite following why we would have to wait until flush_logs_every_n_steps is removed?

@daniellepintz
Copy link
Contributor Author

@awaelchli @carmocca We didn't have time to finish discussing this one the other day but I just added more to the Motivation section. Please take a look when you get the chance.

@carmocca
Copy link
Contributor

Looks good. One note though

Move the code currently in save for LightningLoggerBase, CSV, and Tensorboard to log_metrics

Instead, I think the subclasses should keep the save method and log_metrics should call self.save().

@daniellepintz
Copy link
Contributor Author

daniellepintz commented Feb 22, 2022

@carmocca sounds good! Updated the pitch

@carmocca carmocca modified the milestones: pl:1.7, pl:future Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation logger Related to the Loggers refactor
Projects
None yet
Development

No branches or pull requests

4 participants