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

Enable alternative to run Kedro without Rich again. #4090

Closed
wants to merge 7 commits into from

Conversation

lrcouto
Copy link
Contributor

@lrcouto lrcouto commented Aug 14, 2024

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 importing RichHandler, 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

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

lrcouto and others added 7 commits August 14, 2024 12:25
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>
@lrcouto lrcouto marked this pull request as ready for review August 15, 2024 02:45
@lrcouto lrcouto requested a review from merelcht as a code owner August 15, 2024 02:45
@merelcht
Copy link
Member

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 rich installed the orange colouring isn't there, and I added some print statements and it seems like the RichHandler isn't ever set. Maybe I'm missing something? @noklam @sbrugman

def _has_rich_handler(logger: logging.Logger) -> bool:
"""Returns true if the logger has a RichHandler attached."""
try:
from rich.logging import RichHandler
Copy link
Contributor

@noklam noklam Aug 15, 2024

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?

Copy link
Contributor Author

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.

@lrcouto
Copy link
Contributor Author

lrcouto commented Aug 16, 2024

Closing because this issue will be solved by #4097

@lrcouto lrcouto closed this Aug 16, 2024
@sbrugman sbrugman deleted the fix-rich-alternative branch September 9, 2024 22:01
@sbrugman sbrugman restored the fix-rich-alternative branch September 9, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants