Add logging final version #893

benmalef · 2024-07-02T22:56:51Z

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

github-actions · 2024-07-02T22:57:05Z

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

add logging testing

add logging test in test_full

sarthakpati

I would recommend adding a Logging section in the documentation for extending GaNDLF detailing this process and how a developer needs to use this correctly. For example, there should be 2 sub-sections to this:

What does someone need to do when they are extending an existing function or class?
What does someone need to do when they are adding a new function or class?
Anything else?

Am I missing something @VukW?

testing/test_full.py

GANDLF/logging_config.yaml

VukW · 2024-07-03T14:54:28Z

@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: ...

sarthakpati · 2024-07-03T14:55:16Z

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

benmalef · 2024-07-03T15:31:15Z

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

Co-authored-by: Sarthak Pati <[email protected]>

GANDLF/logging_config.yaml

testing/test_full.py

benmalef · 2024-07-04T08:09:40Z

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

GANDLF/utils/gandlf_logger.py

sarthakpati · 2024-07-17T14:17:33Z

GANDLF/utils/gandlf_logger.py

+        output_dir = Path(log_dir)
+        Path(output_dir).mkdir(parents=True, exist_ok=True)
+        with resources.open_text("GANDLF", config_path) as file:
+            config_dict = yaml.safe_load(file)
+            config_dict["handlers"]["rotatingFileHandler"]["filename"] = str(
+                Path.joinpath(output_dir, "gandlf.log")
+            )
+            logging.config.dictConfig(config_dict)


Here, I will suggest the following (pseudo-code):

try: write a single line to the `log_file` (something like `"Starting GaNDLF logging session"`). except: # this means that the user does not have write access to the location given by `log_file`, so give that error, and tell the user that we are falling back to the default of flushing output to console call logging setup again but with `log_file` as `None`

GANDLF/utils/gandlf_logger.py

Co-authored-by: Sarthak Pati <[email protected]>

sarthakpati

Minor comments, and we should be ready to merge! Thanks a ton for this, @benmalef!

GANDLF/utils/gandlf_logger.py

sarthakpati · 2024-07-19T14:56:58Z

GANDLF/utils/gandlf_logger.py

+    try:
+        if log_file is None:  # create tmp file
+            log_tmp_file = _create_tmp_log_file()
+            _save_logs_in_file(log_tmp_file, config_path)
+            logging.info(f"The logs are saved in {log_tmp_file}")
+        else:  # create the log file
+            _create_log_file(log_file)
+            _save_logs_in_file(log_file, config_path)
+    except Exception as e:
+        _flush_to_console()
+        logging.error(f"log_file:{e}")
+        logging.warning("The logs will be flushed to console")


I would suggest the following execution order for clarity (also, try is no longer needed since the temp file should always have user-level write access):

Suggested change

try:

if log_file is None: # create tmp file

log_tmp_file = _create_tmp_log_file()

_save_logs_in_file(log_tmp_file, config_path)

logging.info(f"The logs are saved in {log_tmp_file}")

else: # create the log file

_create_log_file(log_file)

_save_logs_in_file(log_file, config_path)

except Exception as e:

_flush_to_console()

logging.error(f"log_file:{e}")

logging.warning("The logs will be flushed to console")

log_tmp_file = log_file

if log_file is None: # create tmp file

log_tmp_file = _create_tmp_log_file()

logging.info(f"The logs are saved in {log_tmp_file}")

_create_log_file(log_tmp_file)

_save_logs_in_file(log_tmp_file, config_path)

Yes, this code is cleaner. Thanks for the refactor.!

sarthakpati · 2024-07-19T14:57:27Z

GANDLF/utils/gandlf_logger.py

+def _flush_to_console():
+    formatter = colorlog.ColoredFormatter(
+        "%(log_color)s%(asctime)s - %(levelname)s - %(message)s",
+        datefmt="%Y-%m-%d %H:%M:%S",
+        log_colors={
+            "DEBUG": "blue",
+            "INFO": "green",
+            "WARNING": "yellow",
+            "ERROR": "red",
+            "CRITICAL": "bold_red",
+        },
+    )
+    console_handler = logging.StreamHandler()
+    console_handler.setFormatter(formatter)
+    logging.root.setLevel(logging.DEBUG)
+    logging.root.addHandler(console_handler)


