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

Abstract matplotlib figure logging from individual logger API #4973

Closed
Haydnspass opened this issue Dec 4, 2020 · 6 comments
Closed

Abstract matplotlib figure logging from individual logger API #4973

Haydnspass opened this issue Dec 4, 2020 · 6 comments
Labels
feature Is an improvement or enhancement help wanted Open to be worked on logger Related to the Loggers won't fix This will not be worked on

Comments

@Haydnspass
Copy link

🚀 Feature

Implement default log_figure for all implemented loggers

Motivation

Currently, logging figures relies on calling the API of the individual logger directly. This is not really convenient for a variety of reasons:

  • It is cumbersome to change loggers
  • It is cumbersome to disable logging (e.g. for debugging) --> if self.logger is not None: \n[...]
  • It is not really nice if you have multiple loggers

Pitch

I propose something like
logger.log_figure(figure_name, plt.figure, step, close_figure, kwargs)
where the kwargs are passed on to the respective logger implementation (i.e. if one wants something specific).

Additional context

Should a log_image method also be considered? Should it rather be log_figures (plural, i.e. passing multiple figures)?

@Haydnspass Haydnspass added feature Is an improvement or enhancement help wanted Open to be worked on labels Dec 4, 2020
@SeanNaren SeanNaren added the logging Related to the `LoggerConnector` and `log()` label Dec 4, 2020
@edenlightning edenlightning linked a pull request Dec 4, 2020 that will close this issue
18 tasks
@awaelchli awaelchli added logger Related to the Loggers and removed logging Related to the `LoggerConnector` and `log()` labels Dec 6, 2020
@awaelchli
Copy link
Contributor

This is a very difficult feature to implement. There is a great disparity in the API between the loggers regarding the logging of images and graphs and it would be quite the challenge to bring this under a common denominator without sacrificing functionality.

You mention that kwargs could be used to pass down specific arguments to a logger but that will defeat the purpose because changing the logger externally will break the LightningModule as it would today anyway.

One other point one has to consider is that if the third party api changes our wrappers will also break very easily and adds maintenance cost.

@Haydnspass
Copy link
Author

I agree that the kwargs option kind of contradicts the purpose of changing loggers and is difficult for multiple loggers, it still solves the temporary disabling logger issue though. But I agree, if one wants more than a default implementation one should possibly ask the user to still us the actual API via logger.experiment.[]

However, I don't quite agree with the API. To me, it is not really different than the differences in API for metrics among the different loggers. Parsing a matplotlib figure is a pretty standard thing which is supported by almost all of the loggers and there's not much up for discussion what to do with the figure is it?

I think figure logging is an important point to debug and monitor the performance of a model and should be well supported.

@awaelchli
Copy link
Contributor

I misunderstood, I didn't know you were only considering "matplotlib" figures.

@awaelchli awaelchli changed the title Abstract figure logging from individual logger API Abstract matplotlib figure logging from individual logger API Dec 7, 2020
@stale
Copy link

stale bot commented Jan 6, 2021

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!

@stale stale bot added the won't fix This will not be worked on label Jan 6, 2021
@awaelchli awaelchli removed the won't fix This will not be worked on label Jan 7, 2021
@Haydnspass
Copy link
Author

Can someone help me with MLFlow. How do you usually log figures over multiple epochs there?
My suggestion would be something like

filename = name + f"_step{step}.png"
figure.savefig(filename)
self.experiment.log_artifact(?)

@stale
Copy link

stale bot commented Feb 14, 2021

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!

@stale stale bot added the won't fix This will not be worked on label Feb 14, 2021
@stale stale bot closed this as completed Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement help wanted Open to be worked on logger Related to the Loggers won't fix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants