[RFC] Remove log_text and log_image from LightningLoggerBase API

[daniellepintz]

Proposed refactor

Remove these methods from the LightningLoggerBase API:
https://github.com/PyTorchLightning/pytorch-lightning/blob/8d23f6287adf1e1040948802e6649c4ae2b3eda1/pytorch_lightning/loggers/base.py#L193-L205

Motivation

log_text and log_image are both only overridden on 2 loggers: WandbLogger and NeptuneLogger.

For the rest of the loggers, since these methods are on the Base API it seems to imply that you can call them, but if you do for most loggers you will get a NotImplementedError. This is not a good user experience and it would be better to just keep these methods specific to WandbLogger and NeptuneLogger so it is clear that they can only be called for those loggers.

Similar motivation to #11234

From #11234: "A good rule of thumb is that the only required interfaces for the Logger are what the Trainer directly calls. Everything else can be pushed to various implementations". Since the trainer does not call log_text or log_image these do not need to be required on the Logger API.

Additional Context

These were added in #9545

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

[ananthsub]

The Trainer explicitly calls log_graph here: https://github.com/PyTorchLightning/pytorch-lightning/blob/789fae828dd3c3196b09bb5451e568c776dc99a6/pytorch_lightning/trainer/trainer.py#L1243

This means it's required to be on the logger API

In general, this issue (and any deprecation) should have much more context as to:

• Why these features were added (links to the original issues & PRs for discussion are very helpful for context)
• What their current usage is, either in the codebase or by users
• Why do we need to deprecate it? Is it that the feature is broken? Has significant scalability issues? Or that it can be better extended elsewhere?

[akashkw]

The Trainer explicitly calls log_graph here:

https://github.com/PyTorchLightning/pytorch-lightning/blob/789fae828dd3c3196b09bb5451e568c776dc99a6/pytorch_lightning/trainer/trainer.py#L1243

This means it's required to be on the logger API

In general, this issue (and any deprecation) should have much more context as to:

• Why these features were added (links to the original issues & PRs for discussion are very helpful for context)
• What their current usage is, either in the codebase or by users
• Why do we need to deprecate it? Is it that the feature is broken? Has significant scalability issues? Or that it can be better extended elsewhere?

@ananthsub I noticed this as well, but for most loggers this is a no-op because log_graph has nothing but a pass statement in the base implementation. I simply wrapped this in hasattr(self.logger, "log_graph"). Is this a reasonable solution? I don't see any other references to log_graph in the trainer.

Other than passing or checking for the attribute what else could we do if a logger doesn't support log_graph?

[daniellepintz]

Updated the issue to not include log_graph, and will update it shortly with more information overall.

[ananthsub]

Based on #9545, I agree with the motivations:

• WandbLogger.log_image and WandbLogger.log_text still exist, so the primary use case is unaffected by this
• WandbLogger does not require these interfaces on the base class
• The arguments to these functions are completely left open. This means all argument validation is left to logger subclasses, and users don't get the benefit of typehints, which can easily lead to errors.
• Only log_image and log_text were added to the base class, but log_table wasn't, despite being added in feat(wandb): support media logging #9545. The inconsistency here demonstrates that these methods don't need to be on the base class

[daniellepintz]

Yeah, good points, especially about log_table!