If the try block is removed from the module below, then this function is no longer needed, right?

Suggested change

def _flush_to_console():

formatter = colorlog.ColoredFormatter(

"%(log_color)s%(asctime)s - %(levelname)s - %(message)s",

datefmt="%Y-%m-%d %H:%M:%S",

log_colors={

"DEBUG": "blue",

"INFO": "green",

"WARNING": "yellow",

"ERROR": "red",

"CRITICAL": "bold_red",

},

)

console_handler = logging.StreamHandler()

console_handler.setFormatter(formatter)

logging.root.setLevel(logging.DEBUG)

logging.root.addHandler(console_handler)

yes if we don't want to flush to the console, the function is no longer needed.

Co-authored-by: Sarthak Pati <[email protected]>

benmalef · 2024-07-19T15:51:17Z

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

sarthakpati

Minor semantic change. This PR should be good to merge after this.

GANDLF/utils/gandlf_logger.py

codecov · 2024-07-20T15:54:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VukW · 2024-07-23T08:01:00Z

GANDLF/entrypoints/cli_tool.py

@@ -24,7 +24,8 @@ def gandlf(ctx, loglevel):
    """GANDLF command-line tool."""
    ctx.ensure_object(dict)
    ctx.obj["LOGLEVEL"] = loglevel
-    setup_logging(loglevel)
+    # setup_logging(loglevel)


Let's remove redundant old loglevel stuff here

GANDLF/utils/gandlf_logging.py

VukW · 2024-07-23T08:08:35Z

GANDLF/utils/gandlf_logging.py

+    tmp_dir = Path(tempfile.gettempdir())
+    log_dir = Path.joinpath(tmp_dir, ".gandlf")
+    log_dir.mkdir(parents=True, exist_ok=True)
+    log_file = Path.joinpath(log_dir, get_unique_timestamp() + ".log")


VukW · 2024-07-23T08:10:59Z

GANDLF/utils/gandlf_logging.py

+    log_file.write_text("Starting GaNDLF logging session \n")
+
+
+def _save_logs_in_file(log_file, config_path):


just a nitpick - this function is not saving any logs, instead just configures logging. Maybe rename it to smth more relevant? For ex,

Suggested change

def _save_logs_in_file(log_file, config_path):

def _configure_logging_with_logfile(log_file, config_path):

VukW

Checked the overall code, no any significant issues, looks good to me! Thanks, man @benmalef

sarthakpati · 2024-07-23T19:09:12Z

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?

VukW

@sarthakpati Agree, looks good to me, let's merge it (just to remind, I believe @benmalef cannot merge it till your previous PR review result Request Changes is active)

benmalef added 10 commits July 2, 2024 18:07

add logging implementation

fe75f9c

update utils.__init__

08550d6

change logging_config

ea42f95

change logging_config

f54641c

change logging_config

10dee45

blacked gandlf_logger

e0aa707

add gandlf_setup in the entrypoints

d5493f1

blacked gandlf_logger

0d1ec65

blacked some files

8ee6308

blacked forward_pass

04fee16

benmalef and others added 8 commits July 3, 2024 12:20

add logging testing

b5203cf

update test_full

06c9d80

Merge pull request #8 from benmalef/add_test_logging

b29e4ef

add logging testing

black test_full

abddbd7

add logging test in test_full

c25396c

remove unnecessary imports

bd9ba0d

black forward_pass

57f02c9

Merge pull request #9 from benmalef/add_logging_test

78af3fa

add logging test in test_full

sarthakpati requested changes Jul 3, 2024

View reviewed changes

testing/test_full.py Outdated Show resolved Hide resolved

VukW reviewed Jul 3, 2024

View reviewed changes

GANDLF/logging_config.yaml Outdated Show resolved Hide resolved

change the logging test name

f1a3dfc

Co-authored-by: Sarthak Pati <[email protected]>

benmalef commented Jul 3, 2024

View reviewed changes

GANDLF/logging_config.yaml Show resolved Hide resolved

benmalef commented Jul 3, 2024

View reviewed changes

testing/test_full.py Show resolved Hide resolved

