Abstract log_figure API that implements reasonable default for all lo… #6227

Haydnspass · 2021-02-26T16:01:48Z

Transfer of #4975
Sorry for the mess with choosing the wrong branch. It was faster for me to manually include the changes, so I am opening a new PR.

What does this PR do?

Make abstraction layer for figure logging, i.e. add default for logging figures for all implemented loggers.

My suggestion to #4973

ToDo:

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

…ggers Takes a plt.figure (along with some metadata) and logs it to the respective logger - CSV logger and testtube are in silent no-op - matplotlib becomes base requirement

pep8speaks · 2021-02-26T16:02:01Z

Hello @Haydnspass! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-18 11:17:14 UTC

…lity)

CHANGELOG.md

pytorch_lightning/loggers/base.py

pytorch_lightning/loggers/neptune.py

pytorch_lightning/loggers/base.py

awaelchli · 2021-02-27T14:26:40Z

pytorch_lightning/loggers/base.py

@@ -173,6 +174,20 @@ def log_metrics(self, metrics: Dict[str, float], step: Optional[int] = None):
        """
        pass

+    def log_figure(self, name: str, figure: plt.figure, step: Optional[int] = None, close: bool = True) -> None:


I think we should add additional **kwargs here.
When the user knows they are using a particular logger that supports additinal arguments, we should allow it to be passed down.

for example, in one of the loggers you have a description, and this could be customized then by the user.
This would then still be logger agnostic, where loggers simply ignore unknown **kwargs when it's not applicable.

Funnily I had it including **kwargs two times, and changed it back and forth ...

I do not like it because:

Silently ignoring arguments that are unexpected is not what python does by default when you parse **kwargs

It'll most certainly break for LoggerCollection unless one implements a more complex logic which then makes the whole implementation unnecessary

I also think **kwargs is really necessary here. Each logger can have many different arguments

Silently ignoring arguments that are unexpected is not what python does by default when you parse **kwargs

I don't get this

If you add **kwargs it will break LoggerCollection if you don't do anything. So what do you do if you have logger A that can take a certain argument that logger B in your collection does not? Ignore argument, raise?
Or add more complex logic that looks something like this

my_mwargs = { 'loggerA': {'kwarga': 1}, 'loggerB': {'kwargb': 2}, }

--> Needs design decision

requirements.txt

justusschock · 2021-02-27T19:03:55Z

pytorch_lightning/loggers/mlflow.py

+        figure.savefig(filename)
+        self.experiment.log_artifact(self.run_id, filename, artifact_path="figure_" + name)
+
+        Path(filename).unlink()  # delete temporary file


do we need to wait here before this was uploaded (upload may be async, not sure)?

Also if this is temporary, can we consider this to be saved in /tmp?

That's a good point, couldn't find anything in the docs of mlflow ...
I don't like this temporary thing anyways but could not come up with a better solution

Changed to temporary file

requirements.txt

docs/source/extensions/logging.rst

tchaton

Overall looks good ! Some comments to be addressed from @awaelchli @justusschock

pytorch_lightning/loggers/mlflow.py

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

tests/loggers/test_all.py

stale · 2021-06-04T15:29:54Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

Haydnspass · 2021-06-04T19:17:44Z

@carmocca @Borda what's the status of the review? The tests pass :)
Changelog.md is conflicting but it does not make sense to update it, cause it will always run behind master if you take a bit of time for the review.

pytorch_lightning/loggers/neptune.py

tchaton · 2021-06-14T10:30:54Z

pytorch_lightning/loggers/mlflow.py

+        # ToDo: Once its stable, use ml_flow.log_figure
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            figure_path = Path(tmp_dir) / figure_fname
+            figure.savefig(figure_path)


Did you check the figure is properly rendered ? From past experience, figure.savefig never worked for me.

Just changed the test for it to an actual functional one instead of mocking, and for me it works fine. The .png / .pdf is created as expected.
Locally it works as well. What happened in your case? The file was created but not filled?

It was filled, but savefig was adding borders and axis around the image.

…cking

Haydnspass · 2021-07-16T09:12:52Z

@tchaton @Borda how can we best move this forward? I figure there are a few small design decisions to me made by your team which can be found in the conversation above :)

justusschock · 2021-09-16T09:19:05Z

Hi @Haydnspass , I am sorry that the response was delayed so much. What do you think about enabling general media logging first (see #9545 for an example in wandb) for all loggers and then treat (matplotlib) figures just as a special case of images?

That does not mean, that we need to abandon this PR, but maybe you and @borisdayma can put together a draft for a more general logger API for media upon which we can then rebuild this PR?

Haydnspass · 2021-09-20T13:25:01Z

Hi @Haydnspass , I am sorry that the response was delayed so much. What do you think about enabling general media logging first (see #9545 for an example in wandb) for all loggers and then treat (matplotlib) figures just as a special case of images?

That does not mean, that we need to abandon this PR, but maybe you and @borisdayma can put together a draft for a more general logger API for media upon which we can then rebuild this PR?

@justusschock
I would argue to start with the log_figure thing cause I have the impression this is a nice little well-defined scope and not too different among the different loggers. Later on, one could try to generalise?

But of course, I don't have an emotional connection to this PR :D if something better/more general appears then this should be used.

The issue for me is, that I don't have much time to work on this, and its a bit unfortunate to run behind master all the time and spend the time on resolving conflicts :/

carmocca · 2022-03-28T15:30:38Z

We've chosen to move away from defining a set interface for all logger implementations as it becomes limiting and difficult when all their nuances and differences are considered.

Closing this PR. Thank you so much for your effort anyways @Haydnspass! 💜

cc @daniellepintz

Abstract log_figure API that implements reasonable default for all lo…

7b2d2d8

…ggers Takes a plt.figure (along with some metadata) and logs it to the respective logger - CSV logger and testtube are in silent no-op - matplotlib becomes base requirement

Fix missing line at end of file, rm default arg for unlink (compatibi…

6f8bd9a

…lity)

Haydnspass marked this pull request as ready for review February 26, 2021 18:00

Haydnspass requested review from awaelchli, Borda, carmocca, edenlightning, justusschock, SeanNaren, tchaton and williamFalcon as code owners February 26, 2021 18:00

mergify bot added the has conflicts label Feb 27, 2021

awaelchli reviewed Feb 27, 2021

View reviewed changes

justusschock reviewed Feb 27, 2021

View reviewed changes

SkafteNicki reviewed Mar 1, 2021

View reviewed changes

docs/source/extensions/logging.rst Outdated Show resolved Hide resolved

tchaton reviewed Mar 3, 2021

View reviewed changes

pytorch_lightning/loggers/mlflow.py Outdated Show resolved Hide resolved

Haydnspass and others added 7 commits March 12, 2021 10:32

Update CHANGELOG.md

95b4d4e

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Update pytorch_lightning/loggers/neptune.py

dcdfbee

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Update docs/source/extensions/logging.rst

eca36b2

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

use pathlib for path

b0b67d7

Make matplotlib extra

a77028a

Merge branch 'master' into feature/log_figure_abstract_clean

4424626

Make matplotlib local import

e11fc0c

mergify bot removed the has conflicts label Mar 12, 2021

Haydnspass commented Mar 12, 2021

View reviewed changes

tests/loggers/test_all.py Outdated Show resolved Hide resolved

mergify bot added the has conflicts label Mar 15, 2021

Haydnspass requested a review from justusschock March 25, 2021 08:48

Haydnspass added 2 commits March 26, 2021 12:13

Merge master

32f67c6

Change from manual to pythonic tempfile

6af3f1e

awaelchli added feature Is an improvement or enhancement logger Related to the Loggers labels May 21, 2021

Haydnspass added 2 commits May 21, 2021 12:22

Change log_figure saving logic for mlflow

82c6da9

Merge branch 'master' into feature/log_figure_abstract_clean

fc86940

Haydnspass marked this pull request as ready for review May 21, 2021 13:56

mergify bot added the has conflicts label May 21, 2021

stale bot added the won't fix This will not be worked on label Jun 4, 2021

stale bot removed the won't fix This will not be worked on label Jun 4, 2021

awaelchli added the design Includes a design discussion label Jun 14, 2021

awaelchli added this to the v1.4 milestone Jun 14, 2021

Merge branch 'master' into feature/log_figure_abstract_clean

ec64bf8

mergify bot removed the has conflicts label Jun 14, 2021

tchaton reviewed Jun 14, 2021

View reviewed changes

mergify bot added the has conflicts label Jun 17, 2021

Lucas-Raphael Mueller added 3 commits June 18, 2021 11:01

Change mlflow log_figure test to actual functional test instead of mo…

126c0ec

…cking

Move matplotlib available check to utilities.import

f972410

Rm unnecessary line

5684258

edenlightning modified the milestones: v1.4, v1.5 Jul 9, 2021

awaelchli modified the milestones: v1.5, v1.6 Nov 1, 2021

carmocca closed this Mar 28, 2022

timday23 mentioned this pull request Sep 21, 2023

Retrain with new images maxfrei750/paddle#7

Closed

robmarkcole mentioned this pull request Jun 30, 2024

Support logging figures to multiple loggers microsoft/torchgeo#2138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract log_figure API that implements reasonable default for all lo… #6227

Abstract log_figure API that implements reasonable default for all lo… #6227

Haydnspass commented Feb 26, 2021 •

edited

Loading

pep8speaks commented Feb 26, 2021 •

edited

Loading

awaelchli Feb 27, 2021

Haydnspass Feb 27, 2021

carmocca May 12, 2021

Haydnspass May 14, 2021

Haydnspass Jun 21, 2021

justusschock Feb 27, 2021

Haydnspass Mar 12, 2021

Haydnspass Mar 26, 2021

tchaton left a comment

stale bot commented Jun 4, 2021

Haydnspass commented Jun 4, 2021

tchaton Jun 14, 2021

Haydnspass Jun 18, 2021

tchaton Sep 10, 2021

Haydnspass commented Jul 16, 2021

justusschock commented Sep 16, 2021 •

edited

Loading

Haydnspass commented Sep 20, 2021 •

edited

Loading

carmocca commented Mar 28, 2022

Abstract log_figure API that implements reasonable default for all lo… #6227

Abstract log_figure API that implements reasonable default for all lo… #6227

Conversation

Haydnspass commented Feb 26, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Feb 26, 2021 • edited Loading

Comment last updated at 2021-06-18 11:17:14 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

stale bot commented Jun 4, 2021

Haydnspass commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haydnspass commented Jul 16, 2021

justusschock commented Sep 16, 2021 • edited Loading

Haydnspass commented Sep 20, 2021 • edited Loading

carmocca commented Mar 28, 2022

Haydnspass commented Feb 26, 2021 •

edited

Loading

pep8speaks commented Feb 26, 2021 •

edited

Loading

justusschock commented Sep 16, 2021 •

edited

Loading

Haydnspass commented Sep 20, 2021 •

edited

Loading