Skip to content

Conversation

@ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Feb 23, 2023

Related Issues

Proposed Changes:

  • Replace all direct calls to logging.debug|info|warning|error|critical with calls to each file's dedicated logger instance.
  • Add a custom PyLint checker to prevent future occurrences of the same issue
  • Add a test that makes sure that handlers are not set.

How did you test it?

  • CI
  • Manual run of PyLint locally

Notes for the reviewer

Check out the linked issue for the reasoning behind this change. The only changes applied here are related to logging lines and replace logging with logger, adding the linter and the test: everything else is out of scope.

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Looks very good to me! 👍 The only question that remains is how to prevent that from happening again in future. Can be easily overlooked. 🤔 Before merging you'll just need to deal with pylint...

@ZanSara
Copy link
Contributor Author

ZanSara commented Feb 23, 2023

I'll need to remove the test, seems like PyTest is setting up loggers: https://github.com/deepset-ai/haystack/actions/runs/4254366090/jobs/7400573111

@julian-risch julian-risch self-requested a review February 23, 2023 16:35
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Great that you found more of the problematic lines of code. Now we can be sure that we found all of them. The solution is more complex than I hoped but let's try it out and see if it works well for us. 👍

@ZanSara
Copy link
Contributor Author

ZanSara commented Feb 23, 2023

Agree! I also find it a bit complex, but on the other hand it's easy to remove in the moment when it causes trouble, so let's see how it goes 😊

@ZanSara ZanSara merged commit 13c4ff1 into main Feb 23, 2023
@ZanSara ZanSara deleted the remove-direct-logging branch February 23, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ignore-for-release-notes PRs with this flag won't be included in the release notes. type:refactor Not necessarily visible to the users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Haystack should not configure root logger handlers

3 participants