Ensure `logging.yml` is optional and tidy logging config #1543

antonymilne · 2022-05-16T16:29:04Z

Description

Closes #1469.

In short, you no longer need to supply a project-side logging.yml file.

Sorry it's a bit of a huge PR to review, but the changes here are actually quite small. I've left lots of comments in the files that hopefully make it easier! Almost all the files changed are just updating tests. These are much simpler now because we don't have to provide a mock logging.yml file for them to run 🎉

Equivalent changes to starters templates are in kedro-org/kedro-starters#90.

Testing

Tested with and without a logging.yml the following commands:

kedro run
kedro run --runner=ParallelRunner
kedro info
kedro ipython
kedro jupyter lab
kedro run with an exception thrown in the pipeline
kedro package and then run packaged project

All work as expected, but it turns out there's actually always been a problem with the parallel runner not using the framework-side logging config. This problem is now more prominent because it means that if you don't have a project-side logging.yml the logging will disappear completely (because there's effectively no logging config at all in the runner any more). I'm going to make a separate issue to fix this as part of the point we already have in #1461 to try and improve the parallel runner's logging handling.

TODO

Update release notes
Make parallel runner logging ticket

Checklist

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the RELEASE.md file
Added tests to cover my changes

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

…to feature/optional-logging-yml Signed-off-by: Antony Milne <antony.milne@quantumblack.com> # Conflicts: # features/steps/test_starter/{{ cookiecutter.repo_name }}/conf/base/logging.yml # kedro/templates/project/{{ cookiecutter.repo_name }}/conf/base/logging.yml

antonymilne · 2022-05-16T16:30:53Z

kedro/config/logging.yml

@@ -1,44 +1,21 @@
 version: 1


Most of these changes are just formatting. What's actually changed here is:

removed info_file_handler and error_file_handler. These are now only defined on the project-side logging config

removed special case kedro.framework.cli.hooks.manager logger (which was the same as the default, just without the file-based handlers enabled. Now there's no file-based handlers it's exactly the same as the default, so no need to define it separately)

antonymilne · 2022-05-16T16:31:46Z

features/steps/test_starter/{{ cookiecutter.repo_name }}/conf/base/logging.yml

@@ -1,55 +1,44 @@
 version: 1


Changes here are the same as to the template file - see explanation there.

antonymilne · 2022-05-16T16:36:11Z

kedro/templates/project/{{ cookiecutter.repo_name }}/conf/base/logging.yml

@@ -1,55 +1,44 @@
 version: 1


Most of these changes are just formatting. What's actually changed here is:

removed json_formatter which was never used anywhere

removed the special case kedro.x loggers in favour of the generic kedro one, which did the same thing anyway

altered root logger so it no longer shows all INFO level messages (it's now the same as the framework-side logging config and doesn't specify this explicitly, so defaults to Python's WARNING level)

added a special logger for {{ cookiecutter.python_package }}. This is to ensure that calls like this still emit logs

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne · 2022-05-18T23:34:41Z

tests/framework/cli/test_cli.py

+class TestDefaultLogging:
+    def test_setup_root_logger(self):
+        root_logger = logging.getLogger()
+        assert "console" in {handler.name for handler in root_logger.handlers}
+
+    def test_setup_kedro_logger(self):
+        kedro_logger = logging.getLogger("kedro")
+        assert kedro_logger.level == logging.INFO


These are updated versions of tests that used to be in test_cli_logging_setup.py (tests were in the wrong place before, now that file is deleted).

antonymilne · 2022-05-18T23:35:26Z

tests/framework/session/test_session.py

+    def test_no_logging_config(self, fake_project, caplog, mock_package_name, mocker):
+        caplog.set_level(logging.DEBUG, logger="kedro")
+
+        mocker.patch("subprocess.check_output")
+        session = KedroSession.create(mock_package_name, fake_project)
+        session.close()
+
+        expected_log_messages = [
+            "No project logging configuration loaded; "
+            "Kedro's default logging configuration will be used."
+        ]
+        actual_log_messages = [
+            rec.getMessage()
+            for rec in caplog.records
+            if rec.name == SESSION_LOGGER_NAME and rec.levelno == logging.DEBUG
+        ]
+        assert actual_log_messages == expected_log_messages
+


This is the only vaguely interesting new test.

This test is specifically checking that we get that debug message, but the other tests aren't actually using logging config either right? So in theory they would all show that message in debug mode?

Yes indeed! Though some of the other tests have mocker.patch("kedro.framework.session.KedroSession._setup_logging") so the debug message won't be emitted there anyway.

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

This reverts commit 9892d8e.

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne · 2022-05-19T09:06:54Z

features/steps/util.py

 from contextlib import contextmanager
 from pathlib import Path
 from time import sleep, time
 from typing import Any, Callable, Iterator, List

-import pandas as pd


Unrelated to the logging changes. Just removing some unused code from e2e tests while I spotted it.

antonymilne · 2022-05-19T09:07:23Z

kedro/framework/session/session.py

@@ -185,8 +185,15 @@ def _get_logging_config(self) -> Dict[str, Any]:

    def _setup_logging(self) -> None:
        """Register logging specified in logging directory."""
-        conf_logging = self._get_logging_config()
-        configure_logging(conf_logging)
+        try:


This is the key change: we now allow there to be no logging.yml on the project-side.

antonymilne · 2022-05-19T09:08:30Z

tests/test_cli_logging_setup.py

@@ -1,19 +0,0 @@
-"""This module contains unit tests for methods in the Kedro __init__.py


These were in the wrong place and are now (in updated form) in test_cli.py.

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

AhdraMeraliQB

Nicely done! Great work on the description too, super helpful 🌟

merelcht

Great work @AntonyMilneQB ! 🤩 This has cleaned up the logging so much!

One minor question around testing, do we have any tests now that check stuff works correctly when the user has a custom project level logging.yml? I couldn't completely figure it out from the changes made to the tests.

merelcht · 2022-05-19T10:29:47Z

kedro/templates/project/{{ cookiecutter.repo_name }}/conf/base/logging.yml

+  kedro:
+    level: INFO


Just so I understand correctly, what handlers is this logger now using? The ones specified for root?

Yes, exactly! There's no handlers explicitly specified here, so it will propagate to root and use those.

merelcht · 2022-05-19T10:32:46Z

tests/framework/session/test_session.py

+    def test_no_logging_config(self, fake_project, caplog, mock_package_name, mocker):
+        caplog.set_level(logging.DEBUG, logger="kedro")
+
+        mocker.patch("subprocess.check_output")
+        session = KedroSession.create(mock_package_name, fake_project)
+        session.close()
+
+        expected_log_messages = [
+            "No project logging configuration loaded; "
+            "Kedro's default logging configuration will be used."
+        ]
+        actual_log_messages = [
+            rec.getMessage()
+            for rec in caplog.records
+            if rec.name == SESSION_LOGGER_NAME and rec.levelno == logging.DEBUG
+        ]
+        assert actual_log_messages == expected_log_messages
+


This test is specifically checking that we get that debug message, but the other tests aren't actually using logging config either right? So in theory they would all show that message in debug mode?

antonymilne · 2022-05-19T11:03:20Z

Great work @AntonyMilneQB ! 🤩 This has cleaned up the logging so much!

One minor question around testing, do we have any tests now that check stuff works correctly when the user has a custom project level logging.yml? I couldn't completely figure it out from the changes made to the tests.

Thank you! This is covered by test_setup_logging_using_absolute_path and also the e2e tests but it's not very explicit at the moment. I will change this so it's better.

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne added 7 commits May 16, 2022 16:07

Simplify framework logging.yml

35b9267

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Format framework logging.yml

f409702

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Update project template and test starter logging.yml

fa3dec7

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Make logging.yml optional

78650f6

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Simplify project logging.yml since disable_existing_loggers: False

b7234d5

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Make sure project namespace logs at level INFO

3a71a79

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne commented May 16, 2022

View reviewed changes

antonymilne and others added 5 commits May 18, 2022 21:38

Merge branch 'main' into feature/optional-logging-yml

939bb25

Update tests

583e5a5

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Simplify lots of tests

6d6a57a

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Final test fix

477bbbd

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Final test tidy

9839786

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne commented May 18, 2022

View reviewed changes

antonymilne added 5 commits May 19, 2022 00:56

Fix e2e tests and lint

9891d73

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Swap out caplog for neater solution

9892d8e

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Fix parallel runner to work with no logging.yml

2da60ca

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Revert "Swap out caplog for neater solution"

0301e12

This reverts commit 9892d8e.

Lint

0c37e45

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne commented May 19, 2022

View reviewed changes

Fix test

74889ac

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne marked this pull request as ready for review May 19, 2022 09:26

antonymilne requested a review from idanov as a code owner May 19, 2022 09:26

antonymilne requested review from merelcht, AhdraMeraliQB, SajidAlamQB and noklam May 19, 2022 09:26

antonymilne mentioned this pull request May 19, 2022

Update logging.yml kedro-org/kedro-starters#90

Merged

3 tasks

antonymilne self-assigned this May 19, 2022

AhdraMeraliQB approved these changes May 19, 2022

View reviewed changes

merelcht approved these changes May 19, 2022

View reviewed changes

antonymilne and others added 3 commits May 19, 2022 12:17

Make e2e tests check files too

2e6337d

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Argh

ef50bd8

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Merge branch 'main' into feature/optional-logging-yml

c3551f3

antonymilne merged commit 3560063 into main May 20, 2022

antonymilne deleted the feature/optional-logging-yml branch May 20, 2022 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `logging.yml` is optional and tidy logging config #1543

Ensure `logging.yml` is optional and tidy logging config #1543

antonymilne commented May 16, 2022 •

edited

Loading

antonymilne May 16, 2022 •

edited

Loading

antonymilne May 16, 2022 •

edited

Loading

antonymilne May 16, 2022

antonymilne May 18, 2022

antonymilne May 18, 2022 •

edited

Loading

merelcht May 19, 2022

antonymilne May 19, 2022

antonymilne May 19, 2022

antonymilne May 19, 2022

antonymilne May 19, 2022

AhdraMeraliQB left a comment

merelcht left a comment

merelcht May 19, 2022

antonymilne May 19, 2022

merelcht May 19, 2022

antonymilne commented May 19, 2022

		@@ -1,19 +0,0 @@
		"""This module contains unit tests for methods in the Kedro __init__.py

Ensure logging.yml is optional and tidy logging config #1543

Ensure logging.yml is optional and tidy logging config #1543

Conversation

antonymilne commented May 16, 2022 • edited Loading

Description

Testing

TODO

Checklist

antonymilne May 16, 2022 • edited Loading

Choose a reason for hiding this comment

antonymilne May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonymilne May 18, 2022 • edited Loading

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

AhdraMeraliQB left a comment

Choose a reason for hiding this comment

merelcht left a 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

antonymilne commented May 19, 2022

Ensure `logging.yml` is optional and tidy logging config #1543

Ensure `logging.yml` is optional and tidy logging config #1543

antonymilne commented May 16, 2022 •

edited

Loading

antonymilne May 16, 2022 •

edited

Loading

antonymilne May 16, 2022 •

edited

Loading

antonymilne May 18, 2022 •

edited

Loading