-
Notifications
You must be signed in to change notification settings - Fork 906
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
Enable alternative to run Kedro without Rich again. #4090
Conversation
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Thanks @lrcouto ! I've tried it and with this fix it's possible to uninstall rich again ⭐ However, I think the change introduced in #3682 never really worked properly. The logic to detect the rich handler doesn't seem to work. When I have |
def _has_rich_handler(logger: logging.Logger) -> bool: | ||
"""Returns true if the logger has a RichHandler attached.""" | ||
try: | ||
from rich.logging import RichHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed to move it from logging
to utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was checking if rich is installed at all, because if it isn't, the handler is not going to be there, making the function automatically return false. If it is installed, the user might still not be using it on their logging config, so we move to checking if it's present.
This approach is not working though, I'm verifying it right now.
Closing because this issue will be solved by #4097 |
Description
#3682 made our workaround to allow Kedro to be run without Rich stop working. This happened because one of the new functions implemented,
_has_rich_handler
, required importingRichHandler
, which depended on Rich. The changes on this PR allow the workaround to function again in the same way it did before.Development notes
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file