[RFC] Deprecate aggregation logic from base LightningLoggerBase interface

[ananthsub]

Proposed refactor

1. Deprecate these arguments to the base LightningLoggerBase constructor
   https://github.com/PyTorchLightning/pytorch-lightning/blob/dedfde6859a15270385e6e751567b29ab6488e87/pytorch_lightning/loggers/base.py#L49-L73

2. Deprecate this setter for these attributes
   https://github.com/PyTorchLightning/pytorch-lightning/blob/dedfde6859a15270385e6e751567b29ab6488e87/pytorch_lightning/loggers/base.py#L83-L102

3. Deprecate the aggregation logic here, such that the Trainer only calls log_metrics instead of agg_and_log_metrics
   https://github.com/PyTorchLightning/pytorch-lightning/blob/dedfde6859a15270385e6e751567b29ab6488e87/pytorch_lightning/loggers/base.py#L109-L165

Motivation

Simplify the core LightningLoggerBase interface

For 1) and 2):
I personally have never used or even seen others use the agg_key_funcs / agg_default_func arguments on the base logger constructor: https://github.com/PyTorchLightning/pytorch-lightning/blob/eb5b350f9a6bd27a66dfebcb00b3acb33b7bbb89/pytorch_lightning/loggers/base.py#L67-L68

Nor have I seen users update them via the setter: https://github.com/PyTorchLightning/pytorch-lightning/blob/eb5b350f9a6bd27a66dfebcb00b3acb33b7bbb89/pytorch_lightning/loggers/base.py#L83-L102

Moreover, none of the logger implementations even pass through those args to the super().__init__() initialization! I'm sure if these were used, that at least the commonly used logger implementations would support this customization.

Custom logger implementations could still customize this behavior whenever log_metrics is called if absolutely needed. But this isn't required as part of the core logger interface.

For agg_and_log_metrics, this was added to check if there were multiple (key, value) pairs logged for the same batch index, based on #1173. This happens when accumulating gradients across batches.

As the initial issue laid out, a slightly better solution than pushing this to the Logger superclass would be to handle this inside of the training loop when dealing with accumulated gradients: #1173 (comment)

This would simplify the logger interface, but make the training loop more complex. @carmocca do you see this as feasible/desirable with the current loop and logger connector structure? It looks like there's already some handling for gradient accumulation here: https://github.com/PyTorchLightning/pytorch-lightning/blob/dedfde6859a15270385e6e751567b29ab6488e87/pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py#L182-L183

Pitch

1 and 2 are more straightforward. 3 is more challenging but is the ultimate motivator for these deprecations

Additional context

Part of #7740

Originally posted by @ananthsub in #11209 (comment)

---

If you enjoy Lightning, check out our other projects! ⚡

• Metrics: Machine learning metrics for distributed, scalable PyTorch applications.

• Lite: enables pure PyTorch users to scale their existing code on any kind of device while retaining full control over their own loops and optimization logic.

• Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, fine-tuning, and solving problems with deep learning.

• Bolts: Pretrained SOTA Deep Learning models, callbacks, and more for research and production with PyTorch Lightning and PyTorch.

• Lightning Transformers: Flexible interface for high-performance research using SOTA Transformers leveraging Pytorch Lightning, Transformers, and Hydra.

cc @justusschock @awaelchli @akihironitta @rohitgr7 @tchaton @Borda @edward-io @ananthsub

[tchaton]

Hey @ananthsub,

Yes, I agree. This is definitely legacy code and should be refactored.

Like you, I have never seen agg_key_funcs and agg_default_func behind used in the past year.

Concerning the aggregate and log, I believe this was meant to properly handle multiple log_metrics calls.

We should have a check to ensure this is being called only once per global step + one on the epoch end.

[ananthsub]

I realized @edward-io raised this as well back in #9145 and I created a dupe here. I'll close this issue out in favor of the original one.