Add logging documentation (#10)

7d6f25e

change the log format

a2b834d

sarthakpati requested changes Jul 17, 2024

View reviewed changes

update gandlf_logger_setup

36da4d2

sarthakpati reviewed Jul 18, 2024

View reviewed changes

GANDLF/utils/gandlf_logger.py Outdated Show resolved Hide resolved

sarthakpati reviewed Jul 18, 2024

View reviewed changes

GANDLF/utils/gandlf_logger.py Outdated Show resolved Hide resolved

benmalef and others added 4 commits July 18, 2024 17:58

Update GANDLF/utils/gandlf_logger.py

d357fff

Co-authored-by: Sarthak Pati <[email protected]>

Update GANDLF/utils/gandlf_logger.py

3051b4b

Co-authored-by: Sarthak Pati <[email protected]>

change the default to create a tmp file

ca076df

Merge branch 'new-apis_v0.1.0-dev' into add_logging_final_version

50668b9

sarthakpati reviewed Jul 19, 2024

View reviewed changes

benmalef and others added 3 commits July 19, 2024 18:02

fix the error

e129be1

Update GANDLF/utils/gandlf_logger.py

05fa733

Co-authored-by: Sarthak Pati <[email protected]>

made proposed changes

e5bfd23

benmalef and others added 2 commits July 19, 2024 19:07

Update setup.py

f857e65

Merge branch 'new-apis_v0.1.0-dev' into add_logging_final_version

ca64876

sarthakpati requested changes Jul 19, 2024

View reviewed changes

GANDLF/utils/gandlf_logger.py Outdated Show resolved Hide resolved

GANDLF/utils/gandlf_logger.py Outdated Show resolved Hide resolved

change the def name to logger_setup

f425289

VukW reviewed Jul 23, 2024

View reviewed changes

GANDLF/utils/gandlf_logging.py Show resolved Hide resolved

VukW reviewed Jul 23, 2024

View reviewed changes

made some code changes

1364c22

VukW approved these changes Jul 24, 2024

View reviewed changes

sarthakpati approved these changes Jul 24, 2024

View reviewed changes

sarthakpati merged commit e36f274 into mlcommons:new-apis_v0.1.0-dev Jul 24, 2024
19 checks passed

github-actions bot locked and limited conversation to collaborators Jul 24, 2024

benmalef deleted the add_logging_final_version branch September 13, 2024 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging final version #893

Add logging final version #893

benmalef commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

sarthakpati left a comment •

edited

Loading

VukW commented Jul 3, 2024

sarthakpati commented Jul 3, 2024

benmalef commented Jul 3, 2024

benmalef commented Jul 4, 2024 •

edited

Loading

sarthakpati Jul 17, 2024

sarthakpati left a comment

sarthakpati Jul 19, 2024

benmalef Jul 19, 2024

sarthakpati Jul 19, 2024

benmalef Jul 19, 2024

benmalef commented Jul 19, 2024

sarthakpati left a comment

codecov bot commented Jul 20, 2024

VukW Jul 23, 2024

VukW Jul 23, 2024

VukW Jul 23, 2024

VukW left a comment

sarthakpati commented Jul 23, 2024

VukW left a comment •

edited

Loading

		log_file.write_text("Starting GaNDLF logging session \n")


		def _save_logs_in_file(log_file, config_path):

	def _save_logs_in_file(log_file, config_path):
	def _configure_logging_with_logfile(log_file, config_path):

Add logging final version #893

Add logging final version #893

Conversation

benmalef commented Jul 2, 2024 • edited Loading

Brief Description

Screenshots

Proposed Changes

Checklist

github-actions bot commented Jul 2, 2024 • edited Loading

sarthakpati left a comment • edited Loading

Choose a reason for hiding this comment

VukW commented Jul 3, 2024

sarthakpati commented Jul 3, 2024

benmalef commented Jul 3, 2024

benmalef commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

sarthakpati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmalef commented Jul 19, 2024

sarthakpati left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VukW left a comment

Choose a reason for hiding this comment

sarthakpati commented Jul 23, 2024

VukW left a comment • edited Loading

Choose a reason for hiding this comment

benmalef commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

sarthakpati left a comment •

edited

Loading

benmalef commented Jul 4, 2024 •

edited

Loading

VukW left a comment •

edited

Loading