GaNDLF icon indicating copy to clipboard operation
GaNDLF copied to clipboard

Add logging final version

Open benmalef opened this issue 1 year ago • 5 comments

Fixes #755

Brief Description

This PR fixes the issues from this PR

I have tried to implement this ref

I have NOT implemented the tqdm ref. I will create a separate PR.

Screenshots

This screenshot shows how logging messages are in the file.

image

Proposed Changes

Checklist

  • [ ] CONTRIBUTING guide has been followed.
  • [ ] PR is based on the current GaNDLF master .
  • [ ] Non-breaking change (does not break existing functionality): provide as many details as possible for any breaking change.
  • [ ] Function/class source code documentation added/updated (ensure typing is used to provide type hints, including and not limited to using Optional if a variable has a pre-defined value).
  • [ ] Code has been blacked for style consistency and linting.
  • [ ] If applicable, version information has been updated in GANDLF/version.py.
  • [ ] If adding a git submodule, add to list of exceptions for black styling in pyproject.toml file.
  • [ ] Usage documentation has been updated, if appropriate.
  • [ ] Tests added or modified to cover the changes; if coverage is reduced, please give explanation.
  • [ ] If customized dependency installation is required (i.e., a separate pip install step is needed for PR to be functional), please ensure it is reflected in all the files that control the CI, namely: python-test.yml, and all docker files [1,2,3].

benmalef avatar Jul 02 '24 22:07 benmalef

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

github-actions[bot] avatar Jul 02 '24 22:07 github-actions[bot]

@sarthakpati I agree it would be nice to mention logging in extension guide. However, the whole PR is created in the way that we hide all configuration from the developer - so developer almost doesn't need to bother about it. So, I believe there is not a big need to describe how to create loggers in new or modified code, but instead it would be useful to describe how logging is configured right now and how to log stuff sustainably

### Logging

#### Use loggers instead of print
We use native Python `logging` library for logs management. It is already configured, so if you are extending the code, please use loggers instead of `print` calls.
    ```
def my_new_cool_function(df: pd.DataFrame):
    logger = logging.getLogger(__name__)  # you can use any your own logger name or just pass a current file name
    logger.debug("Message for debug file only")
    logger.info("Hi GaNDLF user, I greet you in the CLI output")
    logger.error(f"A detailed message about any error if needed. Exception: {str(e)}, params: {params}, df shape: {df.shape}")
    # print("Hi GaNDLF user!")  # don't use prints please.
    ```

#### What and where is logged

GaNDLF logs are splitted into multiple parts:
- CLI output: only `info` messages are shown here
- debug file: ...
- errors file: ...

VukW avatar Jul 03 '24 14:07 VukW

Sounds good, @VukW! Thank you for the explanation. 😄

sarthakpati avatar Jul 03 '24 14:07 sarthakpati

Hi guys @VukW, @sarthakpati, Thanks for the detailed review. I agree with it. I will do it.

benmalef avatar Jul 03 '24 15:07 benmalef

@sarthakpati @VukW I added a Logging section in the documentation for extending GaNDLF

benmalef avatar Jul 04 '24 08:07 benmalef

@VukW @sarthakpati I have tried to implement this.

  • Updated the ganldlf_logger.
  • I added the colorlog, to beautify the logs in the console.

As mentioned, a new separated PR will be created for CLI commands ref.

benmalef avatar Jul 15 '24 10:07 benmalef

@sarthakpati I made the proposed changes...!! Thanks a lot for the suggestions.!!

benmalef avatar Jul 19 '24 15:07 benmalef

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.46%. Comparing base (b6dfe2d) to head (f425289).

Additional details and impacted files
@@                   Coverage Diff                   @@
##           new-apis_v0.1.0-dev     #893      +/-   ##
=======================================================
+ Coverage                94.41%   94.46%   +0.04%     
=======================================================
  Files                      159      160       +1     
  Lines                     9387     9482      +95     
=======================================================
+ Hits                      8863     8957      +94     
- Misses                     524      525       +1     
Flag Coverage Δ
unittests 94.46% <100.00%> (+0.04%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 20 '24 15:07 codecov[bot]

The recent changes are good for me to merge. @VukW, if you are okay as well, let's merge this in and start the process of migrating the current master to an old_api branch and move on?

sarthakpati avatar Jul 23 '24 19:07 sarthakpati