-
Notifications
You must be signed in to change notification settings - Fork 159
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
[BUG] Use python logging level #2705
Conversation
src/lib.rs
Outdated
_ => LevelFilter::Error, | ||
}; | ||
|
||
let logger = pyo3_log::Logger::default().filter(level_filter); |
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.
We should only create the logger and install it on the first run.
Currently if the user calls refresh_logger
in python it panics.
import daft
daft.refresh_logger()
# PanicException: called `Result::unwrap()` on an `Err` value: SetLoggerError(())
src/lib.rs
Outdated
|
||
let logger = pyo3_log::Logger::default().filter(level_filter); | ||
let handle = logger.install().unwrap(); | ||
LOG_RESET_HANDLE.get_or_init(|| handle); |
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.
You should be able to put everything above in a function and then call that in the get_or_init
let func = || {
use log::LevelFilter;
let logging = py.import("logging")?;
let python_log_level = logging
...
logger.install().unwrap()
}
LOG_RESET_HANDLE.get_or_init(func).reset();
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.
this will ensure that we only init once and that it's reset every time it's called
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.
also let's add a test for that
CodSpeed Performance ReportMerging #2705 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2705 +/- ##
==========================================
- Coverage 63.52% 63.39% -0.13%
==========================================
Files 979 978 -1
Lines 112148 112180 +32
==========================================
- Hits 71239 71119 -120
- Misses 40909 41061 +152
|
d109504
to
7ac9b5d
Compare
src/lib.rs
Outdated
@@ -63,13 +64,41 @@ pub mod pylib { | |||
} | |||
|
|||
#[pyfunction] | |||
pub fn refresh_logger() { | |||
pub fn test_logging() { |
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.
it might be cleaner to have this return the max log level instead as get_max_log_level()
Example script:
Output: