[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 2 commits 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 CPU-only 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
|
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
| 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.
…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>
The 12 tests added with this change never ran: pipeline 51960 reports l_test_count_passed=4, with the file absent from collection entirely. The list entry was not the problem -- l0_cpu.yml covers the whole unittest/others directory. The marker was. tests/unittest/conftest.py's pytest_ignore_collect drops any file whose source lacks the literal "pytest.mark.cpu_only" when pytest runs with -m cpu_only, which is how the CPU-Generic stage invokes it. Add the marker, matching the sibling unittest/others/test_http_utils_fail_fast.py. Nothing here needs a GPU: the tests exercise the client-deadline resolution and the SIGTERM->SIGKILL teardown escalation using subprocesses spawned from sys.executable. The module-under-test import resolves in the stage because the harness runs `python -m pytest` with cwd=tests/, which puts tests/ on sys.path for `test_common`; verified by replaying that sys.path locally, where the import advances past test_common and stops only at tensorrt_llm, which the CPU stage has installed. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
8605b3a to
5f9b978
Compare
|
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. |
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/integration/defs/perf/test_perf_sanity.py (1)
102-237: 📐 Maintainability & Code Quality | 🔵 TrivialTest coverage summary.
Changed test-code file:
tests/integration/defs/perf/test_perf_sanity.py. No test functions were added, modified, or removed in this file. The change adds three harness helpers:_benchmark_client_timeout,stop_process, andrun_benchmark_client. Existing perf-sanity test entries are unchanged, so no updates totests/integration/test_lists/test-db/ortests/integration/test_lists/qa/are required for this file.Covered by
tests/unittest/others/test_perf_sanity_bounds.py: timeout resolution, cooperative and stubborn process termination, already-exited processes, successful client output, and nonzero client exit.Coverage gaps:
- No test asserts the behavior for a negative
TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SEC.- No test passes a nonexistent server-log path to
run_benchmark_clienton the timeout path. That case exercises the unguardedcheck_error()call.Verdict: insufficient. Add the two cases above to
tests/unittest/others/test_perf_sanity_bounds.py.As per path instructions: "Always produce a test coverage summary, even if no issues are found" and the summary must list changed test functions, list-file status, and a coverage verdict.
🤖 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 102 - 237, Add tests in test_perf_sanity_bounds.py for a negative TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SEC value, verifying the intended timeout resolution, and for run_benchmark_client timing out with a nonexistent server-log path, verifying it reports the timeout without crashing in check_error(). Preserve existing coverage and keep the test-list files unchanged.Source: Path instructions
🤖 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 203-216: Update the server-log handling around check_error() so
unreadable or vanishing logs are caught without interrupting timeout diagnostic
assembly. Catch OSError from check_error(log_path) as well as the existing
open(log_path) call, preserve available keyword hits and continue processing
remaining logs. Add coverage for missing or unreadable server logs if the test
structure supports it.
In `@tests/unittest/others/test_perf_sanity_bounds.py`:
- Around line 134-156: Update test_client_hang_is_bounded_and_names_the_knob so
the simulated client emits a flushed progress line before sleeping, then assert
that this line appears in the RuntimeError message alongside the existing
timeout details.
- Line 51: Add return annotations of -> None to all 12 test functions in this
test module, including test_client_timeout_defaults_to_one_hour and the other
timeout, teardown, failure, and diagnostics tests. Annotate fixture parameters
with concrete types where applicable, using pytest.MonkeyPatch for monkeypatch
fixtures and pathlib.Path for path fixtures.
---
Nitpick comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 102-237: Add tests in test_perf_sanity_bounds.py for a negative
TRTLLM_PERF_SANITY_CLIENT_TIMEOUT_SEC value, verifying the intended timeout
resolution, and for run_benchmark_client timing out with a nonexistent
server-log path, verifying it reports the timeout without crashing in
check_error(). Preserve existing coverage and keep the test-list files
unchanged.
🪄 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: d97da3d6-0a24-4c1b-a906-67f5dc4ae3ad
📒 Files selected for processing (2)
tests/integration/defs/perf/test_perf_sanity.pytests/unittest/others/test_perf_sanity_bounds.py
| for log_path in server_logs or []: | ||
| base = os.path.basename(log_path) | ||
| for line_idx, line in check_error(log_path): | ||
| keyword_hits.append(f"{base}:{line_idx}: {line}") | ||
| try: | ||
| with open(log_path, "r", errors="replace") as handle: | ||
| # deque(maxlen=) streams the file and keeps only the tail. | ||
| # readlines() would materialise the whole log first, and | ||
| # measured perf-sanity gen logs reach 77-232 MB -- risking | ||
| # an OOM on the rank that is already failing, which would | ||
| # swallow the very timeout report we are assembling. | ||
| tail = list(deque(handle, maxlen=SERVER_LOG_TAIL_LINES)) | ||
| except OSError: | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate error_utils and inspect check_error's file handling.
fd -t f 'error_utils.py' | while IFS= read -r f; do
echo "=== $f ==="
ast-grep run --pattern $'def check_error($$$):
$$$' --lang python "$f"
doneRepository: NVIDIA/TensorRT-LLM
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -eu
f="tests/test_common/error_utils.py"
echo "=== $f outline ==="
ast-grep outline "$f" || true
echo "=== check_error definition and nearby code ==="
rg -n -A80 -B20 '(^|[[:space:]])def[[:space:]]+check_error|check_error[[:space:]]*=' "$f" || true
echo "=== target call site and related log helpers ==="
rg -n -A35 -B25 'check_error|get_server_logs|SERVER_LOG_TAIL_LINES' tests/integration/defs/perf/test_perf_sanity.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 28937
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import builtins
import os
# Probe the exact control flow in tests/test_common/error_utils.py without
# importing or executing repository code.
def check_error_probe(file_path, *, exists, opener):
if not exists(file_path):
return []
with opener(file_path, "r", errors="replace"):
return []
class Handle:
def __enter__(self):
return self
def __exit__(self, *_):
return False
print("missing-at-check: ", check_error_probe(
"missing.log", exists=lambda _: False, opener=builtins.open
))
try:
check_error_probe(
"raced-away.log",
exists=lambda _: True,
opener=lambda *_args, **_kwargs: (_ for _ in ()).throw(
FileNotFoundError("raced-away.log")
),
)
except OSError as exc:
print("exists-then-open failure:", type(exc).__name__, str(exc))
try:
check_error_probe(
"unreadable.log",
exists=lambda _: True,
opener=lambda *_args, **_kwargs: (_ for _ in ()).throw(
PermissionError("unreadable.log")
),
)
except OSError as exc:
print("unreadable failure:", type(exc).__name__, str(exc))
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 289
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== tests related to run_benchmark_client and timeout diagnostics ==="
rg -n -S -g '*.py' 'run_benchmark_client|BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME|server-side error keywords|<no server logs readable>' tests || true
echo "=== applicable integration test lists ==="
rg -n -S 'test_perf_sanity|perf_sanity' tests/integration/test_lists || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
f="tests/unittest/others/test_perf_sanity_bounds.py"
echo "=== test functions ==="
rg -n '^def test_|^async def test_' "$f"
echo "=== timeout-test coverage ==="
sed -n '1,220p' "$f"Repository: NVIDIA/TensorRT-LLM
Length of output: 9058
Handle unreadable server logs in check_error().
check_error() handles an already-missing path, but open() can still raise OSError after os.path.exists() succeeds. Catch this exception so a log race or permission error does not replace the timeout diagnostic.
Test coverage: Existing timeout tests cover readable logs, but not missing or unreadable logs. Coverage is insufficient.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 207-207: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_path, "r", errors="replace")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 203 - 216,
Update the server-log handling around check_error() so unreadable or vanishing
logs are caught without interrupting timeout diagnostic assembly. Catch OSError
from check_error(log_path) as well as the existing open(log_path) call, preserve
available keyword hits and continue processing remaining logs. Add coverage for
missing or unreadable server logs if the test structure supports it.
Source: Linters/SAST tools
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_client_timeout_defaults_to_one_hour(monkeypatch): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="tests/unittest/others/test_perf_sanity_bounds.py"
printf '%s\n' '--- file inventory ---'
git ls-files "$file" CODING_GUIDELINES.md 'tests/integration/test_lists/*' | sed -n '1,160p'
printf '%s\n' '--- target file size ---'
wc -l "$file"
printf '%s\n' '--- target file ---'
cat -n "$file"
printf '%s\n' '--- coding guideline references ---'
if [ -f CODING_GUIDELINES.md ]; then
rg -n -C 3 'Annotate every function|test|copyright|Python' CODING_GUIDELINES.md | sed -n '1,220p'
else
printf '%s\n' 'CODING_GUIDELINES.md not found at repository root'
fi
printf '%s\n' '--- target module in test lists ---'
rg -n -C 2 'test_perf_sanity_bounds|perf_sanity_bounds' tests/integration/test_lists 2>/dev/null || true
printf '%s\n' '--- AST signature report ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("tests/unittest/others/test_perf_sanity_bounds.py")
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith("test_"):
args = [*node.args.posonlyargs, *node.args.args, *node.args.kwonlyargs]
annotations = {
arg.arg: ast.unparse(arg.annotation)
for arg in args
if arg.annotation is not None
}
if node.args.vararg is not None:
annotations["*" + node.args.vararg.arg] = (
ast.unparse(node.args.vararg.annotation)
if node.args.vararg.annotation is not None else None
)
if node.args.kwarg is not None:
annotations["**" + node.args.kwarg.arg] = (
ast.unparse(node.args.kwarg.annotation)
if node.args.kwarg.annotation is not None else None
)
print(
f"{node.lineno}: {node.name} "
f"return={ast.unparse(node.returns) if node.returns else None} "
f"params={annotations}"
)
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 18051
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- annotation guideline ---'
rg -n -C 5 'Annotate every function|function annotations|return annotations|type annotations' CODING_GUIDELINES.md
printf '%s\n' '--- test review and test-list guidance ---'
rg -n -C 8 'test coverage|test list|test-list|tests/integration/test_lists|test functions' CODING_GUIDELINES.md .github 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- all test-list references to unit tests or perf sanity ---'
rg -n -i -C 2 'unittest|test_perf_sanity|perf.?sanity|sanity_bounds' tests/integration/test_lists 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- nearby unit-test signatures ---'
find tests/unittest -maxdepth 3 -type f -name '*.py' -print 2>/dev/null \
| sort \
| head -n 80 \
| while IFS= read -r path; do
rg -n '^def test_|^async def test_' "$path" 2>/dev/null | head -n 8 | sed "s#^#$path:#"
doneRepository: NVIDIA/TensorRT-LLM
Length of output: 186
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- Python guideline section ---'
sed -n '479,530p' CODING_GUIDELINES.md
printf '%s\n' '--- test review instructions in repository ---'
rg -n -i -C 8 'test coverage|test list|test-list|tests/integration/test_lists|test functions' CODING_GUIDELINES.md .github 2>/dev/null \
| sed -n '1,300p' || true
printf '%s\n' '--- all test-list references to unit tests or perf sanity ---'
rg -n -i -C 2 'unittest|test_perf_sanity|perf.?sanity|sanity_bounds' tests/integration/test_lists 2>/dev/null \
| sed -n '1,300p' || true
printf '%s\n' '--- test-list file shapes ---'
for path in \
tests/integration/test_lists/test-db/l0_cpu.yml \
tests/integration/test_lists/test-db/l0_perf.yml \
tests/integration/test_lists/test-db/l0_sanity_check.yml
do
if [ -f "$path" ]; then
printf '\n--- %s ---\n' "$path"
cat -n "$path" | sed -n '1,180p'
fi
done
printf '%s\n' '--- nearby unit-test signatures ---'
find tests/unittest -maxdepth 3 -type f -name '*.py' -print 2>/dev/null \
| sort \
| head -n 80 \
| while IFS= read -r path; do
matches=$(rg -n '^def test_|^async def test_' "$path" 2>/dev/null || true)
if [ -n "$matches" ]; then
printf '%s\n' "$matches" | head -n 8 | sed "s#^#$path:#"
fi
doneRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
Add function annotations.
Add -> None to all 12 test functions. Annotate fixtures with concrete types, such as pytest.MonkeyPatch and pathlib.Path.
Test coverage: 12 tests cover timeout bounds, teardown escalation, client failures, and log diagnostics. Coverage verdict: sufficient.
🤖 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` at line 51, Add return
annotations of -> None to all 12 test functions in this test module, including
test_client_timeout_defaults_to_one_hour and the other timeout, teardown,
failure, and diagnostics tests. Annotate fixture parameters with concrete types
where applicable, using pytest.MonkeyPatch for monkeypatch fixtures and
pathlib.Path for path fixtures.
Source: Coding guidelines
| ignores_sigterm = ( | ||
| "import signal, time\nsignal.signal(signal.SIGTERM, signal.SIG_IGN)\ntime.sleep(60)\n" | ||
| ) | ||
| proc = subprocess.Popen([sys.executable, "-c", ignores_sigterm]) | ||
| # Let the child install its handler before we signal it. | ||
| time.sleep(1.0) | ||
|
|
||
| started = time.monotonic() | ||
| perf_sanity.stop_process(proc, "stubborn", grace=3) | ||
| elapsed = time.monotonic() - started | ||
|
|
||
| assert proc.poll() is not None, "SIGKILL should have reaped the process" | ||
| assert elapsed < 30, f"stop_process took {elapsed:.1f}s; it must not wait out the full sleep" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prove that SIGKILL escalation occurred.
The fixed one-second delay does not prove that the child installed its SIGTERM handler. If the child starts late, SIGTERM terminates it and all current assertions pass without testing escalation.
Make the child signal readiness after it installs the handler. Then assert that proc.returncode is -signal.SIGKILL.
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 94-94: Command coming from incoming request
Context: subprocess.Popen([sys.executable, "-c", ignores_sigterm])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[error] 95-95: subprocess call: check for execution of untrusted input
(S603)
| def test_client_hang_is_bounded_and_names_the_knob(monkeypatch, tmp_path): | ||
| """The regression that mattered: a client that never returns. | ||
|
|
||
| Before, this ran until Slurm killed the stage hours later. It must now | ||
| raise promptly, name the env var, and carry the server-side errors. | ||
| """ | ||
| monkeypatch.setenv(perf_sanity.BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME, "2") | ||
| server_log = tmp_path / "trtllm-serve.CTX_0.0.log" | ||
| server_log.write_text("some line\n[TRT-LLM] [E] Error in event loop: boom\n") | ||
|
|
||
| started = time.monotonic() | ||
| with pytest.raises(RuntimeError) as excinfo: | ||
| perf_sanity.run_benchmark_client( | ||
| [sys.executable, "-c", "import time; time.sleep(120)"], | ||
| dict(os.environ), | ||
| [str(server_log)], | ||
| ) | ||
| elapsed = time.monotonic() - started | ||
|
|
||
| msg = str(excinfo.value) | ||
| assert perf_sanity.BENCHMARK_CLIENT_TIMEOUT_ENV_VAR_NAME in msg | ||
| assert "made no progress" in msg | ||
| assert elapsed < 60, f"bound did not fire promptly ({elapsed:.1f}s)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert partial client output in the timeout report.
This test verifies the timeout message but not the partial-output contract. Make the client print a flushed progress line before sleeping, then assert that the line is included in RuntimeError.
This protects the diagnostic behavior required when a benchmark client stalls.
🤖 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 134 - 156,
Update test_client_hang_is_bounded_and_names_the_knob so the simulated client
emits a flushed progress line before sleeping, then assert that this line
appears in the RuntimeError message alongside the existing timeout details.
|
PR_Github #65610 [ run ] triggered by Bot. Commit: |
|
PR_Github #65610 [ run ] completed with state
|
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
Changes required: terminate the complete server process tree.\n\nNVBug demonstrates the exact gap here: after the pytest thread timeout, MPI worker PIDs retained about , and later GLM cases OOMed in the same allocation. The current sends / only to ; descendants can therefore outlive the launcher.\n\nPlease start each server in its own session/process group and terminate that group, or use a verified process-tree cleanup helper. Add a regression where a descendant ignores and prove both parent and descendant are gone. CI pipeline failed on an unrelated invalid test-list entry.\n\nThis is required before merge.
Replacing this review because shell formatting stripped its inline identifiers.
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
Changes required: terminate the complete server process tree.
NVBug 6526562 demonstrates the exact gap here: after the pytest thread timeout, MPI worker PIDs retained about 255.74 GiB, and later GLM cases OOMed in the same allocation. The current stop_process() sends SIGTERM/SIGKILL only to server_proc; descendants can therefore outlive the launcher.
Please start each server in its own session/process group and terminate that group, or use a verified process-tree cleanup helper. Add a regression where a descendant ignores SIGTERM and prove both parent and descendant are gone. CI pipeline #53337 failed on an unrelated invalid test-list entry.
This is required before merge.
5 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_SECtimeout.0to disable the timeout.check_output()behavior.stop_process().SIGTERMtoSIGKILLafter the grace period.ERROR_KEYWORDSunchanged.QA Engineer Review
tests/unittest/others/test_perf_sanity_bounds.py.tests/integration/test_lists/.test-db/orqa/coverage entry is reported.