[None][chore] Fix/disagg perf failure detection

[yingguo-trt]

Summary by CodeRabbit

• New Features

  • Added tracking and reporting of failed request counts in benchmark execution results
  • Benchmarks now automatically fail when requests fail, with detailed failure metrics (X/Y requests)

• Improvements

  • Enhanced error messages to include request failure details
  • Improved log parsing to extract and report failed/total request counts

Description

Test Coverage

PR Checklist

Please review the following before submitting your PR:

• PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

• PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

• Test cases are provided for new code paths (see test instructions)

• Any new dependencies have been scanned for license and vulnerabilities

• CODEOWNERS updated if ownership changes

• Documentation updated as needed

• Update tava architecture diagram if there is a significant design change in PR.

• The reviewers assigned automatically/manually are appropriate for the PR.

• Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes add tracking and reporting of failed requests to the benchmark execution pipeline. The report parser now extracts failed and total request counts from log files. The executor uses these metrics to conditionally override benchmark results to FAILED status with descriptive error messages when requests fail.

Changes

Cohort / File(s)	Summary
Report Parsing tests/integration/defs/perf/disagg/reporting/report.py	Added extraction of failed and total request counts from log content via regex matching. Return dictionary now includes failed_requests and total_requests fields for both missing log and successful parse cases.
Execution Status Override tests/integration/defs/perf/disagg/execution/executor.py	LogParser results now used to derive failed_requests and total_requests. Benchmark status conditionally overridden to FAILED if requests failed, with error message augmentation including request failure counts.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description contains only the template with all sections unfilled (Description, Test Coverage, and PR Checklist sections are empty or uncompleted). Required information about the changes and testing is missing.	Fill in the Description section explaining the failure detection changes, list relevant tests in Test Coverage, and complete the PR Checklist items appropriately.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title '[None][chore] Fix/disagg perf failure detection' is specific and directly reflects the main changes: adding failure detection for disaggregated performance runs with error directory suffixes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/integration/defs/perf/disagg/execution/executor.py (1)

735-742: Set an explicit terminal status on parse failure.

At Line 742, this returns with default status="UNKNOWN". Setting status="FAILED" (or "ERROR") here would make downstream reporting less ambiguous.

💡 Proposed tweak

         if not parse_result["status"]:
             error_msg = "Failed to parse benchmark logs"
             if failed_requests > 0:
                 error_msg += f" ({failed_requests}/{total_requests} requests failed)"
+            result["status"] = "FAILED"
             result["error"] = error_msg
             return result

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/defs/perf/disagg/execution/executor.py` around lines 735 -
742, When parse_result indicates failure (parse_result["status"] is falsy) and
you build the error message into result["error"], also set an explicit terminal
status (e.g., result["status"] = "FAILED" or "ERROR") so downstream reporting
isn't left as the default "UNKNOWN"; update the block that checks
parse_result["status"] (the same block that computes
failed_requests/total_requests and sets result["error"]) to assign
result["status"] = "FAILED" (or "ERROR") alongside setting result["error"] to
make the final state explicit.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/integration/defs/perf/disagg/reporting/report.py`:
- Around line 93-97: The code currently only assigns failed_requests and
total_requests when both re.search calls match, using the first match; instead,
parse each counter independently and pick the latest occurrence (use re.findall
instead of re.search or iterate over re.finditer) so you set failed_requests
from the last match of the "Total failed requests" pattern and total_requests
from the last match of the "Total requests" pattern, and handle missing values
explicitly (e.g., default to 0 or raise/log a clear error) so downstream logic
using failed_requests/total_requests is correct; update references to
failed_match/total_match and the assignments to failed_requests/total_requests
accordingly.

---

Nitpick comments:
In `@tests/integration/defs/perf/disagg/execution/executor.py`:
- Around line 735-742: When parse_result indicates failure
(parse_result["status"] is falsy) and you build the error message into
result["error"], also set an explicit terminal status (e.g., result["status"] =
"FAILED" or "ERROR") so downstream reporting isn't left as the default
"UNKNOWN"; update the block that checks parse_result["status"] (the same block
that computes failed_requests/total_requests and sets result["error"]) to assign
result["status"] = "FAILED" (or "ERROR") alongside setting result["error"] to
make the final state explicit.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2250f97d-b5ce-4b3e-a3a5-ed53c8c6eab2

📥 Commits

Reviewing files that changed from the base of the PR and between 3768b35 and cee629d.

📒 Files selected for processing (2)

• tests/integration/defs/perf/disagg/execution/executor.py
• tests/integration/defs/perf/disagg/reporting/report.py

[yingguo-trt]

/bot run --stage-list ""

[tensorrt-cicd]

PR_Github #37675 [ run ] triggered by Bot. Commit: 2eb759c Link to invocation

[fredricz-20070104]

Okay, it's used to detect the failed requests.

[tensorrt-cicd]

PR_Github #37675 [ run ] completed with state SUCCESS. Commit: 2eb759c
/LLM/release-1.2/L0_MergeRequest_PR pipeline #458 (Partly Tested) completed with status: 'SUCCESS'

Link to invocation

[yingguo-trt]

/bot reuse-pipeline

[tensorrt-cicd]

PR_Github #37783 [ reuse-pipeline ] triggered by Bot. Commit: 2eb759c Link to invocation

[tensorrt-cicd]

PR_Github #37783 [ reuse-pipeline ] completed with state SUCCESS. Commit: 2eb759c
Reusing PR_Github #37675 (Partly Tested) for commit 2eb759c

Link to invocation