Repository: huggingface/transformers
Pull request: Improve new failing test analysis for PR comment CI

# What does this PR do?

Improve new failing test analysis for PR comment CI

Changed files:
filename: .github/workflows/self-comment-ci.yml
patch: @@ -113,6 +113,7 @@ jobs:
         env:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           GITHUB_RUN_URL: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}
+          PREFIX: '[Workflow Run ⚙️](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})\n\n'
           github_repository: ${{ github.repository }}
           pr_number: ${{ needs.get-pr-number.outputs.PR_NUMBER }}
         run: |
@@ -121,7 +122,7 @@ jobs:
             -H "Accept: application/vnd.github+json" \
             -H "X-GitHub-Api-Version: 2022-11-28" \
             "repos/${github_repository}/issues/${pr_number}/comments" \
-            -f body="💔 This comment contains \`run-slow\`, but unknown error occurred and [the workflow run]($GITHUB_RUN_URL) aborted!"
+            -f body="$(echo -e "$PREFIX")💔 This comment contains \`run-slow\`, but unknown error occurred and [the workflow run]($GITHUB_RUN_URL) aborted!"
 
   reply_to_comment:
     name: Reply to the comment
@@ -134,6 +135,8 @@ jobs:
       - name: Reply to the comment
         env:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+          PREFIX: '[Workflow Run ⚙️](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})'
+          INFO: '\n\nThis comment contains `run-slow`, running the specified jobs'
           BODY: '\n\nmodels: ${{ needs.get-tests.outputs.models }}\nquantizations: ${{ needs.get-tests.outputs.quantizations }}'
           github_repository: ${{ github.repository }}
           pr_number: ${{ needs.get-pr-number.outputs.PR_NUMBER }}
@@ -143,7 +146,7 @@ jobs:
             -H "Accept: application/vnd.github+json" \
             -H "X-GitHub-Api-Version: 2022-11-28" \
             "repos/${github_repository}/issues/${pr_number}/comments" \
-            -f body="This comment contains \`run-slow\`, running the specified jobs: $(echo -e "$BODY")"
+            -f body="$(echo -e "$PREFIX")$(echo -e "$INFO"): $(echo -e "$BODY")"
 
   create_run:
     name: Create run
@@ -260,7 +263,7 @@ jobs:
                     # Count failures where commit is not None
                     total += sum(
                         1 for failure in failures 
-                        if isinstance(failure, dict) and failure.get('commit') is not None
+                        if isinstance(failure, dict) and failure.get('bad_commit') is not None
                     )
             return total
           
@@ -274,22 +277,26 @@ jobs:
                 model_lines = []
                 for device, failures in model_result.items():
                     
