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

Add the Kedro RichHandler #2592

Merged
merged 45 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
db214c2
Add the Kedro RichHandler
noklam May 19, 2023
d817420
Merge branch 'main' into 2277-introduce-our-own-rich-console-logging-…
noklam May 19, 2023
6a31eff
Clean up imports and comments
noklam May 23, 2023
2b02951
Map arguments to add flexibility to customize arguments available in …
noklam May 23, 2023
df7308e
Clean up rich_logger.py and unused imports
noklam May 23, 2023
c507984
Add docs - first iteration
noklam May 23, 2023
3ba89c9
Fix imports and small refactoring
noklam May 23, 2023
0fb70c3
Improve the doc
noklam May 23, 2023
2dbc0c0
Update the project's template
noklam May 23, 2023
05eaa91
format docstring
noklam May 23, 2023
7b9a989
Apply Jo's comment on the doc
noklam May 23, 2023
8e531f0
Fix the link with RichHandler
noklam May 23, 2023
810d25b
Apply suggestions from code review
noklam May 24, 2023
fc0d519
Merge branch 'main' into 2277-introduce-our-own-rich-console-logging-…
noklam May 24, 2023
1e5d14a
Fix broken doc
noklam May 24, 2023
85ff28d
update template defaults
noklam May 24, 2023
1b09078
refactoring RichHandler and improve doc
noklam May 24, 2023
1fffdc0
Merge branch '2277-introduce-our-own-rich-console-logging-handler' of…
noklam May 24, 2023
f9dffc2
Fix broken test due to new default RichHandler
noklam May 24, 2023
f5be2d4
Fix doc
noklam May 24, 2023
044ccda
Move kedro.extras.logging.RichHandler -> kedro.logging.RichHandler
noklam May 24, 2023
23f6427
add kedro/logging.py and remove the unnecessary import that cause nam…
noklam May 24, 2023
04571ee
Refactor test
noklam May 24, 2023
ea23b38
Add more tests
noklam May 24, 2023
8894aa5
Fix lots of import, docstring
noklam May 24, 2023
5efefd8
Reorder docstring
noklam May 24, 2023
9d65959
Fix tests
noklam May 24, 2023
8b4f63a
Link options to doc
noklam May 25, 2023
0c166ad
Clean up doc and unused logger
noklam May 25, 2023
b81a5ea
Fix spelling `customise`
noklam May 25, 2023
8f2788a
Refactor test with fixture
noklam May 25, 2023
f263931
Remove unnecessary copy() because it is now a fixture
noklam May 25, 2023
5071f5a
Split up test_rich_traceback_configuration() to smaller tests
noklam May 25, 2023
f9d5769
Add RELEASE.md
noklam May 25, 2023
df58139
Update doc
noklam May 25, 2023
af4a9ce
Fix template logging
noklam May 25, 2023
d123a3e
Fix the. template
noklam May 26, 2023
6ad4463
Lint
noklam May 26, 2023
632d29e
Merge branch 'main' into 2277-introduce-our-own-rich-console-logging-…
noklam May 26, 2023
b883906
Add doc to fix doc issues
noklam May 30, 2023
e749dca
fix doc
noklam May 30, 2023
c726d6e
Fix doc rst so it don't reference to the upstream class
noklam May 30, 2023
af3e3d0
Remvoe whitespace
noklam May 30, 2023
40e9156
remove undoc member and inherited-member for RichHandler
noklam May 30, 2023
b7d94e6
Please the linter once again
noklam May 30, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion docs/source/logging/logging.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ We now give some common examples of how you might like to change your project's

### Using `KEDRO_LOGGING_CONFIG` environment variable

`KEDRO_LOGGING_CONFIG` is an optional environment variable that you can use to specify the path of your logging configuration file, overriding the default path of `conf/base/logging.yml`.
`KEDRO_LOGGING_CONFIG` is an optional environment variable that you can use to specify the path of your logging configuration file, overriding the default Kedro's `default_logging.yml`.

