[TRTLLM-13409][fix] bound the perf-sanity harness so a stalled stage fails on its own - #17298
[TRTLLM-13409][fix] bound the perf-sanity harness so a stalled stage fails on its own#17298JunyiXu-nv wants to merge 1 commit into
Conversation
|
/bot run |
WalkthroughThe perf sanity integration now bounds benchmark-client execution and server shutdown. It captures partial output and server diagnostics on timeout. New regression tests cover configuration, termination, output, exit errors, and diagnostic failures. ChangesPerf sanity execution bounds
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BenchmarkPath
participant run_benchmark_client
participant ServerProcess
participant ServerLogs
BenchmarkPath->>run_benchmark_client: Start benchmark client with timeout
run_benchmark_client->>ServerLogs: Inspect server errors and log tail
run_benchmark_client-->>BenchmarkPath: Return output or raise diagnostic error
BenchmarkPath->>ServerProcess: Stop with SIGTERM
ServerProcess-->>BenchmarkPath: Exit or receive SIGKILL
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (3)
tests/integration/defs/perf/test_perf_sanity.py (2)
143-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the remaining parameters.
proc,cmd,env, andserver_logshave no type annotations. Usesubprocess.Popen[bytes],list[str],Mapping[str, str], andSequence[str] | None.As per coding guidelines: "Annotate every function, use
Nonefor procedures, avoid unnecessaryAnyandtype: ignore, prefer built-in generic types and|".Also applies to: 165-165
🤖 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/integration/defs/perf/test_perf_sanity.py` at line 143, Annotate the remaining parameters in stop_process and the related function at the referenced location: use subprocess.Popen[bytes] for proc, list[str] for cmd, Mapping[str, str] for env, and Sequence[str] | None for server_logs. Preserve the existing None return annotations and import any required typing symbols.Source: Coding guidelines
143-162: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTerminate the full subprocess tree during teardown.
trtllm-servecan create worker descendants, butstop_process()signals only the direct child. Usecleanup_process_tree()fromtests/integration/defs/trt_test_alternative.pyfor server teardown. Apply the same cleanup to the timeout path inrun_benchmark_client()when the client can create descendants.🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 143 - 162, Update stop_process() to use cleanup_process_tree() so SIGTERM/SIGKILL teardown reaches the server process and all descendants, preserving the existing grace-period behavior. Also replace direct client termination in run_benchmark_client()’s timeout path with cleanup_process_tree() when the client may create descendants.Source: Learnings
tests/unittest/others/test_perf_sanity_bounds.py (1)
127-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the partial client output section.
The timeout message ends with
--- last client output ---and the last 4000 characters of client stdout. No test asserts that a client which prints before it hangs keeps that output in the failure message. That section is the part that shows how far the run got, so a regression there would be silent.A client such as
print('phase 1', flush=True); time.sleep(120)covers it.🤖 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/unittest/others/test_perf_sanity_bounds.py` around lines 127 - 149, Extend test_client_hang_is_bounded_and_names_the_knob to run a hanging client that emits identifiable flushed output before sleeping, then assert the RuntimeError message contains that output after the --- last client output --- section. Preserve the existing timeout, environment-variable, server-error, and prompt-bound assertions.
🤖 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 206-212: Update the log-tail collection around the existing
open/read block to use collections.deque with maxlen=SERVER_LOG_TAIL_LINES,
importing deque with the standard-library imports. Iterate through the file and
retain only the final configured number of lines, preserving the existing
OSError handling and tail formatting.
In `@tests/unittest/others/test_perf_sanity_bounds.py`:
- Around line 46-49: Update test_client_timeout_defaults_to_one_hour to remove
the timeout environment variable with pytest’s monkeypatch fixture via
monkeypatch.delenv, preserving any pre-existing value after the test instead of
mutating os.environ directly.
---
Nitpick comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Line 143: Annotate the remaining parameters in stop_process and the related
function at the referenced location: use subprocess.Popen[bytes] for proc,
list[str] for cmd, Mapping[str, str] for env, and Sequence[str] | None for
server_logs. Preserve the existing None return annotations and import any
required typing symbols.
- Around line 143-162: Update stop_process() to use cleanup_process_tree() so
SIGTERM/SIGKILL teardown reaches the server process and all descendants,
preserving the existing grace-period behavior. Also replace direct client
termination in run_benchmark_client()’s timeout path with cleanup_process_tree()
when the client may create descendants.
In `@tests/unittest/others/test_perf_sanity_bounds.py`:
- Around line 127-149: Extend test_client_hang_is_bounded_and_names_the_knob to
run a hanging client that emits identifiable flushed output before sleeping,
then assert the RuntimeError message contains that output after the --- last
client output --- section. Preserve the existing timeout, environment-variable,
server-error, and prompt-bound assertions.
🪄 Autofix
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: 39b76602-bb68-49b8-aaea-c43884a8cf70
📒 Files selected for processing (2)
tests/integration/defs/perf/test_perf_sanity.pytests/unittest/others/test_perf_sanity_bounds.py
|
PR_Github #63989 [ run ] triggered by Bot. Commit: |
|
PR_Github #63989 [ run ] completed with state
|
…fails on its own Perf-sanity stages could not fail by themselves. The benchmark client ran under subprocess.check_output() with no timeout, and the file-rendezvous polls are bounded by DEFAULT_TIMEOUT (10800s), above the pytest per-test marker. NVIDIA#16403 bounded the /health readiness wait (30 min agg, 60 min disagg), but that is the startup phase; nothing bounded the steady-state benchmark run after /health had answered. A stall there surfaced only as 'the client is still running' until Slurm or Jenkins killed the stage, with no results XML and no diagnostic. Measured over the five days after NVIDIA#16961: 13 disagg hangs / 143 GPU-h, of which 115 GPU-h were multi-hour stages with no identified cause -- 1.7-2.3h runtimes against 18-41 minute budgets. Those exceed the 60-minute readiness bound, so they are in the client phase. - run_benchmark_client() runs the client under a deadline (default 3600s, TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SEC to change, 0 disables). On expiry it kills the client and raises naming the elapsed time and the knob, with server-side keyword hits, a tail of each server log, and the partial client output attached. check_output semantics are otherwise preserved. Applied to both the aggregated and disaggregated client paths. - stop_process() replaces the bare terminate(); wait() at the three server teardown sites, escalating to SIGKILL after a grace period. A rank wedged in a non-interruptible native call never runs the Python signal handler. - The timeout path attaches a raw log tail as well as the check_error() keyword scan, because ERROR_KEYWORDS are Python exception names and do not match '[TRT-LLM] [E]' lines. ERROR_KEYWORDS is deliberately left alone: it also drives wait_for_endpoint_ready()'s fast-fail, where a benign [E] during startup would begin failing healthy runs. The tail streams via deque(maxlen=N) rather than readlines(). Measured perf-sanity gen logs reach 77-232 MB, so materialising the whole file risked an OOM on the rank that is already failing, swallowing the very timeout report being assembled. This bounds the stage regardless of why it stalled; it does not diagnose or fix any particular hang. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
e95cbfa to
8605b3a
Compare
|
/bot run |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Line 1571: Initialize server_proc before its try block and guard the
corresponding stop_process cleanup so it runs only when a process was created;
apply the same change to disagg_server_proc and its finally cleanup in
tests/integration/defs/perf/test_perf_sanity.py at lines 1571-1571 and
1599-1599.
- Line 141: Update the timeout normalization logic in
tests/integration/defs/perf/test_perf_sanity.py at lines 141-141 so negative
values are invalid and fall back to DEFAULT_BENCHMARK_CLIENT_TIMEOUT, while
preserving 0 as the only value that disables the deadline. Add a regression test
in tests/unittest/others/test_perf_sanity_bounds.py at lines 65-68 asserting
that a negative timeout does not disable the benchmark-client deadline.
- Around line 128-141: Complete the Python 3.10+ annotations for the new
performance sanity helpers and tests: in
tests/integration/defs/perf/test_perf_sanity.py lines 128-141, change
_benchmark_client_timeout to return int | None; at lines 144-164, annotate the
process parameter as subprocess.Popen[bytes]; at lines 166-235, annotate cmd as
Sequence[str], env as dict[str, str], and server_logs as list[str] | None. In
tests/unittest/others/test_perf_sanity_bounds.py lines 46-189, add -> None to
every test function and annotate fixture parameters with MonkeyPatch and Path,
importing those types as needed.
🪄 Autofix
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: e10f96c1-7a52-4235-99e9-5a5ac1be4683
📒 Files selected for processing (2)
tests/integration/defs/perf/test_perf_sanity.pytests/unittest/others/test_perf_sanity_bounds.py
| def _benchmark_client_timeout() -> Optional[int]: | ||
| """Effective client bound in seconds, or None when explicitly disabled.""" | ||
| raw = os.environ.get(BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME) | ||
| if raw is None: | ||
| return DEFAULT_BENCHMARK_CLIENT_TIMEOUT | ||
| try: | ||
| value = int(raw) | ||
| except ValueError: | ||
| print_info( | ||
| f"{BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME}={raw!r} is not an integer; " | ||
| f"falling back to {DEFAULT_BENCHMARK_CLIENT_TIMEOUT}s" | ||
| ) | ||
| return DEFAULT_BENCHMARK_CLIENT_TIMEOUT | ||
| return None if value <= 0 else value |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n \
'^\s*def (_benchmark_client_timeout|stop_process|run_benchmark_client|test_)' \
tests/integration/defs/perf/test_perf_sanity.py \
tests/unittest/others/test_perf_sanity_bounds.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 1997
🏁 Script executed:
# Extract the function signatures and immediate context to verify annotations
sed -n '128,141p' tests/integration/defs/perf/test_perf_sanity.py
echo "---"
sed -n '144,164p' tests/integration/defs/perf/test_perf_sanity.py
echo "---"
sed -n '166,180p' tests/integration/defs/perf/test_perf_sanity.py
echo "---"
# Check imports at the top of both files
head -20 tests/integration/defs/perf/test_perf_sanity.py
echo "=== UNITTEST FILE ==="
head -20 tests/unittest/others/test_perf_sanity_bounds.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 4192
🏁 Script executed:
# Get full import block for integration test file
sed -n '1,50p' tests/integration/defs/perf/test_perf_sanity.py | grep -E '(^import|^from)'
echo "---"
# Get full import block for unittest file
sed -n '1,50p' tests/unittest/others/test_perf_sanity_bounds.py | grep -E '(^import|^from)'
echo "---"
# Check test function signatures in unittest file
sed -n '46,70p' tests/unittest/others/test_perf_sanity_bounds.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 2221
🏁 Script executed:
# Check how parameters are used to determine correct types
# For cmd, env, server_logs - look at their usage in run_benchmark_client
sed -n '166,185p' tests/integration/defs/perf/test_perf_sanity.py | grep -E '(cmd|env|server_logs)'
echo "---"
# For proc - check usage in stop_process
sed -n '144,164p' tests/integration/defs/perf/test_perf_sanity.py | grep -E 'proc\.'
echo "---"
# Check test invocations in unittest to see what types are passed
sed -n '114,150p' tests/unittest/others/test_perf_sanity_bounds.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 1933
🏁 Script executed:
# Check for imports of typing modules and how fixtures are used
sed -n '1,30p' tests/unittest/others/test_perf_sanity_bounds.py
echo "---"
# Check if there are other test functions with annotations in unittest
sed -n '76,110p' tests/unittest/others/test_perf_sanity_bounds.py
echo "---"
# Look for pathlib usage
grep -n "pathlib\|Path" tests/unittest/others/test_perf_sanity_bounds.py | head -10Repository: NVIDIA/TensorRT-LLM
Length of output: 2914
Add complete annotations to all new functions using Python 3.10+ syntax.
All three helper functions and test functions lack required type annotations per coding guidelines.
tests/integration/defs/perf/test_perf_sanity.py#L128: ChangeOptional[int]toint | None.tests/integration/defs/perf/test_perf_sanity.py#L144: Addproc: subprocess.Popen[bytes].tests/integration/defs/perf/test_perf_sanity.py#L166: Addcmd: Sequence[str],env: dict[str, str],server_logs: list[str] | None.tests/unittest/others/test_perf_sanity_bounds.py#L46-189: Add-> Noneto all test functions. Annotate fixture parameters:monkeypatch: MonkeyPatch(from_pytest.monkeypatch),tmp_path: Path(frompathlib).
📍 Affects 2 files
tests/integration/defs/perf/test_perf_sanity.py#L128-L141(this comment)tests/integration/defs/perf/test_perf_sanity.py#L144-L164tests/integration/defs/perf/test_perf_sanity.py#L166-L235tests/unittest/others/test_perf_sanity_bounds.py#L46-L189
🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 128 - 141,
Complete the Python 3.10+ annotations for the new performance sanity helpers and
tests: in tests/integration/defs/perf/test_perf_sanity.py lines 128-141, change
_benchmark_client_timeout to return int | None; at lines 144-164, annotate the
process parameter as subprocess.Popen[bytes]; at lines 166-235, annotate cmd as
Sequence[str], env as dict[str, str], and server_logs as list[str] | None. In
tests/unittest/others/test_perf_sanity_bounds.py lines 46-189, add -> None to
every test function and annotate fixture parameters with MonkeyPatch and Path,
importing those types as needed.
Source: Coding guidelines
| f"falling back to {DEFAULT_BENCHMARK_CLIENT_TIMEOUT}s" | ||
| ) | ||
| return DEFAULT_BENCHMARK_CLIENT_TIMEOUT | ||
| return None if value <= 0 else value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject negative benchmark-client timeouts.
Only 0 is documented to disable the deadline. Line 141 also treats negative values as disabled. A value such as -1 can restore an unbounded client wait.
tests/integration/defs/perf/test_perf_sanity.py#L141-L141: treat negative values as invalid and fall back toDEFAULT_BENCHMARK_CLIENT_TIMEOUT.tests/unittest/others/test_perf_sanity_bounds.py#L65-L68: add a regression test that asserts a negative value does not disable the deadline.
Proposed fix
except ValueError:
print_info(
f"{BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME}={raw!r} is not an integer; "
f"falling back to {DEFAULT_BENCHMARK_CLIENT_TIMEOUT}s"
)
return DEFAULT_BENCHMARK_CLIENT_TIMEOUT
- return None if value <= 0 else value
+ if value < 0:
+ print_info(
+ f"{BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME}={raw!r} is negative; "
+ f"falling back to {DEFAULT_BENCHMARK_CLIENT_TIMEOUT}s"
+ )
+ return DEFAULT_BENCHMARK_CLIENT_TIMEOUT
+ return None if value == 0 else value📍 Affects 2 files
tests/integration/defs/perf/test_perf_sanity.py#L141-L141(this comment)tests/unittest/others/test_perf_sanity_bounds.py#L65-L68
🤖 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/integration/defs/perf/test_perf_sanity.py` at line 141, Update the
timeout normalization logic in tests/integration/defs/perf/test_perf_sanity.py
at lines 141-141 so negative values are invalid and fall back to
DEFAULT_BENCHMARK_CLIENT_TIMEOUT, while preserving 0 as the only value that
disables the deadline. Add a regression test in
tests/unittest/others/test_perf_sanity_bounds.py at lines 65-68 asserting that a
negative timeout does not disable the benchmark-client deadline.
| print_info(f"Server {self.disagg_serving_type} stopped") | ||
| server_proc.terminate() | ||
| server_proc.wait() | ||
| stop_process(server_proc, "server") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Initialize process locals before entering try.
If open() or subprocess.Popen() fails, these finally blocks reference an unassigned local and mask the server-start failure with UnboundLocalError.
tests/integration/defs/perf/test_perf_sanity.py#L1571-L1571: initializeserver_proc = Nonebeforetry, then callstop_processonly when it is assigned.tests/integration/defs/perf/test_perf_sanity.py#L1599-L1599: initializedisagg_server_proc = Nonebeforetry, then callstop_processonly when it is assigned.
📍 Affects 1 file
tests/integration/defs/perf/test_perf_sanity.py#L1571-L1571(this comment)tests/integration/defs/perf/test_perf_sanity.py#L1599-L1599
🤖 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/integration/defs/perf/test_perf_sanity.py` at line 1571, Initialize
server_proc before its try block and guard the corresponding stop_process
cleanup so it runs only when a process was created; apply the same change to
disagg_server_proc and its finally cleanup in
tests/integration/defs/perf/test_perf_sanity.py at lines 1571-1571 and
1599-1599.
|
PR_Github #64030 [ run ] triggered by Bot. Commit: |
|
PR_Github #64030 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The harness change is right and the diagnostics you attach on expiry are the ones a triager actually needs. Main thing to fix before merge is the test wiring — as written, test_perf_sanity_bounds.py doesn't run anywhere in CI, so the bounds it pins are unprotected.
On the fixed 3600s: it isn't derived from anything the test declares. AggrTestCmds/the disagg cmds already carry self.timeout, and the PR description says real budgets are 18–41 min, so a stalled stage still overruns its own budget by up to 40 minutes before the bound fires. min(self.timeout, _benchmark_client_timeout()) — the same pattern already used for the readiness wait at test_perf_sanity.py:1241 — would make the bound track the test instead of a constant. Not a blocker, but worth considering while you're here.
| if _INTEGRATION not in sys.path: | ||
| sys.path.insert(0, os.path.abspath(_INTEGRATION)) | ||
|
|
||
| perf_sanity = pytest.importorskip("defs.perf.test_perf_sanity") |
There was a problem hiding this comment.
Two problems that together mean this file never runs in CI:
- It's not in any
tests/integration/test_lists/test-db/*.yml. The sibling file from [TRTLLM-13409][test] fail fast + surface server logs when a perf-sanity server dies or never becomes healthy #16403,unittest/others/test_http_utils_fail_fast.py, is listed inl0_cpu.yml:47— add this one there too. - It's missing
pytestmark = pytest.mark.cpu_only. That sibling carries the marker with an explicit comment: the CPU-Generic stages select with-m cpu_only, so without it every test here is deselected and pytest exits 5.
Separately, importorskip is the wrong guard for a regression test. defs.perf.test_perf_sanity imports ..conftest, tensorrt_llm._utils, yaml, and test_common.* at module scope; if any of that breaks the whole file turns into a silent skip and the bounds you're pinning go unprotected while CI stays green. Import it directly (from defs.perf import test_perf_sanity) so a broken import is a failure.
| # also drives wait_for_endpoint_ready()'s fast-fail, where a benign [E] | ||
| # during startup would start failing healthy runs. Keyword hits are | ||
| # best-effort; a raw tail is always attached. | ||
| keyword_hits = [] |
There was a problem hiding this comment.
keyword_hits is an unbounded list, but only the last 20 entries are ever used (keyword_hits[-20:]). The comment nine lines below explains that gen logs reach 77–232 MB and that materialising them risks an OOM on the already-failing rank — a log wedged in a retry loop emitting ConnectionRefusedError/TimeoutError per line is exactly the case that produces millions of hits here. Use deque(maxlen=20) for the same reason you used it for the tail.
| return None if value <= 0 else value | ||
|
|
||
|
|
||
| def stop_process(proc, name: str, grace: int = SERVER_TERMINATE_GRACE_SEC) -> None: |
There was a problem hiding this comment.
SIGKILL on server_proc reaches only the launcher. trtllm-serve forks MPI worker ranks for TP>1, and those don't get the signal — the old terminate(); wait() at least gave the parent a chance to reap them. The trade (a guaranteed hang for possible GPU-holding orphans) is probably still right inside a Slurm step whose cgroup gets cleaned up, but for a multi-client / multi-server-index test the next iteration runs in the same allocation and will hit "device in use".
Cheap fix: pass start_new_session=True at the three Popen sites (1232, 1558, 1586) and os.killpg(os.getpgid(proc.pid), SIGKILL) here on escalation.
| f"falling back to {DEFAULT_BENCHMARK_CLIENT_TIMEOUT}s" | ||
| ) | ||
| return DEFAULT_BENCHMARK_CLIENT_TIMEOUT | ||
| return None if value <= 0 else value |
There was a problem hiding this comment.
The zero/negative convention here is the opposite of server_ready_timeout() at line 248, which treats <= 0 as "invalid, ignore, use the default". Here 0 means "disable the bound entirely" and a negative value disables it silently (no print_info, unlike the ValueError path). Two adjacent timeout knobs in the same file reading 0 in opposite directions is a foot-gun for whoever sets these in a Jenkins stage. At minimum log when a value <= 0 disables the bound.
| tail_blob = "\n".join(tails) if tails else "<no server logs readable>" | ||
|
|
||
| raise RuntimeError( | ||
| f"Benchmark client made no progress for {elapsed:.0f}s " |
There was a problem hiding this comment.
"made no progress for Ns" isn't what was measured — the client may well have been progressing, just slower than the budget (large ISL/OSL, high concurrency, an over-tight override). Someone triaging from this line will go looking for a stall that isn't there. "did not finish within {elapsed:.0f}s" states exactly what the harness knows.
3 participants
The problem
Perf-sanity stages cannot fail on their own.
The benchmark client runs under
subprocess.check_output()with no timeout, and every other harness wait is bounded byDEFAULT_TIMEOUT(10800s) — which sits above the pytest per-test marker (TIMEOUT (120)⇒ 7200s). So no harness wait can expire first. A stall anywhere below the HTTP layer surfaces only as "the client is still running", and the stage burns its whole Slurm allocation until something external kills it — producing no results XML and no diagnostic.The timeout stack is inverted:
/healthreadiness, aggmin(self.timeout, AGG_SERVER_READY_TIMEOUT)= 30 min/healthreadiness, disaggmin(self.timeout, DISAGG_SERVER_READY_TIMEOUT)= 60 minself.timeout= 10800 sself.timeout= 10800 swait_for_benchmark_readyself.timeout= 10800 sbackend_request_func.py:18)-r 10800)Relationship to #16403
#16403 bounded the startup phase — "the server never becomes healthy" — with a
per-mode budget (disagg gets 60 min because its
/healthanswers only after everyctx/gen worker is up). That is a different phase from this PR, which bounds the
steady-state benchmark run after
/healthhas already answered.The two do not overlap, and the timeline says the remaining hangs are in the second
phase: with disagg readiness bounded at 60 min, a startup hang now fails at 60 min,
yet the stages measured below ran 1.7-2.3 h with servers healthy and the
benchmark running. Only the client phase can absorb that time.
An earlier revision of this description claimed the readiness polls were bounded at
10800 s. That was wrong -- it is true of the three file-rendezvous polls, but not of
/health, which #16403 already fixed.Why now
Over the five days after the benchmark-fill-target fix (#16961), disagg hangs were 13 events / 143 GPU-h, of which 115 GPU-h were multi-hour stages with no identified cause — 1.7–2.3 h runtimes against 18–41 minute budgets, no results XML, killed externally.
This PR does not diagnose those. It makes the stage fail on its own, quickly, with the server-side context attached — so the next one is diagnosable instead of a silent multi-hour burn.
Changes
run_benchmark_client()runs the client under a deadline (default 3600s;TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SECto change,0disables). On expiry it kills the client and raises, naming the elapsed time and the knob, with server-side keyword hits, a raw tail of each server log, and the partial client output attached.check_outputsemantics are otherwise preserved — combined stdout/stderr on success,CalledProcessErroron nonzero exit. Applied to both the aggregated and disaggregated client paths.stop_process()replaces the bareterminate(); wait()at the three server teardown sites, escalating toSIGKILLafter a grace period. A rank wedged in a non-interruptible native call never runs the Python signal handler, and the unboundedwait()held the allocation until Slurm intervened.The timeout path attaches a raw log tail in addition to the
check_error()keyword scan.ERROR_KEYWORDSare Python exception names (RuntimeError,TimeoutError, …) and do not match[TRT-LLM] [E]lines, so a server that died the TRT-LLM way surfaced nothing at all.A deliberate non-change
I did not add
[TRT-LLM] [E]toERROR_KEYWORDS, even though that is the obvious fix for the last point.ERROR_KEYWORDSalso driveswait_for_endpoint_ready()'s fast-fail, where a benign[E]line during startup would begin failing currently-healthy runs. The raw tail gets the diagnostic to the reader without that risk. Worth doing separately with its own evaluation.Tests
tests/unittest/others/test_perf_sanity_bounds.py— 12 tests, no GPU:0as "disabled", and falls back on a malformed value (a CI typo must not silently restore the unbounded behaviour);stop_processreaps a cooperative process, escalates to SIGKILL against a child that ignores SIGTERM (the case the barewait()could not survive), and no-ops on an already-dead process;CalledProcessErroron nonzero exit, bounds a hanging client promptly while naming the knob, and surfaces both keyword hits and the raw tail.All 12 pass locally. Note the module needs
oyaml(declared inrequirements-dev.txt).Risk
The behaviour change is that a run exceeding one hour of client wall-clock now fails instead of hanging. The largest per-test budget in the perf-sanity lists is 120 minutes of pytest budget with real runtimes well under an hour, so this bounds the pathological case with headroom. If a legitimate long run trips it,
TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SECraises or disables it per-stage.Dev Engineer Review
TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SEC.0to disable the timeout.check_output()behavior for successful and nonzero-exit executions.stop_process().SIGTERMtoSIGKILLafter the grace period.ERROR_KEYWORDSunchanged.QA Engineer Review
tests/unittest/others/test_perf_sanity_bounds.py.SIGTERMtermination,SIGKILLescalation, successful output, nonzero client exits, and timeout diagnostics.tests/integration/test_lists/.test-db/orqa/coverage entry is reported.