Add initial logger support #24

XapaJIaMnu · 2023-06-29T16:13:31Z

Will finish this after other PRs are in and then i'll also fix the tests:
Currently the output looks something like this:

[2023-06-29 17:09:41] [Trainer] [INFO] Starting stage start
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 0
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 1
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 2
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 3
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 4
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 5
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 6
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 7
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 8
[2023-06-29 17:09:41] [Trainer] [INFO] Reading clean for epoch 9

jelmervdl

There's a warn() from the warnings package somewhere in the codebase as well. Should we also replace that one with logging.warning()?

src/opustrainer/trainer.py

tests/test_logger.py

XapaJIaMnu · 2023-07-04T02:38:27Z

tests/test_endtoend.py

+            # Check if the log is the same as the reference log. This is more complicated as we have a time field
+            with open('contrib/test-data/test_enzh_config_plain_expected.log', 'r', encoding='utf-8') as reffile:
+                reference: List[str] = reffile.readlines()
+                # Loglist has one extra `\n` compared to reference list, due to stderr flushing an extra empty line?


As far as i can tell, this is harmless.

XapaJIaMnu · 2023-07-04T03:47:42Z

There's a warn() from the warnings package somewhere in the codebase as well. Should we also replace that one with logging.warning()?

Done. Ready to review now.

XapaJIaMnu · 2023-07-04T03:48:49Z

src/opustrainer/logger.py

+        handlers.append(logging.StreamHandler(stream=stderr))
+    if outputfilename is not None:
+        handlers.append(logging.FileHandler(filename=outputfilename))
+    # This is the only logger we'd ever use. However during testing, due to the context, logger can't be recreated,


Not entirely happy about this, but the alternative is to create different loggers which I don't want to do.

XapaJIaMnu · 2023-07-04T03:49:01Z

src/opustrainer/logger.py

+    """Sets up the logger with the necessary settings. Outputs to both file and stderr"""
+    loggingformat = '[%(asctime)s] [Trainer] [%(levelname)s] %(message)s'
+    handlers: List[logging.StreamHandler[TextIO] | logging.StreamHandler[TextIOWrapper]] = []
+    # disable_stderr is to be used only when testing the logger


grumpy noises

Why do you want strings for loglevel? Why not just export the log levels we support and reference logger.WARNING instead of "WARNING" in code? Additional upside is that you can't reference log levels that don't exist because the symbols to do so are not there.

Why StreamHandler[TextIO]? I can't find any reference of StreamHandler inheriting Generic.

Btw If you're okay with it I want to go through your changes and make them Python 3.8 compatible (which basically means not using generics where they're not supported, and using Union instead of |)

My type checker flagged it as an incompatible type assignment and this was the inferred type. I guess UNION should be used instead.

For the WARNING I thought it'd be easier to use a text based log level from modifiers, in case users want to add their own logging, rather than getting to python internals.

That being said, logging.getLevelNamesMapping() is only available in python 3.11 so I shouldn't be using it.

Also, since we define the level at runtime, I had to have str -> loglevel conversion anyways..

jelmervdl

There's a line print(f"[Trainer] Empty field in {self.dataset.name} line:\"{line}\", skipping...") on trainer.py:212 that should probably also be a warning to the logger.

jelmervdl · 2023-07-04T09:58:03Z

src/opustrainer/logger.py

+    """Sets up the logger with the necessary settings. Outputs to both file and stderr"""
+    loggingformat = '[%(asctime)s] [Trainer] [%(levelname)s] %(message)s'
+    handlers: List[logging.StreamHandler[TextIO] | logging.StreamHandler[TextIOWrapper]] = []
+    # disable_stderr is to be used only when testing the logger


Why do you want strings for loglevel? Why not just export the log levels we support and reference logger.WARNING instead of "WARNING" in code? Additional upside is that you can't reference log levels that don't exist because the symbols to do so are not there.

jelmervdl · 2023-07-04T10:04:08Z

src/opustrainer/logger.py

+    """Sets up the logger with the necessary settings. Outputs to both file and stderr"""
+    loggingformat = '[%(asctime)s] [Trainer] [%(levelname)s] %(message)s'
+    handlers: List[logging.StreamHandler[TextIO] | logging.StreamHandler[TextIOWrapper]] = []
+    # disable_stderr is to be used only when testing the logger


Why StreamHandler[TextIO]? I can't find any reference of StreamHandler inheriting Generic.

Btw If you're okay with it I want to go through your changes and make them Python 3.8 compatible (which basically means not using generics where they're not supported, and using Union instead of |)

XapaJIaMnu · 2023-07-04T11:48:50Z

Go ahead and do your changes, I think i'm good with everything. Runs on CSD3

jelmervdl · 2023-07-04T11:55:18Z

Looks like 3.8 isn't complaining about the weird generics your type checker is introducing (I checked the source, they don't make any sense to me, but okay). Fixed an encoding argument that's only available since 3.9. But now tests are okay on macOS/python3.8.

LGTM! 🥳

Add initial logger support

b6a2a01

XapaJIaMnu requested a review from jelmervdl June 29, 2023 16:13

jelmervdl reviewed Jun 30, 2023

View reviewed changes

src/opustrainer/trainer.py Show resolved Hide resolved

src/opustrainer/trainer.py Show resolved Hide resolved

src/opustrainer/trainer.py Show resolved Hide resolved

XapaJIaMnu added 5 commits June 30, 2023 16:06

Always log to stderr

4da4712

Merge branch 'main' into logger

22e49e0

Fix existing tests

5bf35a7

Make typing happy

f994a74

Log tests

e08d605

XapaJIaMnu commented Jul 4, 2023

View reviewed changes

tests/test_logger.py Outdated Show resolved Hide resolved

XapaJIaMnu added 2 commits July 4, 2023 02:54

Extra comment in test

c80581e

Move things around and test log_once functionality

451a22e

XapaJIaMnu commented Jul 4, 2023

View reviewed changes

XapaJIaMnu marked this pull request as ready for review July 4, 2023 02:38

Make tests more proper and remove warnings package usage

04937ee

XapaJIaMnu commented Jul 4, 2023

View reviewed changes

Test the log level as well

c45fdc8

jelmervdl reviewed Jul 4, 2023

View reviewed changes

Make it workable on older python

80b3252

Backport to 3.8

b9c8545

Small formatting fix

6d0ff04

XapaJIaMnu merged commit f695e04 into main Jul 4, 2023

XapaJIaMnu deleted the logger branch July 4, 2023 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial logger support #24

Add initial logger support #24

XapaJIaMnu commented Jun 29, 2023

jelmervdl left a comment

XapaJIaMnu Jul 4, 2023

XapaJIaMnu commented Jul 4, 2023

XapaJIaMnu Jul 4, 2023

XapaJIaMnu Jul 4, 2023

jelmervdl Jul 4, 2023

jelmervdl Jul 4, 2023

XapaJIaMnu Jul 4, 2023

XapaJIaMnu Jul 4, 2023

XapaJIaMnu Jul 4, 2023

jelmervdl left a comment

jelmervdl Jul 4, 2023

jelmervdl Jul 4, 2023

XapaJIaMnu commented Jul 4, 2023

jelmervdl commented Jul 4, 2023

Add initial logger support #24

Add initial logger support #24

Conversation

XapaJIaMnu commented Jun 29, 2023

jelmervdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XapaJIaMnu commented Jul 4, 2023

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

jelmervdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XapaJIaMnu commented Jul 4, 2023

jelmervdl commented Jul 4, 2023