-                    # Filter out None commits and extract just the test names
-                    test_names = [
-                        failure['test'] 
-                        for failure in failures 
-                        if isinstance(failure, dict) and failure.get('commit') is not None
+                    filtered_failures = [
+                        failure for failure in failures 
+                        if isinstance(failure, dict) and failure.get('bad_commit') is not None
                     ]
 
                     # Add tests to model lines
-                    for idx, test_name in enumerate(test_names):
+                    for idx, failure in enumerate(filtered_failures):
                         if idx == 0:
-                            job_link = failures[idx]['job_link']
+                            job_link = failure['job_link']
                             model_lines.append(f"- [{model}]({job_link}):")
-          
-                        model_lines.append(f"    {test_name}")
-
-                # Only add model section if it has tests
+        
+                        test_name = failure['test']
+                        status = failure.get('status', '')
+                        if "DIFFERENT error message" in status:
+                            indicator = "(❌ ⟹ ❌)"
+                        else:
+                            indicator = "(✅ ⟹ ❌)"
+        
+                        model_lines.append(f"    {test_name} {indicator}")
+        
                 if len(model_lines) > 0:
                     lines.extend(model_lines)
                     lines.append("")  # Empty line between models
@@ -379,9 +386,9 @@ jobs:
             echo '### Commit Info'
             echo "| Context | Commit | Description |"
             echo "|---------|--------|-------------|"
-            echo "| RUN | ${run_commit_link} | merge commit |"
-            echo "| PR | ${pr_commit_link} | branch commit |"
-            echo "| main | ${main_commit_link} | base commit |"
+            echo "| RUN | ${run_commit_link} | workflow commit (merge commit) |"
+            echo "| PR | ${pr_commit_link} | branch commit (from PR) |"
+            echo "| main | ${main_commit_link} | base commit (on \`main\`) |"
             echo ''
 
             # Check infrastructure health - simple and clean!

filename: utils/check_bad_commit.py
patch: @@ -14,6 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import argparse
+import copy
 import json
 import os
 import re
@@ -106,7 +107,12 @@ def is_bad_commit(target_test, commit):
     if len(o) > 0:
         n_failed = int(o[0])
 
-    return result.returncode != 0, n_failed, n_passed
+    error_message = ""
+    if n_failed > 0:
+        match = re.search(r"^(FAILED .+ - .+)$", result.stdout, re.MULTILINE)
+        error_message = match.group(1).strip() if match else "Cannot retrieve error message."
+
+    return result.returncode != 0, n_failed, n_passed, error_message
 
 
 def find_bad_commit(target_test, start_commit, end_commit):
@@ -118,34 +124,90 @@ def find_bad_commit(target_test, start_commit, end_commit):
         end_commit (`str`): The earliest commit (exclusive).
 
     Returns:
-        `str`: The earliest commit at which `target_test` fails.
+        `dict`: A dict containing the info about the earliest commit at which `target_test` fails.
     """
+    result = {
+        "bad_commit": None,
+        "status": None,
+        "failure_at_workflow_commit": None,
+        "failure_at_base_commit": None,
+        "failure_at_bad_commit": None,
+    }
+
+    is_pr_ci = os.environ.get("GITHUB_EVENT_NAME") in ["issue_comment", "pull_request"]
+
+    # For PR comment CI, we "assume" all tests at `end_commit` passed, so any failing test during a PR CI run is
+    # "a new failing test", and we can perform more detailed checks with this script.
+    # For "a failing tes at start_commit", we check the test against `end_commit` (run multiple times):
+    #   - if all passing at end_commit: an actual new failing test at start_commit
+    #   - if all failing at end_commit: get the failure message and compare it against the one from start_commit:
+    #     - same failure message: not a new failing test --> don't report it
+    #      - different failure message: kind of a new failing test --> need to report it
+    #   - if both failing and passing at end_commit: mark it as flaky
 
     # check if `end_commit` fails the test
-    # (we only need one failure to conclude the test is flaky on the previous run with `end_commit`)
-    failed_before, _, _ = is_bad_commit(target_test, end_commit)
-    if failed_before:
-        return (
-            None,
-            f"flaky: test passed in the previous run (commit: {end_commit}) but failed (on the same commit) during the check of the current run.",
+    failed_before, n_failed, n_passed, failure_at_base_commit = is_bad_commit(target_test, end_commit)
+    # We only need one failure to conclude the test is flaky on the previous run with `end_commit`.
+    # However, when running on CI, we need at least one failure and one pass to conclude.
+    is_flaky_at_end_commit = ((not is_pr_ci) and n_failed > 0) or (is_pr_ci and n_failed > 0 and n_passed > 0)
+    # `n_passed == 0` itself is not enough, as the test may not exist in the codebase at `end_commit`.
+    is_failing_at_end_commit = failed_before and n_passed == 0
+
+    if is_flaky_at_end_commit:
+        result["status"] = (
+            f"flaky: test both passed and failed during the check of the current run on the previous commit: {end_commit}"
+        )
+        return result
+
+    elif (not is_pr_ci) and is_failing_at_end_commit:
+        result["status"] = (
+            f"flaky: test passed in the previous run (commit: {end_commit}) but failed (on the same commit) during the check of the current run."
         )
+        return result
 
     # if there is no new commit (e.g. 2 different CI runs on the same commit):
     #   - failed once on `start_commit` but passed on `end_commit`, which are the same commit --> flaky (or something change externally) --> don't report
     if start_commit == end_commit:
-        return (
-            None,
-            f"flaky: test fails on the current CI run but passed in the previous run which is running on the same commit {end_commit}.",
+        result["status"] = (
+            f"flaky: test fails on the current CI run but passed in the previous run which is running on the same commit {end_commit}."
         )
+        return result
 
-    # Now, we are (almost) sure `target_test` is not failing at `end_commit`
-    # check if `start_commit` fail the test
+    # Now, we are (almost) sure `target_test` is not failing at `end_commit`. (For a PR CI, it may fail at `end_commit`)
+    # Check if `start_commit` fails the test.
     # **IMPORTANT** we only need one pass to conclude the test is flaky on the current run with `start_commit`!
-    _, n_failed, n_passed = is_bad_commit(target_test, start_commit)
+    _, n_failed, n_passed, failure_at_workflow_commit = is_bad_commit(target_test, start_commit)
     if n_passed > 0:
         # failed on CI run, but not reproducible here --> don't report
-        return None, f"flaky: test fails on the current CI run (commit: {start_commit}) but passes during the check."
+        result["status"] = (
+            f"flaky: test fails on the current CI run (commit: {start_commit}) but passes during the check."
+        )
+        return result
+
+    # The test fails on `start_commit`, and
+    #   - if the CI is run on PR: this block checks if the test also failed on `start_commit`.
+    #   - otherwise: the test passed on `end_commit` --> an actual new failing test, this block is skipped.
+    if is_pr_ci and failure_at_base_commit != "" and failure_at_workflow_commit != failure_at_base_commit:
+        result["bad_commit"] = start_commit
+        result["status"] = (
+            f"test fails both on the current commit ({start_commit}) and the previous commit ({end_commit}), but with DIFFERENT error message!"
+        )
+        result["failure_at_workflow_commit"] = failure_at_workflow_commit
+        result["failure_at_base_commit"] = failure_at_base_commit
+        result["failure_at_bad_commit"] = failure_at_workflow_commit
+        return result
+    # Fail on both commits but with the same error message ==> don't include
+    elif is_pr_ci and failure_at_workflow_commit == failure_at_base_commit:
+        result["bad_commit"] = None
+        result["status"] = (
+            f"test fails both on the current commit ({start_commit}) and the previous commit ({end_commit}) with the SAME error message!"
+        )
+        result["failure_at_workflow_commit"] = failure_at_workflow_commit
+        result["failure_at_base_commit"] = failure_at_base_commit
+        result["failure_at_bad_commit"] = failure_at_workflow_commit
+        return result
 
+    # The test fails on `start_commit` but passed on `end_commit`.
     create_script(target_test=target_test)
 
     bash = f"""
@@ -157,31 +219,38 @@ def find_bad_commit(target_test, start_commit, end_commit):
     with open("run_git_bisect.sh", "w") as fp:
         fp.write(bash.strip())
 
-    result = subprocess.run(
+    bash_result = subprocess.run(
         ["bash", "run_git_bisect.sh"],
         check=False,
         capture_output=True,
         text=True,
     )
-    print(result.stdout)
+    print(bash_result.stdout)
 
     # This happens if running the script gives exit code < 0  or other issues
-    if "error: bisect run failed" in result.stderr:
-        error_msg = f"Error when running git bisect:\nbash error: {result.stderr}\nbash output:\n{result.stdout}\nset `bad_commit` to `None`."
+    if "error: bisect run failed" in bash_result.stderr:
+        error_msg = f"Error when running git bisect:\nbash error: {bash_result.stderr}\nbash output:\n{bash_result.stdout}\nset `bad_commit` to `None`."
         print(error_msg)
         return None, "git bisect failed"
 
     pattern = r"(.+) is the first bad commit"
-    commits = re.findall(pattern, result.stdout)
+    commits = re.findall(pattern, bash_result.stdout)
 
     bad_commit = None
+    failure_at_bad_commit = ""
     if len(commits) > 0:
         bad_commit = commits[0]
+        _, _, _, failure_at_bad_commit = is_bad_commit(target_test, bad_commit)
 
     print(f"Between `start_commit` {start_commit} and `end_commit` {end_commit}")
     print(f"bad_commit: {bad_commit}\n")
 
-    return bad_commit, "git bisect found the bad commit."
+    result["bad_commit"] = bad_commit
+    result["status"] = "git bisect found the bad commit."
+    result["failure_at_workflow_commit"] = failure_at_workflow_commit
+    result["failure_at_base_commit"] = failure_at_base_commit
+    result["failure_at_bad_commit"] = failure_at_bad_commit
+    return result
 
 
 def get_commit_info(commit, pr_number=None):
@@ -283,20 +352,30 @@ def get_commit_info(commit, pr_number=None):
 
             failed_tests_with_bad_commits = []
             for failure in failed_tests:
-                test, trace = failure["line"], failure["trace"]
-                commit, status = find_bad_commit(
+                test = failure["line"]
+                bad_commit_info = find_bad_commit(
                     target_test=test, start_commit=args.start_commit, end_commit=args.end_commit
                 )
-                info = {"test": test, "commit": commit, "status": status}
+                info = {"test": test}
+                info.update(bad_commit_info)
+
+                bad_commit = bad_commit_info["bad_commit"]
 
-                if commit in commit_info_cache:
-                    commit_info = commit_info_cache[commit]
+                if bad_commit in commit_info_cache:
+                    commit_info = commit_info_cache[bad_commit]
                 else:
-                    commit_info = get_commit_info(commit)
-                    commit_info_cache[commit] = commit_info
+                    commit_info = get_commit_info(bad_commit)
+                    commit_info_cache[bad_commit] = commit_info
+
+                commit_info_copied = copy.deepcopy(commit_info)
+                commit_info_copied.pop("commit")
+                commit_info_copied.update({"workflow_commit": args.start_commit, "base_commit": args.end_commit})
+                info.update(commit_info_copied)
+                # put failure message toward the end
+                info = {k: v for k, v in info.items() if not k.startswith(("failure_at_", "job_link"))} | {
+                    k: v for k, v in info.items() if k.startswith(("failure_at_", "job_link"))
+                }
 
-                info.update(commit_info)
-                info["trace"] = trace
                 failed_tests_with_bad_commits.append(info)
 
             # If no single-gpu test failures, remove the key

filename: utils/notification_service.py
patch: @@ -586,7 +586,13 @@ def payload(self) -> str:
         for idx, (prev_workflow_run_id, prev_ci_artifacts) in enumerate(
             [self.prev_ci_artifacts] + self.other_ci_artifacts
         ):
-            # To save the dict of new failures and upload to hub repositories
+            # `include_all` is `True` when the CI is running on a pull request, so it treats all failing tests
+            # in the current CI run as "new failing tests". The `utils/check_bad_commit.py`, run in a later job,
+            # will analyze the scenario in depth, in particular if a failing test in the current run is a new
+            # failing test, or already failed before but with the same/different failing reason.
+            include_all = os.environ.get("GITHUB_EVENT_NAME") in ["issue_comment", "pull_request"]
+            if include_all and prev_ci_artifacts is None:
+                prev_ci_artifacts = {}
             new_failures = self.get_new_failures(prev_ci_artifacts=prev_ci_artifacts)
             if new_failures:
                 filename = "new_failures"
@@ -1506,7 +1512,9 @@ def pop_default(l: list[Any], i: int, default: Any) -> Any:
             )
             other_workflow_run_ids.append(other_workflow_run_id)
     # triggered via `issue_comment` for CI on pull requests (e.g. using the comment `run-slow:`)
-    elif os.environ.get("GITHUB_EVENT_NAME") in ["issue_comment"]:
+    # TODO (ydshieh): We introduced `include_all` in `Message.payload` in PR # XXXXX, we no longer need this
+    #                 block. But let's keep this for a while and remove it when the CI runs become more stable.
+    elif os.environ.get("GITHUB_EVENT_NAME") in ["issue_comment", "pull_request"]:
         # TODO (ydshieh): Make this flexible once we implement `run-slow` for AMD CI and others.
         # The id of the workflow `.github/workflows/self-scheduled-caller.yml` (not of a workflow run of it).
         prev_workflow_id = "90575235"