-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Do not initialize logging on import #8634
base: main
Are you sure you want to change the base?
Conversation
I likely have to go through a couple of edge cases and adjust some tests for this to work. I haven't tried how the silence_logs kwarg factors in but overall I think this change is a strictly positive improvement |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 29 files ± 0 29 suites ±0 10h 57m 8s ⏱️ + 1h 14m 12s For more details on these failures, see this check. Results for commit 2260866. ± Comparison against base commit e4a0545. This pull request removes 13 and adds 8 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
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.
Overall, this looks like an improvement to me.
I haven't tried how the silence_logs kwarg factors in but overall I think this change is a strictly positive improvement
Testing this would be good to help us understand if this causes a regression for silence_logs
.
I checked the behavior again. If |
Initializing logging on import has many unwanted side-effects.
Most importantly, it is very difficult to configure / overwrite anything unless the config file is overwritten directly. This should delay log configuration until it is needed. This allows code like this to work
which just prints