Add logging - DEPRECATED

benmalef commented 1 month ago

Fixes #755

Brief description

It adds the logging implementation.

I have tried to implement these requirements

Please review the code and give me any feedback.. :) It is a draft, it hasn't been completed yet.

log is splitted into four parts: debug log file, stdout, stderr, tqdm progress file

debug log: file catches everything DEBUG+. In particular, it should catch errors and warnings etc also (thus, both stdout and stderr content is also catched by debug log file)
stdout: displays INFO messages. Not lower, not higher
stderr: displays WARNING+ messages
tqdm: is not implemented. Maybe we can use this

Usage

You can use the logger directly in the module:

Here is an example

from GANDLF.utils import gandlf_logger_setup
logger = gandlf_logger_setup(__name__) 
logger.warning("warning message")

Proposed Changes

Create the gandlf_logger file in utils dir.
Create the logging_config file.
It contains gandlf_logger_setup fun that sets up the logger using the logging_config.yaml file.

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

github-actions[bot] commented 1 month ago

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

VukW commented 1 month ago

Hi @benmalef ! Thanks for the code. The first 4 points look exactly as expected, I personally like the elegancy of the configuration & usage :)

Concerning tqdm, your proposal with logging_redirect_tqdm looks beauty but requires setting a context for every tqdm cycle. Let's think if we can simplify this by any kind of wrapper over tqdm ?.. so context is set up automatically when cycle is used...

VukW commented 1 month ago

concerning tqdm: we can use the following wrapper to handle the redirect context automatically:

def custom_tqdm(it: Iterable, *args, **kwargs):
    with logging_redirect_tqdm():
        for x in tqdm(it, *args, **kwargs):
            yield x

for ic in custom_tqdm(range(10)):
    ....

benmalef commented 1 month ago

Hi @VukW, First of all, thanks for your review and your comments. They are very meaningful. I will try to address them as soon as possible.

Concerning tqdm, your proposal with logging_redirect_tqdm looks beauty but requires setting a context for every tqdm cycle. Let's think if we can simplify this by any kind of wrapper over tqdm ?.. so context is set up automatically when cycle is used...

I agree with you. I will try to do it. Maybe it would be better to create a new PR for this.?

benmalef commented 1 month ago

concerning tqdm: we can use the following wrapper to handle the redirect context automatically:
def custom_tqdm(it: Iterable, *args, **kwargs):
    with logging_redirect_tqdm():
        for x in tqdm(it, *args, **kwargs):
            yield x

for ic in custom_tqdm(range(10)):
    ....

Yes, we can do it like this. !! Thanks for the advice!!

benmalef commented 1 month ago

Hi @VukW, @sarthakpati I am trying to solve this error in Openfl-test. It can not find the logging_config.yaml file. Locally, it is running fine without any errors. Any thoughts ???

sarthakpati commented 1 month ago

There is an error when running the mlcube test [ref] (which likely goes for others as well):

