[TRTLLM-13409][test] fail fast + surface server logs when a perf-sanity server dies or never becomes healthy - #16403
Conversation
📝 WalkthroughWalkthroughServer readiness utilities now detect exited processes, include server log tails in failures, ignore benign log markers, and support configurable readiness timeouts. Performance benchmark waits pass process and log context, with new unit coverage added to the A10 pre-merge list. ChangesServer readiness diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReadinessWait
participant ServerProcess
participant ServerLogs
participant ErrorReporter
ReadinessWait->>ServerProcess: poll liveness
ReadinessWait->>ServerLogs: inspect readiness logs
ServerLogs->>ErrorReporter: report error matches and tail
ErrorReporter-->>ReadinessWait: raise enriched failure
ServerProcess-->>ReadinessWait: report unexpected exit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_common/error_utils.py (1)
61-76: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCatch only log-reading errors.
This block only performs file I/O, so catching
Exceptioncan misclassify unrelated programming failures as unreadable logs. CatchOSErrorinstead.Proposed fix
- except Exception as e: + except OSError as e:As per coding guidelines, catch the narrowest possible exceptions instead of broad exceptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_common/error_utils.py` around lines 61 - 76, In the log-reading try/except around opening and scanning log_file, replace the broad Exception handler with an OSError handler. Preserve the existing fallback assignments and failure message for actual file I/O errors, while allowing unrelated programming exceptions to propagate.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 109-123: Update server_ready_timeout so the parsed
TRTLLM_TEST_SERVER_READY_TIMEOUT value is accepted only when greater than zero;
return the provided default for zero or negative values, preserving the existing
fallback and invalid-value handling.
In `@tests/test_common/error_utils.py`:
- Around line 19-21: Narrow the autotuner exemption by replacing the broad
BENIGN_LINE_MARKERS check in tests/test_common/error_utils.py:19-21 with a
predicate requiring both the expected autotuner probe message and OOM text;
update check_error() at tests/test_common/error_utils.py:33-34 and
report_error() at tests/test_common/error_utils.py:67-68 to use it. Add coverage
in tests/unittest/others/test_http_utils_fail_fast.py:121-131 confirming both
scanners still detect an autotuner-prefixed non-OOM error.
In `@tests/unittest/others/test_http_utils_fail_fast.py`:
- Around line 121-131: The existing autotuner test does not verify that genuine
errors with the autotuner marker remain detectable. Add
test_autotuner_marker_does_not_hide_real_errors in the test_check_error
coverage, using a log containing an “[Autotuner] RuntimeError: …” entry and
asserting check_error reports it, while preserving the benign warmup OOM
filtering behavior.
---
Outside diff comments:
In `@tests/test_common/error_utils.py`:
- Around line 61-76: In the log-reading try/except around opening and scanning
log_file, replace the broad Exception handler with an OSError handler. Preserve
the existing fallback assignments and failure message for actual file I/O
errors, while allowing unrelated programming exceptions to propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 635587bc-ae93-4e3d-ae48-479f3d96b030
📒 Files selected for processing (5)
tests/integration/defs/perf/test_perf_sanity.pytests/integration/test_lists/test-db/l0_a10.ymltests/test_common/error_utils.pytests/test_common/http_utils.pytests/unittest/others/test_http_utils_fail_fast.py
a484be3 to
434ef30
Compare
|
Approved to unblock this PR. |
434ef30 to
6a4b3d0
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59412 [ run ] triggered by Bot. Commit: |
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
LGTM — correctly fixes two real defects: the server-ready wait was passed DEFAULT_TIMEOUT=10800 (now bounded + env-overridable), and report_error scanned an already-exhausted handle after readlines() (now scans the buffered list). Masking risk is well-guarded — is_benign_line only suppresses [Autotuner]/OOM lines, the log tail is always appended even on a keyword hit, and there's a regression test covering it. Nice fail-fast improvement for the perf-sanity harness.
|
PR_Github #59412 [ run ] completed with state
|
c9e5b13 to
9c0e885
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59722 [ run ] triggered by Bot. Commit: |
|
PR_Github #59722 [ run ] completed with state
|
Agg and disagg servers have very different init times (disagg /health answers only once every ctx/gen worker is up), so a single override var could not extend one without the other. Add TRTLLM_TEST_AGG_SERVER_READY_TIMEOUT and TRTLLM_TEST_DISAGG_SERVER_READY_TIMEOUT, with the existing TRTLLM_TEST_SERVER_READY_TIMEOUT as a shared fallback. Addresses brnguyen2's review comment on PR NVIDIA#16403. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Agg and disagg servers have very different init times (disagg /health answers only once every ctx/gen worker is up), so a single override var could not extend one without the other. Add TRTLLM_TEST_AGG_SERVER_READY_TIMEOUT and TRTLLM_TEST_DISAGG_SERVER_READY_TIMEOUT, with the existing TRTLLM_TEST_SERVER_READY_TIMEOUT as a shared fallback. Addresses brnguyen2's review comment on PR NVIDIA#16403. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
fa46df7 to
18ada87
Compare
|
/bot run |
|
PR_Github #61780 [ run ] triggered by Bot. Commit: |
|
PR_Github #61780 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61861 [ run ] triggered by Bot. Commit: |
|
PR_Github #61861 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61932 [ run ] triggered by Bot. Commit: |
|
PR_Github #61932 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62067 [ run ] triggered by Bot. Commit: |
|
PR_Github #62067 [ run ] completed with state
|
|
Pipeline #50252 failed on a single stage,
Re-running CI. |
|
/bot run |
|
PR_Github #62109 [ run ] triggered by Bot. Commit: |
|
PR_Github #62109 [ run ] completed with state
|
|
Pipeline #50292 failed on
Note this is a different ambient failure from the previous run (#50252, the A100X |
|
/bot run |
|
PR_Github #62132 [ run ] triggered by Bot. Commit: |
|
PR_Github #62132 [ run ] completed with state
|
|
Pipeline #50311 failed on Not caused by this PR, on two independent grounds:
Re-running CI. |
|
/bot run |
|
PR_Github #62219 [ run ] triggered by Bot. Commit: |
|
Superseding pipeline #62219, which had been running ~11h with no verdict. #16978 has merged, so the |
|
/bot run |
|
PR_Github #62398 [ run ] triggered by Bot. Commit: |
|
PR_Github #62219 [ run ] completed with state |
|
PR_Github #62398 [ run ] completed with state |
6 participants
Summary by CodeRabbit
Bug Fixes
Tests
Description
Post-merge CI analysis of TRTLLM-13409 showed the dominant remaining hang-shaped burn is a disagg/perf-sanity server that dies or wedges during startup: the observed pattern is "trtllm-serve disaggregated launched, died before binding, and nobody noticed" -- the benchmark client polls /health (connection refused) until the 90-minute per-test pytest timeout, the CTX/GEN/DISAGG babysitter ranks wait only for the benchmark-done file without ever polling their server process, 8-36 GPUs are held, and nothing in the CI log shows the server-side story. Harness defects compound this:
Test Coverage
Adds CPU-only unit tests (registered in l0_a10) covering the fail-fast paths, the babysitter liveness helper, the benign-marker filter, the always-tail behavior, and the report_error regression; a NO_PROXY fixture keeps the dead-endpoint tests correct behind corporate HTTP proxies.
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)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.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.