-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: add loggers and change prints to logs #27
Conversation
Hey @portellaa this is a big PR in terms of diffs but small in terms of core changes, your insight is useful on the following key aspects:
Notice on the logger class that despite us doing get logger and retrieving always the same logger, we were adding a new handler over each time the function was run (exactly once per engine, 8x warnings on the full package class 💥 ). That is why the add handler is conditioned on the number of existing handlers of the logger. We are still gonna iterate this over time but still persisting and most in need ASAP fix is finding a clever way to print the module name with the logs. Ideas are much appreciated! Cheers 🍻 |
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.
Very nice functionality. Cannot forget to add the target=>label conversion in the features added by this PR
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.
minor details. looks good overall 🚀
src/ydata_quality/utils/logger.py
Outdated
|
||
def get_logger(name, stream: TextIO = sys.stdout, level: str=logging.INFO): | ||
acceptable_levels = [None]+list(_nameToLevel.keys()) | ||
assert level in acceptable_levels, "Valid levels for warning severity are {}. Defaults to info level.".format(acceptable_levels) |
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.
why not use fstr formatting?
assert level in acceptable_levels, f"Valid levels for warning severity are {acceptable_levels}. Defaults to info level."
Changed all prints to logs
Defined a base logger for DQ
Changed lingering target variable names to label
TODO: Find a good way to print module names in logs
Fix a persisting runtime bug