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

Make it possible to use Kedro after uninstalling Rich. #3838

Merged
merged 57 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
3f39cd3
attempt to configure fallback logger
lrcouto Apr 24, 2024
80f0ff2
Set up fallback for logging on the logging function itself
lrcouto Apr 24, 2024
8c07db1
Lint
lrcouto Apr 24, 2024
6c66689
Lint
lrcouto Apr 24, 2024
d883b5d
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto Apr 25, 2024
e35cdaa
Merge branch 'main' into remove-rich-as-mandatory-dependence
SajidAlamQB Apr 30, 2024
bfbb1f1
Update kedro/logging.py
lrcouto Apr 30, 2024
2038792
Changes to rich variable on logging
lrcouto Apr 30, 2024
5b85711
Lint
lrcouto May 2, 2024
b75327d
attempt to configure fallback logger
lrcouto Apr 24, 2024
de86e8d
Set up fallback for logging on the logging function itself
lrcouto Apr 24, 2024
658d0c1
Lint
lrcouto Apr 24, 2024
609a0c2
Lint
lrcouto Apr 24, 2024
99bd891
Update kedro/logging.py
lrcouto Apr 30, 2024
a15bdcd
Changes to rich variable on logging
lrcouto Apr 30, 2024
e80d514
Lint
lrcouto May 2, 2024
f802384
adapt default logging
lrcouto May 3, 2024
b5aed65
Merge
lrcouto May 3, 2024
5061229
Update kedro/logging.py
lrcouto May 3, 2024
7c731f3
Create separate logging config file for rich
lrcouto May 6, 2024
c63c068
Merge branch 'remove-rich-as-mandatory-dependence' of github.com:kedr…
lrcouto May 6, 2024
a29220c
Hello linter my old friend
lrcouto May 6, 2024
f43c72c
Alternative for rich in kedro ipython
lrcouto May 6, 2024
8389023
Remove unnecessary try/except block
lrcouto May 6, 2024
1ea51e8
Update pyproject.toml
lrcouto May 7, 2024
6b98284
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 8, 2024
3663bdf
Resolve merge conflict
lrcouto May 8, 2024
115d339
Merge branch 'remove-rich-as-mandatory-dependence' of github.com:kedr…
lrcouto May 8, 2024
16e05b8
Lint
lrcouto May 8, 2024
269ec98
Fix config file paths
lrcouto May 8, 2024
5f3640b
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 9, 2024
235a5bf
Update kedro/ipython/__init__.py
lrcouto May 9, 2024
c1f6cbf
Update kedro/ipython/__init__.py
lrcouto May 9, 2024
210b0ed
Prevent kedro ipython from reimporting rich multiple times
lrcouto May 9, 2024
98f8e4d
Logging config
lrcouto May 9, 2024
191bba7
Change test config
lrcouto May 10, 2024
b44fb5e
Remove ' yEs ' tests
lrcouto May 10, 2024
c4c8aa7
Tests to cover ipython changes
lrcouto May 10, 2024
d76124f
Add test for import
lrcouto May 10, 2024
9dbaf94
Ignore test coverage on import try/except
lrcouto May 10, 2024
0c63233
Lint
lrcouto May 10, 2024
f3c65fa
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 10, 2024
1be4b05
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 14, 2024
4775fe4
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 14, 2024
671917d
Unpin cookiecutter version
lrcouto May 14, 2024
a8fea7a
Merge branch 'remove-rich-as-mandatory-dependence' of github.com:kedr…
lrcouto May 14, 2024
eb3b151
Add changes to documentation
lrcouto May 15, 2024
0f0214f
Update RELEASE.md
lrcouto May 15, 2024
192d349
Change RICH variable name to RICH_INTALLED
lrcouto May 15, 2024
28534ad
Merge branch 'remove-rich-as-mandatory-dependence' of github.com:kedr…
lrcouto May 15, 2024
02d8919
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 16, 2024
2074054
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 16, 2024
8c15acb
Add info about uninstalling rich on logging doc page
lrcouto May 16, 2024
6cf7d4d
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 20, 2024
61f44ce
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 20, 2024
6a41b4d
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 21, 2024
4f57399
Merge branch 'main' into remove-rich-as-mandatory-dependence
lrcouto May 21, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,6 @@ handlers:
encoding: utf8
delay: True

rich:
class: kedro.logging.RichHandler
rich_tracebacks: True
# Advance options for customisation.
# See https://docs.kedro.org/en/stable/logging/logging.html#project-side-logging-configuration
# tracebacks_show_locals: False

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

import logging
import sys
from pathlib import Path
from typing import Any

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

try:
import rich.logging
import rich.pretty
import rich.traceback
except ImportError:
rich: Any = None

lrcouto marked this conversation as resolved.
Show resolved Hide resolved
from kedro.utils import _is_databricks


class RichHandler(rich.logging.RichHandler):
class RichHandler(logging.StreamHandler if rich is None else 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)
Expand All @@ -30,6 +30,10 @@ class RichHandler(rich.logging.RichHandler):
"""

def __init__(self, *args: Any, **kwargs: Any):
if rich is None:
super().__init__(stream=sys.stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the basic idea that users who don't install rich should default to this? So for users who already have rich installed and no longer want it they need to pip uninstall rich? If that is the case we should document the steps they need to take.

Copy link
Contributor

Choose a reason for hiding this comment

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

@noklam do you have any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

My thought on this it shouldn't touch the log handler.

If the custom logger has Rich handler, it should raise error when it is not installed. To get rid of this the solution is just remove rich from the handler.

The thing that user cannot change now is all the traceback error / CLI that ties with rich. Essentially one cannot run Kedro without rich. @astrojuanlu

I am surprised the _ProjectLogging isn't changed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SajidAlamQB yes, the way this is set up at the moment, if the user does not want rich to be used, it would need to be uninstalled.

return
SajidAlamQB marked this conversation as resolved.
Show resolved Hide resolved

super().__init__(*args, **kwargs)
logging.captureWarnings(True)
rich.pretty.install()
Expand Down
Loading