🧼 Refactor log_reports.py for Improved Logging, File Processing, an…

…d Slack Payload Handling (#2249)

* Update log_reports.py

* comments text update

Co-authored-by: Quentin Gallouédec <[email protected]>

* emoji added

Co-authored-by: Quentin Gallouédec <[email protected]>

* Update scripts/log_reports.py

Co-authored-by: Quentin Gallouédec <[email protected]>

* Update scripts/log_reports.py

* style

* Update scripts/log_reports.py

---------

Co-authored-by: Quentin Gallouédec <[email protected]>
Co-authored-by: Quentin Gallouédec <[email protected]>

scripts/log_reports.py

@@ -21,142 +21,146 @@
 from tabulate import tabulate
 
 
-MAX_LEN_MESSAGE = 2900  # slack endpoint has a limit of 3001 characters
+MAX_LEN_MESSAGE = 2900  # Slack endpoint has a limit of 3001 characters
 
 parser = argparse.ArgumentParser()
 parser.add_argument("--slack_channel_name", default="trl-push-ci")
 
 # Set up logging
-logging.basicConfig(level=logging.INFO)
+logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(levelname)s - %(message)s")
 
 
-def main(slack_channel_name=None):
-    failed = []
-    passed = []
+def process_log_file(log):
+    failed_tests = []
+    passed_tests = []
+    section_num_failed = 0
 
-    group_info = []
+    try:
+        with open(log) as f:
+            for line in f:
+                try:
+                    data = json.loads(line)
+                    test_name = data.get("nodeid", "")
+                    duration = f'{data["duration"]:.4f}' if "duration" in data else "N/A"
+                    outcome = data.get("outcome", "")
 
-    total_num_failed = 0
-    empty_file = False or len(list(Path().glob("*.log"))) == 0
+                    if test_name:
+                        if outcome == "failed":
+                            section_num_failed += 1
+                            failed_tests.append([test_name, duration, log.stem.split("_")[0]])
+                        else:
+                            passed_tests.append([test_name, duration, log.stem.split("_")[0]])
+                except json.JSONDecodeError as e:
+                    logging.warning(f"Could not decode line in {log}: {e}")
+
+    except FileNotFoundError as e:
+        logging.error(f"Log file {log} not found: {e}")
+    except Exception as e:
+        logging.error(f"Error processing log file {log}: {e}")
+
+    return failed_tests, passed_tests, section_num_failed
 
+
+def main(slack_channel_name):
+    group_info = []
+    total_num_failed = 0
     total_empty_files = []
 
-    for log in Path().glob("*.log"):
-        section_num_failed = 0
-        i = 0
-        try:  # Added error handling for file operations
-            with open(log) as f:
-                for line in f:
-                    line = json.loads(line)
-                    i += 1
-                    if line.get("nodeid", "") != "":
-                        test = line["nodeid"]
-                        if line.get("duration", None) is not None:
-                            duration = f'{line["duration"]:.4f}'
-                            if line.get("outcome", "") == "failed":
-                                section_num_failed += 1
-                                failed.append([test, duration, log.name.split("_")[0]])
-                                total_num_failed += 1
-                            else:
-                                passed.append([test, duration, log.name.split("_")[0]])
-            empty_file = i == 0
-        except Exception as e:  # Catch any exceptions during file processing
-            logging.error(f"Error processing log file {log}: {e}")
-        else:
-            group_info.append([str(log), section_num_failed, failed])
-            total_empty_files.append(empty_file)
-        finally:
+    log_files = list(Path().glob("*.log"))
+    if not log_files:
+        logging.info("No log files found.")
+        return
+
+    for log in log_files:
+        failed, passed, section_num_failed = process_log_file(log)
+        empty_file = not failed and not passed
+
+        total_num_failed += section_num_failed
+        total_empty_files.append(empty_file)
+        group_info.append([str(log), section_num_failed, failed])
+
+        # Clean up log file
+        try:
             os.remove(log)
-            failed = []
-
-    no_error_payload = {
-        "type": "section",
-        "text": {
-            "type": "plain_text",
-            "text": "🌞 There were no failures!"
-            if not any(total_empty_files)
-            else "Something went wrong there is at least one empty file - please check GH action results.",
-            "emoji": True,
-        },
-    }
+        except OSError as e:
+            logging.warning(f"Could not remove log file {log}: {e}")
 
-    message = ""
+    # Prepare Slack message payload
     payload = [
         {
             "type": "header",
-            "text": {
-                "type": "plain_text",
-                "text": "🤗 Results of the {} TRL tests.".format(os.environ.get("TEST_TYPE", "")),
-            },
+            "text": {"type": "plain_text", "text": f"🤗 Results of the {os.environ.get('TEST_TYPE', '')} TRL tests."},
         },
     ]
+
     if total_num_failed > 0:
-        for i, (name, num_failed, failed_tests) in enumerate(group_info):
+        message = ""
+        for name, num_failed, failed_tests in group_info:
             if num_failed > 0:
-                if num_failed == 1:
-                    message += f"*{name}: {num_failed} failed test*\n"
-                else:
-                    message += f"*{name}: {num_failed} failed tests*\n"
-                failed_table = []
-                for test in failed_tests:
-                    failed_report = test[0].split("::")
-                    # Truncate the last string as some test names might be long
-                    failed_report[-1] = failed_report[-1][:30] + ".."
-                    failed_table.append(failed_report)
-                failed_table = tabulate(
-                    failed_table,
-                    headers=["Test Location", "Test Case", "Test Name"],
-                    showindex="always",
-                    tablefmt="grid",
-                    maxcolwidths=[12, 12, 12],
+                message += f"*{name}: {num_failed} failed test(s)*\n"
+                failed_table = [
+                    test[0].split("::")[:2] + [test[0].split("::")[-1][:30] + ".."] for test in failed_tests
+                ]
+                message += (
+                    "\n```\n"
+                    + tabulate(failed_table, headers=["Test Location", "Test Name"], tablefmt="grid")
+                    + "\n```\n"
                 )
-                message += "\n```\n" + failed_table + "\n```"
 
-            if total_empty_files[i]:
-                message += f"\n*{name}: Warning! Empty file - please check the GitHub action job *\n"
-        logging.info(f"Total failed tests: {total_num_failed}")  # Log the total failed tests
-        print(f"### {message}")
-    else:
-        payload.append(no_error_payload)
+            if any(total_empty_files):
+                message += f"\n*{name}: Warning! Empty file - check GitHub action job*\n"
 
-    if os.environ.get("TEST_TYPE", "") != "":
-        from slack_sdk import WebClient
+        # Logging
+        logging.info(f"Total failed tests: {total_num_failed}")
+        print(f"### {message}")
 
         if len(message) > MAX_LEN_MESSAGE:
-            message = f"There are {total_num_failed} failed tests in total ! Cannot display the entire summary - please check the action results directly"
+            message = (
+                f"❌ There are {total_num_failed} failed tests in total! Please check the action results directly."
+            )
 
-        if len(message) != 0:
-            md_report = {
-                "type": "section",
-                "text": {"type": "mrkdwn", "text": message},
-            }
-            payload.append(md_report)
-            action_button = {
+        payload.append({"type": "section", "text": {"type": "mrkdwn", "text": message}})
+        payload.append(
+            {
                 "type": "section",
                 "text": {"type": "mrkdwn", "text": "*For more details:*"},
                 "accessory": {
                     "type": "button",
-                    "text": {"type": "plain_text", "text": "Check Action results", "emoji": True},
+                    "text": {"type": "plain_text", "text": "Check Action results"},
                     "url": f"https://github.com/huggingface/trl/actions/runs/{os.environ['GITHUB_RUN_ID']}",
                 },
             }
-            payload.append(action_button)
+        )
+        payload.append(
+            {
+                "type": "context",
+                "elements": [
+                    {
+                        "type": "plain_text",
+                        "text": f"On Push main {os.environ.get('TEST_TYPE')} results for {date.today()}",
+                    }
+                ],
+            }
+        )
 
-        date_report = {
-            "type": "context",
-            "elements": [
-                {
-                    "type": "plain_text",
-                    "text": f"On Push main {os.environ.get('TEST_TYPE')} test results for {date.today()}",
-                },
-            ],
-        }
-        payload.append(date_report)
+        # Send to Slack
+        from slack_sdk import WebClient
 
-        print(payload)
+        slack_client = WebClient(token=os.environ.get("SLACK_API_TOKEN"))
+        slack_client.chat_postMessage(channel=f"#{slack_channel_name}", text=message, blocks=payload)
 
-        client = WebClient(token=os.environ.get("SLACK_API_TOKEN"))
-        client.chat_postMessage(channel=f"#{slack_channel_name}", text=message, blocks=payload)
+    else:
+        payload.append(
+            {
+                "type": "section",
+                "text": {
+                    "type": "plain_text",
+                    "text": "✅ No failures! All tests passed successfully.",
+                    "emoji": True,
+                },
+            }
+        )
+        logging.info("All tests passed. No errors detected.")
 
 
 if __name__ == "__main__":