2024-06-15T09:00:25.5466986Z ERROR: Traceback (most recent call last):
2024-06-15T09:00:25.5468631Z The ``converters`` are currently experimental. It may not support operations including (but not limited to) Functions in ``torch.nn.functional`` that involved data dimension
2024-06-15T09:00:25.5471978Z WARNING: Defining 'patch_sampler' as a string will be deprecated in a future release, please use a dictionary instead
2024-06-15T09:00:25.5491341Z   File "/home/runner/work/GaNDLF/GaNDLF/GANDLF/entrypoints/run.py", line 66, in _run
2024-06-15T09:00:25.5492557Z WARNING: Initializing 'memory_save_mode' as False
2024-06-15T09:00:25.5493179Z     main_run(
2024-06-15T09:00:25.5494396Z   File "/home/runner/work/GaNDLF/GaNDLF/GANDLF/cli/main_run.py", line 63, in main_run
2024-06-15T09:00:25.5495445Z WARNING: Initializing 'determinism' as False
2024-06-15T09:00:25.5496174Z WARNING: Initializing 'previous_parameters' as None
2024-06-15T09:00:25.5498525Z     logger = gandlf_logger_setup(__name__)
2024-06-15T09:00:25.5501608Z   File "/home/runner/work/GaNDLF/GaNDLF/GANDLF/utils/gandlf_logger.py", line 63, in gandlf_logger_setup
2024-06-15T09:00:25.5502722Z     with open(config_path, "r") as file:
2024-06-15T09:00:25.5503661Z FileNotFoundError: [Errno 2] No such file or directory: 'logging_config.yaml'

And (in the same logs):

2024-06-15T09:03:37.8325369Z ERROR: Traceback (most recent call last):
2024-06-15T09:03:37.8326307Z   File "/GaNDLF/GANDLF/entrypoints/run.py", line 66, in _run
2024-06-15T09:03:37.8326861Z     main_run(
2024-06-15T09:03:37.8329112Z   File "/GaNDLF/GANDLF/cli/main_run.py", line 121, in main_run
2024-06-15T09:03:37.8329773Z     InferenceManager(
2024-06-15T09:03:37.8330508Z   File "/GaNDLF/GANDLF/inference_manager.py", line 69, in InferenceManager
2024-06-15T09:03:37.8331314Z     inference_loop(
2024-06-15T09:03:37.8332085Z   File "/GaNDLF/GANDLF/compute/inference_loop.py", line 89, in inference_loop
2024-06-15T09:03:37.8333459Z     assert file_to_load != None, "The 'best_file' was not found"
2024-06-15T09:03:37.8334690Z AssertionError: The 'best_file' was not found

VukW commented 2 weeks ago

Hi folks! I was going just to comment how to fix the issue, but it seems it's easier to show the fix on the code itself. https://github.com/benmalef/GaNDLF/pull/6

So, the root issue is you are using a relative logging_config.yaml path; it resolves to the current user working directory. So, if you run tests, the working directory is ./testing/, while config is located at ./logging_config.yaml.

To fix that we need to ensure that logging config path does not depend on user's workdir. The best solution is to add this file to the wheel and install it together with gandlf's module (site-packages/GANDLF in user's python env). At the same time, adding the file to the wheel is crucial as we want to distribute config together with pip package.

So, the solution includes: (1) move 'logging_config.yaml from repo root to ./GANDLF/ module source (only files located in module sources can be distributed with wheel) (2) modify setup.py, MANIFEST.in to include this file to the package (required some setup.py cleaning beforehand, I've done that here https://github.com/mlcommons/GaNDLF/pull/890) (3) modify logging configuration code to use file from package resources instead of usual relative path.

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 94.38%. Comparing base (1715c1d) to head (be22bdb).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## new-apis_v0.1.0-dev #881 +/- ## ======================================================= - Coverage 94.38% 94.38% -0.01% ======================================================= Files 159 160 +1 Lines 9354 9371 +17 ======================================================= + Hits 8829 8845 +16 - Misses 525 526 +1 ``` | [Flag](https://app.codecov.io/gh/mlcommons/GaNDLF/pull/881/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mlcommons) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/mlcommons/GaNDLF/pull/881/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mlcommons) | `94.38% <100.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mlcommons#carryforward-flags-in-the-pull-request-comment) to find out more.

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

benmalef commented 2 weeks ago

Hi folks! I was going just to comment how to fix the issue, but it seems it's easier to show the fix on the code itself. benmalef#6

So, the root issue is you are using a relative logging_config.yaml path; it resolves to the current user working directory. So, if you run tests, the working directory is ./testing/, while config is located at ./logging_config.yaml.

To fix that we need to ensure that logging config path does not depend on user's workdir. The best solution is to add this file to the wheel and install it together with gandlf's module (site-packages/GANDLF in user's python env). At the same time, adding the file to the wheel is crucial as we want to distribute config together with pip package.

So, the solution includes: (1) move 'logging_config.yaml from repo root to ./GANDLF/ module source (only files located in module sources can be distributed with wheel) (2) modify setup.py, MANIFEST.in to include this file to the package (required some setup.py cleaning beforehand, I've done that here #890) (3) modify logging configuration code to use file from package resources instead of usual relative path.

@VukW Thanks a lot for your solution. I am trying to fix it.

VukW commented 2 weeks ago

@benmalef You can just check this: https://github.com/benmalef/GaNDLF/pull/6 here I have implemented the fix already. My comment was actually an explanation of what exactly (and why) I did to fix the issue:)

benmalef commented 2 weeks ago

@benmalef You can just check this: benmalef#6 here I have implemented the fix already. My comment was actually an explanation of what exactly (and why) I did to fix the issue:)

aa ok. I will check it. Thanks a lot..!! :P

mlcommons / GaNDLF