Add logging final version
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.
Proposed Changes
Checklist
- [ ]
CONTRIBUTINGguide 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
typingis used to provide type hints, including and not limited to usingOptionalif 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 installstep 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].
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅
@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: ...
Sounds good, @VukW! Thank you for the explanation. 😄
Hi guys @VukW, @sarthakpati, Thanks for the detailed review. I agree with it. I will do it.
@sarthakpati @VukW I added a Logging section in the documentation for extending GaNDLF
@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.
@sarthakpati I made the proposed changes...!! Thanks a lot for the suggestions.!!
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.
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?