To use this environment variable, set it to the path of your desired logging configuration file before running any Kedro commands. For example, if you have a logging configuration file located at `/path/to/logging.yml`, you can set `KEDRO_LOGGING_CONFIG` as follows:

Expand All @@ -43,6 +43,25 @@ Alternatively, if you would like to keep other configuration in `conf/base/loggi
+ handlers: [console]
```

### Customize the `rich` Handler

Kedro's `kedro.extras.logging.RichHandler` is a subclass of [`rich.logging.RichHand[label](vscode-file://vscode-a section of the class.

pp/Applications/Visual%20Studio%20Code.app/Contents/Resources/app/out/vs/code/electron-sandbox/workbench/workbench.html)ler`](https://rich.readthedocs.io/en/stable/reference/logging.html#rich.logging.RichHandler) and supports the same set of arguments. By default, `rich_tracebacks` is set to `True` to use `rich` to render exceptions. However, you can disable it by setting `rich_tracebacks: False`.

When `rich_tracebacks` is set to `True`, the configuration is propagated to [`rich.traceback.install`](https://rich.readthedocs.io/en/stable/reference/traceback.html#rich.traceback.install). If an argument is compatible with `rich.traceback.install`, it will be passed to the traceback's settings.

For instance, you can enable the display of local variables inside `logging.yml` to aid with debugging.

```yaml
rich:
class: kedro.extras.logging.RichHandler
rich_tracebacks: True
tracebacks_show_locals: True
```

A comprehensive list of available options can be found in the [RichHandler documentation](https://rich.readthedocs.io/en/stable/reference/logging.html#rich.logging.RichHandler).

### Use plain console logging

To use plain rather than rich logging, swap the `rich` handler for the `console` one as follows:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ handlers:
delay: True

rich:
class: rich.logging.RichHandler
class: kedro.logging.RichHandler
rich_tracebacks: True
# tracebacks_show_locals: False
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like settings.py, we could have some commented default here to help user discover possible options. I put show_locals because initially when we introduce rich, we want to enable it as True. It turns out to be a bit noisy for some users, and there was no good way to turn it off by the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also like the idea, but remember this file won't exist in the default template in 0.19 and it will move to docs instead - see #2426 (this was a relatively recent decision in tech design: #2281 (comment)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note you changed the test_starter here, not the actual kedro template starter.


loggers:
kedro:
Expand Down
5 changes: 0 additions & 5 deletions kedro/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,3 @@
"""

__version__ = "0.18.8"


import logging

logging.getLogger(__name__).addHandler(logging.NullHandler())
Comment on lines -7 to -11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates name collision with kedro.logging, I don't think this is needed at all. It was there in the first commit in 0.14

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have considered doing this many times but wasn't brave enough to do so in case it broke something, because I changed something small with logging before and it broke things unexpectedly...

Happy to have it removed but please do a good manual test! I did so yesterday before you removed these lines and everything worked ok, but would be great to double check now 🤞

16 changes: 0 additions & 16 deletions kedro/framework/project/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import logging.config
import operator
import os
import sys
import traceback
import types
import warnings
Expand All @@ -16,10 +15,7 @@
from pathlib import Path
from typing import Any

import click
import importlib_resources
import rich.pretty
import rich.traceback
import yaml
from dynaconf import LazySettings
from dynaconf.validator import ValidationError, Validator
Expand Down Expand Up @@ -221,18 +217,6 @@ def __init__(self):
)
logging_config = Path(path).read_text(encoding="utf-8")
self.configure(yaml.safe_load(logging_config))
logging.captureWarnings(True)

# We suppress click here to hide tracebacks related to it conversely,
# kedro is not suppressed to show its tracebacks for easier debugging.
# sys.executable is used to get the kedro executable path to hide the
# top level traceback.
# Rich traceback handling does not work on databricks. Hopefully this will be
# fixed on their side at some point, but until then we disable it.
# See https://github.com/Textualize/rich/issues/2455
if "DATABRICKS_RUNTIME_VERSION" not in os.environ:
rich.traceback.install(suppress=[click, str(Path(sys.executable).parent)])
rich.pretty.install()
Comment on lines -224 to -235
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rich's traceback won't be installed when kedro.framework.project is imported, it's now just a logging handler. This is an improvement because importing a module no longer has side-effect.


def configure(self, logging_config: dict[str, Any]) -> None:
"""Configure project logging using ``logging_config`` (e.g. from project
Expand Down
4 changes: 3 additions & 1 deletion kedro/framework/project/default_logging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ disable_existing_loggers: False

handlers:
rich:
class: rich.logging.RichHandler
class: kedro.logging.RichHandler
rich_tracebacks: True
# tracebacks_show_locals: False

loggers:
kedro:
Expand Down
60 changes: 60 additions & 0 deletions kedro/logging.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
"""
This module contains a logging handler class which produces coloured logs and tracebacks.
"""

import logging
import os
import sys
from pathlib import Path

import click
import rich.logging
import rich.pretty
import rich.traceback

logger = logging.getLogger(__file__)


class RichHandler(rich.logging.RichHandler):
"""Identical to rich's logging handler but with a few extra behaviours:
* warnings issued by the `warnings` module are redirected to logging
* pretty printing is enabled on the Python REPL (including IPython and Jupyter)
* all tracebacks are handled by rich when rich_tracebacks=True
* constructor's arguments are mapped and pass to `rich.traceback.install`

The list of available options of ``RichHandler`` can be found here:
https://rich.readthedocs.io/en/stable/reference/logging.html#rich.logging.RichHandler

The list of available options of `rich.traceback.install` can be found here:
https://rich.readthedocs.io/en/stable/reference/traceback.html#rich.traceback.install
"""

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
logging.captureWarnings(True)
rich.pretty.install()

# We suppress click here to hide tracebacks related to it conversely,
# kedro is not suppressed to show its tracebacks for easier debugging.
# sys.executable is used to get the kedro executable path to hide the
# top level traceback.

traceback_install_kwargs = {
"suppress": [click, str(Path(sys.executable).parent)]
}

# Mapping Arguments from RichHandler's Constructor to rich.traceback.install
prefix = "tracebacks_"
for key, value in kwargs.items():
if key.startswith(prefix):
key_prefix_removed = key[len(prefix) :]
if key_prefix_removed == "suppress":
traceback_install_kwargs[key_prefix_removed].extend(value)
else:
traceback_install_kwargs[key_prefix_removed] = value

if self.rich_tracebacks and "DATABRICKS_RUNTIME_VERSION" not in os.environ:
# Rich traceback handling does not work on databricks. Hopefully this will be
# fixed on their side at some point, but until then we disable it.
# See https://github.com/Textualize/rich/issues/2455
rich.traceback.install(**traceback_install_kwargs)
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ handlers:
delay: True

rich:
class: rich.logging.RichHandler
class: kedro.logging.RichHandler
rich_tracebacks: True
# tracebacks_show_locals: True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to put in a link to the docs to explain what this commented out line is for. Otherwise it's confusing since a user doesn't know whether they should uncomment it or not.


loggers:
kedro:
Expand Down
66 changes: 55 additions & 11 deletions tests/framework/project/test_logging.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# pylint: disable=import-outside-toplevel,unused-import,reimported
# pylint: disable=import-outside-toplevel
import logging
import sys
from pathlib import Path
Expand All @@ -11,7 +11,9 @@
default_logging_config = {
"version": 1,
"disable_existing_loggers": False,
"handlers": {"rich": {"class": "rich.logging.RichHandler"}},
"handlers": {
"rich": {"class": "kedro.logging.RichHandler", "rich_tracebacks": True}
},
"loggers": {"kedro": {"level": "INFO"}},
"root": {"handlers": ["rich"]},
}
Expand All @@ -35,8 +37,9 @@ def test_environment_variable_logging_config(monkeypatch, tmp_path):
logging_config = {"version": 1, "loggers": {"kedro": {"level": "WARNING"}}}
with config_path.open("w", encoding="utf-8") as f:
yaml.dump(logging_config, f)
del sys.modules["kedro.framework.project"]
from kedro.framework.project import LOGGING # noqa
from kedro.framework.project import _ProjectLogging

LOGGING = _ProjectLogging()

assert LOGGING.data == logging_config
assert logging.getLogger("kedro").level == logging.WARNING
Expand All @@ -50,24 +53,65 @@ def test_configure_logging():


def test_rich_traceback_enabled(mocker):
"""Note we need to force reload; just doing from kedro.framework.project import ...
will not call rich.traceback.install again. Using importlib.reload does not work
well with other tests here, hence the manual del sys.modules."""
rich_traceback_install = mocker.patch("rich.traceback.install")
rich_pretty_install = mocker.patch("rich.pretty.install")
del sys.modules["kedro.framework.project"]
from kedro.framework.project import LOGGING # noqa
Comment on lines -58 to -59
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed many of these tests to just simply reconfigure instead of loading the module. I was wrong about importing kedro.framework.project has no side-effect previously, it still has side-effect, but possible to reconfigure it with LOGGING.configure now.


LOGGING.configure(default_logging_config)

rich_traceback_install.assert_called()
rich_pretty_install.assert_called()


def test_rich_traceback_not_installed(mocker):
rich_traceback_install = mocker.patch("rich.traceback.install")
rich_pretty_install = mocker.patch("rich.pretty.install")
rich_handler = {
"class": "kedro.logging.RichHandler",
"rich_tracebacks": False,
"tracebacks_show_locals": True,
}
test_logging_config = default_logging_config.copy()
test_logging_config["handlers"]["rich"] = rich_handler

LOGGING.configure(test_logging_config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth extracting this into a fixture since it's used in a couple of places? Not a big deal though, so up to you.


rich_pretty_install.assert_called_once()
rich_traceback_install.assert_not_called()


def test_rich_traceback_configuration(mocker):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd split this up into two tests, because the logic that handles suppress is different from all other arguments:

  1. test the normal case to check that tracebacks_show_locals gets turned into show_locals
  2. test the special case to check that tracebacks_suppress extends rather than overrides the argument

rich_traceback_install = mocker.patch("rich.traceback.install")
rich_pretty_install = mocker.patch("rich.pretty.install")

import click

sys_executable_path = str(Path(sys.executable).parent)
traceback_install_defaults = {"suppress": [click, sys_executable_path]}
fake_path = "dummy"
rich_handler = {
"class": "kedro.logging.RichHandler",
"rich_tracebacks": True,
"tracebacks_show_locals": True,
"tracebacks_suppress": [fake_path],
}

test_logging_config = default_logging_config.copy()
test_logging_config["handlers"]["rich"] = rich_handler
LOGGING.configure(test_logging_config)

expected_install_defaults = traceback_install_defaults.copy()
expected_install_defaults["suppress"].extend([fake_path])
expected_install_defaults["show_locals"] = True
rich_traceback_install.assert_called_with(**expected_install_defaults)
rich_pretty_install.assert_called_once()


def test_rich_traceback_disabled_on_databricks(mocker, monkeypatch):
monkeypatch.setenv("DATABRICKS_RUNTIME_VERSION", "1")
rich_traceback_install = mocker.patch("rich.traceback.install")
rich_pretty_install = mocker.patch("rich.pretty.install")
del sys.modules["kedro.framework.project"]
from kedro.framework.project import LOGGING # noqa

LOGGING.configure(default_logging_config)

rich_traceback_install.assert_not_called()
rich_pretty_install.assert_called()