Skip to content
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

Merged
merged 11 commits into from
Sep 22, 2021
Merged

Conversation

jfsantos-ds
Copy link
Contributor

@jfsantos-ds jfsantos-ds commented Sep 21, 2021

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

@jfsantos-ds
Copy link
Contributor Author

jfsantos-ds commented Sep 21, 2021

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:

  • logger class
  • logger instantiation in the engines (see the core engine and data_quality engine)

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 🍻

Copy link
Contributor

@UrbanoFonseca UrbanoFonseca left a 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

src/ydata_quality/core/data_quality.py Outdated Show resolved Hide resolved
src/ydata_quality/core/engine.py Outdated Show resolved Hide resolved
src/ydata_quality/duplicates/engine.py Show resolved Hide resolved
src/ydata_quality/erroneous_data/engine.py Outdated Show resolved Hide resolved
src/ydata_quality/core/data_quality.py Outdated Show resolved Hide resolved
src/ydata_quality/core/data_quality.py Outdated Show resolved Hide resolved
src/ydata_quality/core/data_quality.py Outdated Show resolved Hide resolved
src/ydata_quality/core/data_quality.py Outdated Show resolved Hide resolved
src/ydata_quality/core/engine.py Outdated Show resolved Hide resolved
src/ydata_quality/data_expectations/engine.py Show resolved Hide resolved
src/ydata_quality/data_expectations/engine.py Outdated Show resolved Hide resolved
src/ydata_quality/data_relations/engine.py Show resolved Hide resolved
src/ydata_quality/missings/engine.py Outdated Show resolved Hide resolved
src/ydata_quality/utils/logger.py Outdated Show resolved Hide resolved
Copy link
Contributor

@UrbanoFonseca UrbanoFonseca left a 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/duplicates/engine.py Outdated Show resolved Hide resolved
src/ydata_quality/missings/engine.py Outdated Show resolved Hide resolved

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)
Copy link
Member

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."

@jfsantos-ds jfsantos-ds merged commit 1163aae into master Sep 22, 2021
@jfsantos-ds jfsantos-ds deleted the feat/add_logger branch September 22, 2021 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants