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

[RFC] Deprecate LightningLoggerBase.experiment requirement from Logger interface #11234

Closed
ananthsub opened this issue Dec 23, 2021 · 2 comments · Fixed by #11603
Closed

[RFC] Deprecate LightningLoggerBase.experiment requirement from Logger interface #11234

ananthsub opened this issue Dec 23, 2021 · 2 comments · Fixed by #11603
Assignees
Labels
deprecation Includes a deprecation design Includes a design discussion logger Related to the Loggers refactor

Comments

@ananthsub
Copy link
Contributor

ananthsub commented Dec 23, 2021

Proposed refactor

Deprecate the experiment property required by the Logger interface

https://github.com/PyTorchLightning/pytorch-lightning/blob/dedfde6859a15270385e6e751567b29ab6488e87/pytorch_lightning/loggers/base.py#L104-L107

Motivation

  1. experiment carries no meaning as specified on the base logger. This is an object which can be anything.

Currently, Lightning appears to promise users that logger.experiment will have a consistent interface across different logger implementations, which is not true!

If users are interacting with the experiment directly, then they must already know what Logger implementation they're using.

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 interact with the experiment at all, it does not need to be required on the Logger API.

What makes more sense is for the various logger implementations to directly offer public property on their interface. For example, the TensorBoard logger could expose the SummaryWriter as a property in its public interface for Tensorboard logger users to call in their own code.

  1. Authors of custom loggers need to define this seemingly random experiment property without knowing how/when it will be called.

Pitch

  1. Make the current experiment definitions in the logger implementations top-level properties in their respective classes.
  2. Deprecate experiment off the base logger API

Additional context

Originally posted by @ananthsub in #11209 (comment)

Part of #7740

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

@ananthsub ananthsub added refactor design Includes a design discussion logger Related to the Loggers labels Dec 23, 2021
@ananthsub ananthsub changed the title [RFC] Deprecate Logger.experiment [RFC] Deprecate Logger.experiment from Logger interface Dec 23, 2021
@ananthsub ananthsub changed the title [RFC] Deprecate Logger.experiment from Logger interface [RFC] Deprecate LightningLoggerBase.experiment requirement from Logger interface Dec 23, 2021
@ananthsub ananthsub added the deprecation Includes a deprecation label Dec 23, 2021
@tchaton
Copy link
Contributor

tchaton commented Jan 4, 2022

Hey @ananthsub,

I understand your arguments. IMO, I don't believe users are expecting a common API for the experiment object from experience as it is linked to the Logger Client, but I agree this would make sense to provide a cleaner API for the base.

@daniellepintz daniellepintz self-assigned this Jan 5, 2022
@wilson100hong
Copy link

@ananthsub SGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment