Added basic file logger #2721

motlikm · 2020-07-27T08:26:34Z

What does this PR do?

This PR implements basic file logger as discussed in #1803

Fixes #1803

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?

justusschock

Already looks pretty good. The comments for docstrings and typing apply to all functions and classes :)

pytorch_lightning/loggers/file_logger.py

ethanwharris

This looks really nice 😃 Agree that this should be ~~changed~~ renamed to CSVLogger. It would also be good to add the save_dir property to the logger so that checkpoints are saved in the right place (this may just mean renaming the root_dir property?)

Borda · 2020-07-28T09:24:57Z

This looks really nice Agree that this should be changed to CSVLogger. It would also be good to add the save_dir property to the logger so that checkpoints are saved in the right place (this may just mean renaming the root_dir property?)

I am not very convinced about CSV format for logging, then you would have plant hard time with any separator you use...

ethanwharris · 2020-07-28T09:35:10Z

This looks really nice Agree that this should be changed to CSVLogger. It would also be good to add the save_dir property to the logger so that checkpoints are saved in the right place (this may just mean renaming the root_dir property?)

I am not very convinced about CSV format for logging, then you would have plant hard time with any separator you use...

@Borda Whoops, should have said 'should be renamed to CSVLogger' since that's what it already does (but called FileLogger currently). Presumably if people want more options (e.g. different delimiters, JSON logging, etc.) then these can be added later on?

codecov · 2020-07-30T08:02:22Z

Codecov Report

Merging #2721 into master will decrease coverage by 4%.
The diff coverage is 98%.

@@           Coverage Diff           @@
##           master   #2721    +/-   ##
=======================================
- Coverage      90%     86%    -4%     
=======================================
  Files          78      79     +1     
  Lines        7055    7161   +106     
=======================================
- Hits         6362    6177   -185     
- Misses        693     984   +291

motlikm · 2020-07-30T08:42:22Z

This looks really nice 😃 Agree that this should be ~~changed~~ renamed to CSVLogger. It would also be good to add the save_dir property to the logger so that checkpoints are saved in the right place (this may just mean renaming the root_dir property?)

@ethanwharris Added save_dir property. Now it should be consistent with the TensorboardLogger implementation.

justusschock

LGTM :)

Borda

This looks very similar to Test-Tube logger, mind clarify what is the difference?

motlikm · 2020-07-30T09:58:58Z

This looks very similar to Test-Tube logger, mind clarify what is the difference?

@Borda Yes, it's true it was inspired by the Test-Tube logger as suggested in this comment. The difference is that this logger has no 3rd party deps (pandas).

Borda · 2020-07-30T10:16:35Z

well, would it be easier to make test-tube lighter? @williamFalcon

ethanwharris

LGTM - confirmed working locally 😃

mergify · 2020-08-05T17:31:14Z

This pull request is now in conflict... :(

CHANGELOG.md

pytorch_lightning/loggers/csv_logs.py

Borda · 2020-08-05T20:42:05Z

tests/loggers/test_all.py

@@ -226,6 +232,7 @@ def on_batch_start(self, trainer, pl_module):
 @pytest.mark.skipif(platform.system() == "Windows", reason="Distributed training is not supported on Windows")
 @pytest.mark.parametrize("logger_class", [
    TensorBoardLogger,
+    # CSVLogger,  # todo


any idea why this test is failing?
cc: @PyTorchLightning/core-contributors

Hecim1984

💪🏻🚀

mergify bot requested a review from a team July 27, 2020 08:27

motlikm force-pushed the 1803_file_logger branch from 54e57aa to 35c235d Compare July 27, 2020 08:36

motlikm marked this pull request as ready for review July 27, 2020 08:39

motlikm force-pushed the 1803_file_logger branch from 35c235d to f0d37de Compare July 27, 2020 08:42

Borda added the feature Is an improvement or enhancement label Jul 27, 2020

justusschock requested changes Jul 27, 2020

View reviewed changes

mergify bot requested a review from a team July 27, 2020 13:39

williamFalcon added the allowed_pre_1.0 label Jul 27, 2020

ethanwharris reviewed Jul 28, 2020

View reviewed changes

mergify bot requested a review from a team July 28, 2020 09:20

motlikm changed the title ~~Added basic file logger #1803~~ WIP: Added basic file logger #1803 Jul 30, 2020

motlikm changed the title ~~WIP: Added basic file logger #1803~~ Added basic file logger #1803 Jul 30, 2020

motlikm requested review from justusschock and removed request for a team July 30, 2020 08:46

mergify bot requested a review from a team July 30, 2020 08:46

justusschock approved these changes Jul 30, 2020

View reviewed changes

justusschock requested review from ethanwharris and Borda July 30, 2020 08:55

mergify bot requested a review from a team July 30, 2020 08:55

Borda requested changes Jul 30, 2020

View reviewed changes

mergify bot requested a review from a team July 30, 2020 09:29

ethanwharris approved these changes Jul 30, 2020

View reviewed changes

mergify bot requested a review from a team July 30, 2020 10:21

Borda changed the title ~~Added basic file logger #1803~~ Added basic file logger Jul 30, 2020

Borda force-pushed the 1803_file_logger branch from d8f8371 to 77141d4 Compare August 5, 2020 16:11

Borda changed the title ~~[WIP] Added basic file logger~~ Added basic file logger Aug 5, 2020

Borda reviewed Aug 5, 2020

View reviewed changes

mergify bot requested a review from a team August 5, 2020 19:18

Borda approved these changes Aug 5, 2020

View reviewed changes

Borda force-pushed the 1803_file_logger branch from 259af6a to e242290 Compare August 5, 2020 19:43

Borda reviewed Aug 5, 2020

View reviewed changes

Borda force-pushed the 1803_file_logger branch from 0da2d46 to ee31215 Compare August 5, 2020 23:18

Borda added the ready PRs ready to be merged label Aug 6, 2020

xmotli02 and others added 13 commits August 6, 2020 10:33

Added basic file logger Lightning-AI#1803

86d4fc2

fixup! Added basic file logger Lightning-AI#1803

ef9619a

fixup! Added basic file logger Lightning-AI#1803

06f493c

fixup! Added basic file logger Lightning-AI#1803

5ae2af2

fixup! Added basic file logger Lightning-AI#1803

bc2f233

fixup! Added basic file logger Lightning-AI#1803

018baeb

csv

e7bb792

Apply suggestions from code review

c2c4b7d

tests

051f453

tests

dafdb74

tests

982cce3

miss

f03a7f7

docs

6a85720

Borda force-pushed the 1803_file_logger branch from ee31215 to 6a85720 Compare August 6, 2020 08:33

Borda requested review from jeremyjordan, SkafteNicki and williamFalcon August 6, 2020 09:16

williamFalcon merged commit 767c449 into Lightning-AI:master Aug 6, 2020

Hecim1984 reviewed Oct 20, 2021

View reviewed changes

mergify bot removed the ready PRs ready to be merged label Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added basic file logger #2721

Added basic file logger #2721

motlikm commented Jul 27, 2020 •

edited

Loading

justusschock left a comment

ethanwharris left a comment •

edited

Loading

Borda commented Jul 28, 2020

ethanwharris commented Jul 28, 2020

codecov bot commented Jul 30, 2020 •

edited

Loading

motlikm commented Jul 30, 2020 •

edited

Loading

justusschock left a comment

Borda left a comment

motlikm commented Jul 30, 2020

Borda commented Jul 30, 2020

ethanwharris left a comment

mergify bot commented Aug 5, 2020

Borda Aug 5, 2020

Hecim1984 left a comment

Added basic file logger #2721

Added basic file logger #2721

Conversation

motlikm commented Jul 27, 2020 • edited Loading

What does this PR do?

Before submitting

justusschock left a comment

Choose a reason for hiding this comment

ethanwharris left a comment • edited Loading

Choose a reason for hiding this comment

Borda commented Jul 28, 2020

ethanwharris commented Jul 28, 2020

codecov bot commented Jul 30, 2020 • edited Loading

Codecov Report

motlikm commented Jul 30, 2020 • edited Loading

justusschock left a comment

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

motlikm commented Jul 30, 2020

Borda commented Jul 30, 2020

ethanwharris left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 5, 2020

Borda Aug 5, 2020

Choose a reason for hiding this comment

Hecim1984 left a comment

Choose a reason for hiding this comment

motlikm commented Jul 27, 2020 •

edited

Loading

ethanwharris left a comment •

edited

Loading

codecov bot commented Jul 30, 2020 •

edited

Loading

motlikm commented Jul 30, 2020 •

edited

Loading