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 30 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
1 change: 1 addition & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
include README.md
include LICENSE.md
include kedro/framework/project/default_logging.yml
include kedro/framework/project/rich_logging.yml
include kedro/ipython/*.png
include kedro/ipython/*.svg
recursive-include kedro/templates *
1 change: 1 addition & 0 deletions docs/source/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@
# "anchor not found" but it's a valid selector for code examples
"https://docs.delta.io/latest/delta-update.html#language-python",
"https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/default_logging.yml",
"https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/rich_logging.yml",
"https://github.com/kedro-org/kedro/blob/main/README.md#the-humans-behind-kedro", # "anchor not found" but is valid
"https://opensource.org/license/apache2-0-php/",
"https://docs.github.com/en/rest/overview/other-authentication-methods#via-username-and-password",
Expand Down
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
9 changes: 8 additions & 1 deletion kedro/framework/project/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,14 @@ def __init__(self) -> None:
# Check if the default logging configuration exists
default_logging_path = Path("conf/logging.yml")
if not default_logging_path.exists():
default_logging_path = Path(__file__).parent / "default_logging.yml"
default_logging_path = Path(
os.environ.get(
"KEDRO_LOGGING_CONFIG",
Path(__file__).parent / "rich_logging.yml"
if importlib.util.find_spec("rich")
else Path(__file__).parent / "default_logging.yml",
)
)

# Use the user path if available, otherwise, use the default path
if user_logging_path and Path(user_logging_path).exists():
Expand Down
27 changes: 20 additions & 7 deletions kedro/framework/project/default_logging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,30 @@ version: 1

disable_existing_loggers: False

formatters:
simple:
format: "%(asctime)s - %(name)s - %(levelname)s - %(message)s"

handlers:
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
console:
class: logging.StreamHandler
level: INFO
formatter: simple
stream: ext://sys.stdout

info_file_handler:
class: logging.handlers.RotatingFileHandler
level: INFO
formatter: simple
filename: info.log
maxBytes: 10485760 # 10MB
backupCount: 20
encoding: utf8
delay: True

loggers:
kedro:
level: INFO

root:
handlers: [rich]
handlers: [console]
38 changes: 38 additions & 0 deletions kedro/framework/project/rich_logging.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
version: 1

disable_existing_loggers: False

formatters:
simple:
format: "%(asctime)s - %(name)s - %(levelname)s - %(message)s"

handlers:
console:
class: logging.StreamHandler
level: INFO
formatter: simple
stream: ext://sys.stdout

info_file_handler:
class: logging.handlers.RotatingFileHandler
level: INFO
formatter: simple
filename: info.log
maxBytes: 10485760 # 10MB
backupCount: 20
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

root:
handlers: [rich]
17 changes: 13 additions & 4 deletions kedro/ipython/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from __future__ import annotations

import importlib
import inspect
import logging
import os
Expand All @@ -18,8 +19,12 @@
from IPython.core.getipython import get_ipython
from IPython.core.magic import needs_local_scope, register_line_magic
from IPython.core.magic_arguments import argument, magic_arguments, parse_argstring
from rich.console import Console
from rich.syntax import Syntax

try:
from rich.console import Console
from rich.syntax import Syntax
except ImportError:
pass
lrcouto marked this conversation as resolved.
Show resolved Hide resolved

from kedro.framework.cli import load_entry_points
from kedro.framework.cli.project import CONF_SOURCE_HELP, PARAMS_ARG_HELP
Expand Down Expand Up @@ -281,8 +286,12 @@ def _create_cell_with_text(text: str, is_jupyter: bool = True) -> None:

def _print_cells(cells: list[str]) -> None:
for cell in cells:
Console().print("")
Console().print(Syntax(cell, "python", theme="monokai", line_numbers=False))
if importlib.util.find_spec("rich") is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This is a second attempt at importing rich, and it's actually going to happen for each cell, which is unnecessarily costly.

I think it's a pretty common pattern to just do this stuff once at the top. See above comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point, thanks!

Console().print("")
Console().print(Syntax(cell, "python", theme="monokai", line_numbers=False))
lrcouto marked this conversation as resolved.
Show resolved Hide resolved
else:
print("") # noqa: T201
print(cell) # noqa: T201


def _load_node(node_name: str, pipelines: _ProjectPipelines) -> list[str]:
Expand Down
4 changes: 0 additions & 4 deletions kedro/logging.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
"""
This module contains a logging handler class which produces coloured logs and tracebacks.
"""

import logging
import sys
from pathlib import Path
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,4 @@ loggers:
level: INFO

root:
handlers: [rich, info_file_handler]
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need both handlers? would it duplicates the log?

handlers: [rich]
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ dependencies = [
"build>=0.7.0",
"cachetools>=4.1",
"click>=4.0",
"cookiecutter>=2.1.1,<3.0",
"cookiecutter==2.2.0", # 2.3 onwards depends on rich, which we want to avoid
Copy link
Member

Choose a reason for hiding this comment

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

That sucks...

At some point, maybe it makes sense to separate out project generation somehow (or, there's all the conversations about moving from cookiecutter), but that's out of scope.

Can this at least be left as flexible as possible? Perhaps:

Suggested change
"cookiecutter==2.2.0", # 2.3 onwards depends on rich, which we want to avoid
"cookiecutter>=2.1.1,<2.3", # 2.3 onwards depends on rich, which we want to avoid

Copy link
Member

Choose a reason for hiding this comment

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

At some point, maybe it makes sense to separate out project generation somehow (or, there's all the conversations about moving from cookiecutter)

I 100 % agree. As much as I'm a fan for exploring cookiecutter alternatives, decoupling pip install kedro from cookiecutter is more important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested this broader range and it works. Let's use it.

"dynaconf>=3.1.2,<4.0",
"fsspec>=2021.4",
"gitpython>=3.0",
Expand Down
Loading