Difference between save, finalize and close in Logger?

[edward-io]

For the LoggerBase, we have 3 similar methods, save, finalize, and close. The default implementation for finalize and close just call save.

save only calls _finalize_agg_metrics by default.

Do we need all three? If not, we can consolidate finalize and close into a single method, as none of the existing loggers have any distinction between the two, and make that method abstract for other loggers to implement.

save also makes the assumption that all loggers will use _metrics_to_agg to buffer log data. If users have their own way of buffering data, users would need to override many in the base logger, like agg_and_log_metrics, save, and agg_key_funcs.

[ananthsub]

cc @carmocca @awaelchli @tchaton do you know why we have these 3?

[awaelchli]

Looks like save is currently used to implement saving from memory to disk, and together with Trainer.flush_ever_n_steps this is controlled periodically.

close() seems to be only relevant to test tube logger at the moment

finalize() probably has to stay. we should have some form of method that runs at the end of a run/fit or something that the user can call externally to trigger the end of an experiment. For experiment management

[tchaton]

@ananthsub We could potentially depreciate TestTube support as it is not actively maintained and therefore remove the need for .close.

[ananthsub]

Looks like save is currently used to implement saving from memory to disk, and together with Trainer.flush_ever_n_steps this is controlled periodically.

do you feel that save is an internal implementation detail of the logger? i think the trainer really ought to call log_metrics and the actual saving can be delegated completely to the logger, including buffering & flush frequency.

finalize() probably has to stay. we should have some form of method that runs at the end of a run/fit or something that the user can call externally to trigger the end of an experiment. For experiment management

I agree. teardown seems to be the API name we use elsewhere in lightning

We could potentially depreciate TestTube support as it is not actively maintained and therefore remove the need for .close.

@tchaton ah I didn't realize TestTube was another project of @williamFalcon 's, neat! If it's not actively maintained, then this cleanup makes sense!

[awaelchli]

@ananthsub if flush_every_n_steps ends up in the logger directly as proposed in #8991 then save() can be entirely internal to the logger, so far I see.