-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add better logging in the indicator runner #1892
Conversation
@@ -67,6 +67,7 @@ def run_indicator_pipeline(indicator_fn: Callable[[Params], None], | |||
validator = validator_fn(params) |
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.
The archiver already logs run timing, and each indicator has its own bit of logging which includes that; this adds logging for the flash & validation steps.
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.
the individual indicators' run time logging looks mostly sufficient, except in quidel_covidtest
where it happens at program exit, which will include anything that runs after the core indicator function and thus lead to inaccuracy.
you can refactor this so the runner does the timing and logging, with indicator_fn
(aka each indicator's run_module()
) returning a dict of metrics it wants logged (like csv_export_count and max_lag_in_days)
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.
Hm, I can actually think of one disadvantage of that - it means that the summary line will be gone from the logs if the indicator is ran individually (e.g. env/bin/python -m delphi_quidel_covidtest
as the README suggests).
Are we OK with this?
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.
a lot of the documentation in this repo is is quite old and should be brought up to date.
is it ever desirable to run an indicator without validation and archiving? perhaps we can answer that in #1895. you should at least fix the timing for the quidel indicator in the meanwhile.
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.
i havent looked at all of the individual validator messages yet, but i will in the next round. can you also get timing for that other method mentioned in #1890 ?
@@ -67,6 +67,7 @@ def run_indicator_pipeline(indicator_fn: Callable[[Params], None], | |||
validator = validator_fn(params) |
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.
the individual indicators' run time logging looks mostly sufficient, except in quidel_covidtest
where it happens at program exit, which will include anything that runs after the core indicator function and thus lead to inaccuracy.
you can refactor this so the runner does the timing and logging, with indicator_fn
(aka each indicator's run_module()
) returning a dict of metrics it wants logged (like csv_export_count and max_lag_in_days)
@melange396 updated, rerequesting review! |
# Log stats now instead of at program exit | ||
atexit.unregister(log_exit) | ||
log_exit(start_time, stats, logger) |
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.
nice!
@@ -281,29 +281,35 @@ def check_bad_val(self, df_to_test, nameformat, signal_type, report): | |||
|
|||
if percent_option: | |||
if not df_to_test[(df_to_test['val'] > 100)].empty: | |||
bad_values = df_to_test[(df_to_test['val'] > 100)]['val'].unique() |
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.
nice touch with the .unique()
filter! should we do that for all the messages that print lists of values?
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.
good stuff!
Description
Adds more detail to the logging of the indicator runner, as specified in #1890.
Changelog
Fixes