Lightning logger aggregation API deprecation

[edward-io]

Proposed refactoring or deprecation

Move and/or deprecate aggregation-related code to individual loggers

Motivation

We are auditing the Lightning components and APIs to assess opportunities for improvements:

• https://docs.google.com/document/d/1xHU7-iQSpp9KJTjI3As2EM0mfNHHr37WZYpDpwLkivA/edit#

• Review Lightning architecture & API #7740

Revisiting some of the API decisions regarding aggregations in Lightning Logger to simplify the interface and move logic specific to individual loggers away from the base class:

• Aggregation is currently being done by the step dimension. Users may want more control over complex aggregations, like aggregating metrics over a time window.
• It's not clear to the end-user whether they should use agg_and_log_metrics or log_metrics. In LoggerConnector, when log_metrics is called, we call agg_and_log_metrics.
  • Which method(s) should developers override when creating their custom logger?
• LightningLoggerBase.__init__ accepts two arguments, agg_key_funcs, and agg_default_func. They aren't being called in any sub-classed loggers within Lightning. They can be implementation details of the loggers instead.
  • Do we know if users are adding aggregation functionality using update_agg_funcs directly?

Pitch

• Simplify LightningLoggerBase.__init__ by removing agg_key_funcs and agg_default_func
• Move aggregation-related logger code out of base class to simplify the interface
• Provide clarity between what the Lightning trainer calls between log_metrics and agg_and_log_metrics. Proposal: the trainer should only call log_metrics

Additional context

Related issues:

#8991

#9004

---

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

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

• Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, finetuning 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.

[ananthsub]

Thanks for filing this @edward-io !

I believe this is very related to #8991 and #9004

I agree that we can simplify the base interface for logging metrics. At the very least I see:

• log_metrics -> can internally aggregate/buffer data before logging
• teardown -> flushes any buffered data and closes the experiment

Do we need a flush interface at all on the base logger in this case?

@awaelchli @tchaton @carmocca ?

[awaelchli]

Move aggregation-related logger code out of base class to simplify the interface

where would you move it?

Provide clarity between what the Lightning trainer calls between log_metrics and agg_and_log_metrics. Proposal: the trainer should only call log_metrics

sounds good as long, as we keep exactly the same logging behavior

Do we need a flush interface at all on the base logger in this case?

what we know as flush in the logger connector is just the save() function in the loggers.

[stale]

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

[wilson100hong]

We use some metrics as counter which keep accumulating new logged values. Without agg_and_log_metrics(), users might need API to get metrics values from the logger (like get_metric() in the below example) to do that?

new_value = logger.get_metric(key) + value.  # we don't have get_metric() now 
logger.set_metrics({key: new_value})

[daniellepintz]

@wilson100hong can you put your custom logic into log_metrics directly?

[daniellepintz]

I did a test in https://colab.research.google.com/drive/1SSOOmeJJf8eU4HE4UkPgd2GbtpfRPQa_?usp=sharing and it seems to me that the aggregation logic in agg_and_log_metrics is not currently having an effect. See in the colab I am logging the value 2.0 on even batch_indices, and 3.0 on odd batch_indices, with accumulate_grad_batches=2, but instead of averaging the values, in the TB logger we are just outputting 3.0 on every step.

Thus I believe we can move forward with the deprecation of agg_and_log_metrics, and can immediately replace this line:
https://github.com/PyTorchLightning/pytorch-lightning/blob/86b177ebe5427725b35fde1a8808a7b59b8a277a/pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py#L119

with self.trainer.logger.log_metrics(scalar_metrics, step=step)

[awaelchli]

See in the colab I am logging the value 2.0 on even steps and 3.0 on odd steps, with accumulate_grad_batches=2, but instead of averaging the values, in the TB logger we are just outputting 3.0 on every step.

Why would they be averaged? The example only makes sense when the keys you log are different.
The aggregation is pointless if we log the same key. Aggregation is meant for the following situation:

agg_and_log_metrics("key1", 1.0)  # this adds the key1, does not write out the logs
agg_and_log_metrics("key2", 2.0)  # this adds the key2, does not write out the logs
agg_and_log_metrics("key3", 3.0)  # this adds the key3, does not write out the logs

# this will make the actual call to self.log_metrics
# and this will log a single dict with all keys aggregated: {"key1": 1.0, "key2": 2.0, "key3": 3.0} 
save() 

Let's please not remove this feature under the wrong understanding.
Instead, we should check whether it is redundant today. After all, the logger connector has been added and revamped after logger base was initially designed. The logger connector may already take the role of aggregation today. cc @carmocca

[daniellepintz]

@awaelchli I am confused by your last comment :/ I was under the impression that aggregation was done per key?
see docstring here: https://github.com/PyTorchLightning/pytorch-lightning/blob/f79d75d4b5e50eb3e37073606eba5ca10d48c99a/pytorch_lightning/loggers/base.py#L409-L415

[daniellepintz]

Why would they be averaged?

They would be averaged because that is the default aggregation function: https://github.com/PyTorchLightning/pytorch-lightning/blob/f79d75d4b5e50eb3e37073606eba5ca10d48c99a/pytorch_lightning/loggers/base.py#L66

Also, in my previous comment where I said "I am logging the value 2.0 on even steps and 3.0 on odd steps" instead of step I meant batch_idx. Sorry if that added confusion. I have updated the comment.

[carmocca]

On these topics, it helps to dig up the history: #1173 (issue), #1278 (PR)

I think this can be removed as "logging" manages key collisions already.

[daniellepintz]

Closing this issue as all PRs have landed!