[TRTLLM-13409][feat] Make a stalled executor worker init self-report - #16973
[TRTLLM-13409][feat] Make a stalled executor worker init self-report#16973JunyiXu-nv wants to merge 1 commit into
Conversation
|
/bot run |
|
PR_Github #62336 [ run ] triggered by Bot. Commit: |
WalkthroughExecutor startup now has configurable stall warnings, per-rank watchdogs, stack-dump logging, and tests for startup handshakes, configuration parsing, watchdog lifecycle, and disarm ordering. ChangesExecutor startup stall diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GenerationExecutorProxy
participant worker_init_status_queue
participant worker_main
participant logger
GenerationExecutorProxy->>worker_init_status_queue: poll initialization status
GenerationExecutorProxy->>logger: warn with elapsed stall report
worker_main->>logger: warn and dump thread stacks
worker_init_status_queue-->>GenerationExecutorProxy: ready or error status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@tensorrt_llm/_utils.py`:
- Around line 703-715: Update print_all_stacks to annotate its optional log
parameter with a precise Callable type matching the emitted string message, and
annotate its return type as None. Preserve the existing default logger.error
behavior and stack-trace emission.
In `@tensorrt_llm/executor/proxy.py`:
- Around line 688-708: Update _worker_init_stall_report to stop inferring worker
liveness or asserting that no rank has exited from mpi_futures. Use an
authoritative liveness result if one is available; otherwise describe
running/finished local futures only, handle 0/0, and remove or qualify the “not
a worker crash” claim.
In `@tensorrt_llm/executor/utils.py`:
- Around line 323-333: Update float_from_env to reject non-finite parsed values
by validating the float with math.isfinite() before returning it; treat NaN and
infinities like invalid input, log the existing warning, and return default.
In `@tensorrt_llm/executor/worker.py`:
- Around line 437-439: Update the startup readiness flow around
notify_with_retry() and worker_init_done so the completion event is set only
when the ready notification is delivered successfully. When notify_with_retry()
returns False, keep the leader watchdog active by continuing retries or leaving
startup pending rather than unconditionally calling worker_init_done.set().
In `@tests/unittest/executor/test_proxy_worker_startup.py`:
- Line 182: Replace the broad BaseException handlers in both background
thread-runner closures with Exception handlers, including the closure around
_start_executor_workers and the handler at the additional referenced location.
Preserve the existing RuntimeError capture and handling while allowing
KeyboardInterrupt and SystemExit 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: f1e7ec9d-062e-4080-b841-35efa3b71fe7
📒 Files selected for processing (5)
tensorrt_llm/_utils.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/utils.pytensorrt_llm/executor/worker.pytests/unittest/executor/test_proxy_worker_startup.py
| def print_all_stacks(log=None): | ||
| """Print stack traces for all threads | ||
|
|
||
| Args: | ||
| log: logging callable used to emit the traces; defaults to | ||
| ``logger.error``. Callers dumping stacks for a condition that is | ||
| not (yet) a fault -- e.g. a slow but healthy startup -- should pass | ||
| ``logger.warning`` instead. | ||
| """ | ||
| log = logger.error if log is None else log | ||
| for thread_id, frame in sys._current_frames().items(): | ||
| logger.error(f"Thread {thread_id} stack trace:\n" + | ||
| "".join(traceback.format_stack(frame))) | ||
| log(f"Thread {thread_id} stack trace:\n" + | ||
| "".join(traceback.format_stack(frame))) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add annotations to the new callable interface.
print_all_stacks() now exposes log but still lacks parameter and return annotations. As per coding guidelines, annotate every function and use precise Callable types.
Proposed fix
+from collections.abc import Callable
+
-def print_all_stacks(log=None):
+def print_all_stacks(
+ log: Callable[[str], None] | None = None) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def print_all_stacks(log=None): | |
| """Print stack traces for all threads | |
| Args: | |
| log: logging callable used to emit the traces; defaults to | |
| ``logger.error``. Callers dumping stacks for a condition that is | |
| not (yet) a fault -- e.g. a slow but healthy startup -- should pass | |
| ``logger.warning`` instead. | |
| """ | |
| log = logger.error if log is None else log | |
| for thread_id, frame in sys._current_frames().items(): | |
| logger.error(f"Thread {thread_id} stack trace:\n" + | |
| "".join(traceback.format_stack(frame))) | |
| log(f"Thread {thread_id} stack trace:\n" + | |
| "".join(traceback.format_stack(frame))) | |
| from collections.abc import Callable | |
| def print_all_stacks( | |
| log: Callable[[str], None] | None = None) -> None: | |
| """Print stack traces for all threads | |
| Args: | |
| log: logging callable used to emit the traces; defaults to | |
| ``logger.error``. Callers dumping stacks for a condition that is | |
| not (yet) a fault -- e.g. a slow but healthy startup -- should pass | |
| ``logger.warning`` instead. | |
| """ | |
| log = logger.error if log is None else log | |
| for thread_id, frame in sys._current_frames().items(): | |
| log(f"Thread {thread_id} stack trace:\n" + | |
| "".join(traceback.format_stack(frame))) |
🤖 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 `@tensorrt_llm/_utils.py` around lines 703 - 715, Update print_all_stacks to
annotate its optional log parameter with a precise Callable type matching the
emitted string message, and annotate its return type as None. Preserve the
existing default logger.error behavior and stack-trace emission.
Source: Coding guidelines
| def _worker_init_stall_report(self, elapsed: float) -> str: | ||
| """Describe a startup that has gone quiet, and where to attribute it. | ||
|
|
||
| The proxy has no IPC channel to ranks other than the leader before the | ||
| ready signal arrives, so it cannot collect worker stacks itself. What | ||
| it can state is (a) that no rank has exited -- which distinguishes a | ||
| stalled initialization from a crash, the two being indistinguishable | ||
| from the client side today -- and (b) where the per-rank stacks that | ||
| each worker dumps on the same schedule can be found. | ||
| """ | ||
| total = len(self.mpi_futures) | ||
| running = sum(1 for fut in self.mpi_futures if not fut.done()) | ||
| return ( | ||
| f"Executor worker initialization has not completed after " | ||
| f"{elapsed:.0f}s; {running}/{total} worker task(s) are still " | ||
| f"running, so no rank has exited (a stalled initialization, not a " | ||
| f"worker crash). Every rank dumps its own thread stacks on the " | ||
| f"same schedule: search the worker logs for 'has not finished " | ||
| f"initialization' to see which rank is stuck and where. Tune or " | ||
| f"silence this report with {WORKER_INIT_STALL_WARN_ENV}.") | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not infer worker liveness from mpi_futures.
RemoteMpiCommSessionClient.submit() can return no local futures, producing 0/0; additionally, a local future can complete after the earlier death check but before this report is built. The message can therefore claim “no rank has exited” when a remote or just-exited worker is actually dead. Use an authoritative liveness result, or qualify this as locally tracked futures and remove the no-crash assertion.
Proposed fix
total = len(self.mpi_futures)
running = sum(1 for fut in self.mpi_futures if not fut.done())
return (
f"Executor worker initialization has not completed after "
- f"{elapsed:.0f}s; {running}/{total} worker task(s) are still "
- f"running, so no rank has exited (a stalled initialization, not a "
- f"worker crash). Every rank dumps its own thread stacks on the "
+ f"{elapsed:.0f}s; {running}/{total} locally tracked worker "
+ f"future(s) are still pending. This is not a definitive worker "
+ f"liveness check. Every rank dumps its own thread stacks on the "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _worker_init_stall_report(self, elapsed: float) -> str: | |
| """Describe a startup that has gone quiet, and where to attribute it. | |
| The proxy has no IPC channel to ranks other than the leader before the | |
| ready signal arrives, so it cannot collect worker stacks itself. What | |
| it can state is (a) that no rank has exited -- which distinguishes a | |
| stalled initialization from a crash, the two being indistinguishable | |
| from the client side today -- and (b) where the per-rank stacks that | |
| each worker dumps on the same schedule can be found. | |
| """ | |
| total = len(self.mpi_futures) | |
| running = sum(1 for fut in self.mpi_futures if not fut.done()) | |
| return ( | |
| f"Executor worker initialization has not completed after " | |
| f"{elapsed:.0f}s; {running}/{total} worker task(s) are still " | |
| f"running, so no rank has exited (a stalled initialization, not a " | |
| f"worker crash). Every rank dumps its own thread stacks on the " | |
| f"same schedule: search the worker logs for 'has not finished " | |
| f"initialization' to see which rank is stuck and where. Tune or " | |
| f"silence this report with {WORKER_INIT_STALL_WARN_ENV}.") | |
| def _worker_init_stall_report(self, elapsed: float) -> str: | |
| """Describe a startup that has gone quiet, and where to attribute it. | |
| The proxy has no IPC channel to ranks other than the leader before the | |
| ready signal arrives, so it cannot collect worker stacks itself. What | |
| it can state is (a) that no rank has exited -- which distinguishes a | |
| stalled initialization from a crash, the two being indistinguishable | |
| from the client side today -- and (b) where the per-rank stacks that | |
| each worker dumps on the same schedule can be found. | |
| """ | |
| total = len(self.mpi_futures) | |
| running = sum(1 for fut in self.mpi_futures if not fut.done()) | |
| return ( | |
| f"Executor worker initialization has not completed after " | |
| f"{elapsed:.0f}s; {running}/{total} locally tracked worker " | |
| f"future(s) are still pending. This is not a definitive worker " | |
| f"liveness check. Every rank dumps its own thread stacks on the " | |
| f"same schedule: search the worker logs for 'has not finished " | |
| f"initialization' to see which rank is stuck and where. Tune or " | |
| f"silence this report with {WORKER_INIT_STALL_WARN_ENV}.") |
🤖 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 `@tensorrt_llm/executor/proxy.py` around lines 688 - 708, Update
_worker_init_stall_report to stop inferring worker liveness or asserting that no
rank has exited from mpi_futures. Use an authoritative liveness result if one is
available; otherwise describe running/finished local futures only, handle 0/0,
and remove or qualify the “not a worker crash” claim.
There was a problem hiding this comment.
+1 — worth noting the evidence is all in-tree (submit() is literally return [] at mpi_session.py:610, and check_worker_error() is the authoritative channel there), and pre_shutdown() at proxy.py:705-721 already carries a long comment about this exact empty-list trap ~20 lines away, including a note that a previous change regressed it; the suggested wording still prints 0/0, which reads as "checked, all fine".
There was a problem hiding this comment.
@zhaoyangwang-nvidia, agreed. The proposed wording was still misleading: 0/0 implies a successful liveness check when it actually means no futures are tracked locally.
The correct fix should be two-part:
- During the startup polling loop, consult
check_worker_error()(via_check_remote_worker_death()) soRemoteMpiCommSessionClientfailures are detected through the authoritative channel. - Make
_worker_init_stall_report()report only the absence of the leader’s ready/init-status signal, without deriving rank liveness or crash status frommpi_futures—and omit the future count when there are no locally tracked futures.
I’ll treat the nearby pre_shutdown() empty-list rationale as supporting evidence for this finding as well. 🐇
✏️ Learnings added
Learnt from: zhaoyangwang-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 16973
File: tensorrt_llm/executor/proxy.py:688-708
Timestamp: 2026-07-30T08:42:12.921Z
Learning: In `tensorrt_llm/executor/proxy.py`, `RemoteMpiCommSessionClient.submit()` returns no local MPI futures, so `GenerationExecutorProxy.mpi_futures` cannot be used to infer remote worker liveness or report `0/0` startup status. During worker startup, `check_worker_error()` is the authoritative channel for remote worker-death detection; startup-stall diagnostics must avoid claiming that no rank exited based on local futures.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| def float_from_env(name: str, default: float) -> float: | ||
| """Read a float-valued environment variable, falling back on bad input.""" | ||
| raw = os.getenv(name) | ||
| if raw is None or not raw.strip(): | ||
| return default | ||
| try: | ||
| return float(raw) | ||
| except ValueError: | ||
| logger.warning( | ||
| f"Ignoring invalid {name}={raw!r}; using default {default}.") | ||
| return default |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python - <<'PY'
import math
for raw in ("nan", "inf", "-inf"):
value = float(raw)
assert not math.isfinite(value)
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- utils.py around float_from_env ---\n'
sed -n '300,360p' tensorrt_llm/executor/utils.py
printf '\n--- search for worker_init_stall_warn_sec ---\n'
rg -n "worker_init_stall_warn_sec|float_from_env\(" tensorrt_llm -S
printf '\n--- relevant worker.py ranges ---\n'
sed -n '1,260p' tensorrt_llm/executor/worker.py
printf '\n--- proxy/timeout usage search ---\n'
rg -n "Event\.wait\(|timeout|stall_warn|watchdog|deadline|period <= 0" tensorrt_llm/executor -SRepository: NVIDIA/TensorRT-LLM
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,360p' tensorrt_llm/executor/utils.py
rg -n "worker_init_stall_warn_sec|float_from_env\(" tensorrt_llm -S
sed -n '1,260p' tensorrt_llm/executor/worker.py
rg -n "Event\.wait\(|timeout|stall_warn|watchdog|deadline|period <= 0" tensorrt_llm/executor -SRepository: NVIDIA/TensorRT-LLM
Length of output: 35643
🏁 Script executed:
python3 - <<'PY'
import threading, time, math
ev = threading.Event()
for timeout in [math.nan, math.inf, -math.inf, 0.0, 0.1]:
start = time.monotonic()
try:
result = ev.wait(timeout=timeout)
elapsed = time.monotonic() - start
print(timeout, result, round(elapsed, 6))
except Exception as exc:
print(timeout, type(exc).__name__, exc)
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '640,685p' tensorrt_llm/executor/proxy.py
python3 - <<'PY'
import math, time
stall_warn_sec_values = [math.nan, math.inf, -math.inf, 0.0, 1.0]
for stall_warn_sec in stall_warn_sec_values:
if stall_warn_sec > 0:
next_warn_time = time.monotonic() + stall_warn_sec
print(stall_warn_sec, "armed", next_warn_time, time.monotonic() >= next_warn_time)
else:
print(stall_warn_sec, "disabled")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 2532
Reject non-finite stall intervals.
float() accepts nan and inf. nan slips past the <= 0 check and makes the worker watchdog spin immediately; inf makes Event.wait() blow up in the worker and pushes the proxy stall report out to infinity. Reject non-finite values with math.isfinite() and fall back to the default.
🤖 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 `@tensorrt_llm/executor/utils.py` around lines 323 - 333, Update float_from_env
to reject non-finite parsed values by validating the float with math.isfinite()
before returning it; treat NaN and infinities like invalid input, log the
existing warning, and return default.
There was a problem hiding this comment.
+1, with =nan the watchdog spins with no sleep, dumping every thread's stack at WARNING in every rank (8 processes on TP=8), while the proxy stays silent; and given this PR's own premise that startup logs are where information gets lost, I'd raise rather than fall back to the default.
There was a problem hiding this comment.
@zhaoyangwang-nvidia, agreed. Falling back turns an explicit but invalid configuration into a noisy, asymmetric failure mode; raising makes the bad value immediately actionable and avoids losing the intended startup diagnostics.
- Make
TRTLLM_WORKER_INIT_STALL_WARN_SECparsing fail fast for malformed or non-finite explicit values. - Preserve the default only when the variable is unset or blank.
- Update the unit tests to cover
nan,inf, and-infraising during configuration parsing.
✏️ Learnings added
Learnt from: zhaoyangwang-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 16973
File: tensorrt_llm/executor/utils.py:323-333
Timestamp: 2026-07-30T08:42:11.520Z
Learning: For executor startup stall diagnostics, an explicitly configured invalid `TRTLLM_WORKER_INIT_STALL_WARN_SEC` value—including `nan` and infinities—must fail fast rather than fall back to the default. Silent fallback can cause every worker watchdog to flood warning stack dumps while the proxy emits no stall report.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- 📌 Commit the changes to this branch
You are interacting with an AI system.
| def start_workers(): | ||
| try: | ||
| proxy._start_executor_workers({}) | ||
| except BaseException as exc: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the blind except BaseException in the background thread runners.
Both thread-runner closures catch BaseException, which also swallows KeyboardInterrupt/SystemExit. except Exception is enough to capture the RuntimeError raised by _start_executor_workers and is what the guideline calls for.
As per coding guidelines, "Catch specific exceptions instead of using broad or bare exception handling such as except:." Static analysis also flags this (BLE001 at line 182, S110 at 218-219), though those specific rule families aren't in this repo's configured Ruff select set per prior learnings.
🐛 Proposed fix
def start_workers():
try:
proxy._start_executor_workers({})
- except BaseException as exc:
+ except Exception as exc:
result["exception"] = exc def start_workers():
try:
proxy._start_executor_workers({})
- except BaseException: # noqa: BLE001 - the release path, not the subject
+ except Exception: # the release path, not the subject
passAlso applies to: 218-219
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 182-182: Do not catch blind exception: BaseException
(BLE001)
🤖 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/executor/test_proxy_worker_startup.py` at line 182, Replace
the broad BaseException handlers in both background thread-runner closures with
Exception handlers, including the closure around _start_executor_workers and the
handler at the additional referenced location. Preserve the existing
RuntimeError capture and handling while allowing KeyboardInterrupt and
SystemExit to propagate.
Sources: Coding guidelines, Linters/SAST tools
|
PR_Github #62336 [ run ] completed with state
|
Pipeline #50503 — two failures, and I am not claiming they are unrelatedTwo failing testcases across the 19 stages that uploaded results: Measured ambient rate over the last 55 pipelines:
Why that is not enough to dismiss them. On my other PRs I have been able to argue that the change cannot reach the failing test. I cannot make that argument here. This PR modifies Two candidate mechanisms I considered and could not rule out by inspection:
Against that: the watchdog only logs, Not re-running yet. Once the scaffolding blocker clears I will re-run, and treat a repeat of either failure as evidence against this PR rather than as noise. |
|
#16978 has merged, so the collection error no longer fails DGX_H100 stages. Re-running now that the result will actually be informative. To restate the standard I set on this PR: the two failures from #50503 — So this run is a test of the change, not a retry for a green light. If either failure repeats, I will treat that as evidence against this PR and investigate the watchdog thread and the startup-path changes rather than look for a third explanation. |
|
/bot run |
|
#16978 has merged, so the To restate the standard I set on this PR: the two failures from #50503 — So this run is a test of the change, not a retry hoping for a green light. If either failure repeats, I will treat that as evidence against this PR — and investigate the per-rank watchdog thread and the startup-path changes — rather than reach for a third flake explanation. |
|
PR_Github #62396 [ run ] triggered by Bot. Commit: |
|
PR_Github #62396 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62672 [ run ] triggered by Bot. Commit: |
|
PR_Github #62672 [ run ] completed with state
|
| def float_from_env(name: str, default: float) -> float: | ||
| """Read a float-valued environment variable, falling back on bad input.""" | ||
| raw = os.getenv(name) | ||
| if raw is None or not raw.strip(): | ||
| return default | ||
| try: | ||
| return float(raw) | ||
| except ValueError: | ||
| logger.warning( | ||
| f"Ignoring invalid {name}={raw!r}; using default {default}.") | ||
| return default |
There was a problem hiding this comment.
+1, with =nan the watchdog spins with no sleep, dumping every thread's stack at WARNING in every rank (8 processes on TP=8), while the proxy stays silent; and given this PR's own premise that startup logs are where information gets lost, I'd raise rather than fall back to the default.
| def _worker_init_stall_report(self, elapsed: float) -> str: | ||
| """Describe a startup that has gone quiet, and where to attribute it. | ||
|
|
||
| The proxy has no IPC channel to ranks other than the leader before the | ||
| ready signal arrives, so it cannot collect worker stacks itself. What | ||
| it can state is (a) that no rank has exited -- which distinguishes a | ||
| stalled initialization from a crash, the two being indistinguishable | ||
| from the client side today -- and (b) where the per-rank stacks that | ||
| each worker dumps on the same schedule can be found. | ||
| """ | ||
| total = len(self.mpi_futures) | ||
| running = sum(1 for fut in self.mpi_futures if not fut.done()) | ||
| return ( | ||
| f"Executor worker initialization has not completed after " | ||
| f"{elapsed:.0f}s; {running}/{total} worker task(s) are still " | ||
| f"running, so no rank has exited (a stalled initialization, not a " | ||
| f"worker crash). Every rank dumps its own thread stacks on the " | ||
| f"same schedule: search the worker logs for 'has not finished " | ||
| f"initialization' to see which rank is stuck and where. Tune or " | ||
| f"silence this report with {WORKER_INIT_STALL_WARN_ENV}.") | ||
|
|
There was a problem hiding this comment.
+1 — worth noting the evidence is all in-tree (submit() is literally return [] at mpi_session.py:610, and check_worker_error() is the authoritative channel there), and pre_shutdown() at proxy.py:705-721 already carries a long comment about this exact empty-list trap ~20 lines away, including a note that a previous change regressed it; the suggested wording still prints 0/0, which reads as "checked, all fine".
4aeb0c3 to
9ccf505
Compare
Both findings fixed —
|
| input | threading.Event().wait(x) |
|---|---|
nan |
returns False in ~10 µs — so while not init_done.wait(period) becomes a hot spin, re-entering print_all_stacks(log=logger.warning) as fast as the interpreter allows, on every rank |
-inf |
same hot spin |
inf |
raises OverflowError: timestamp out of range for platform time_t — kills the watchdog thread silently, the opposite failure and equally bad for a diagnostic |
So your read was exactly right, and inf fails the other way.
float_from_env now raises on both non-finite and unparsable input; only unset/blank falls back. I took your point about raising rather than defaulting and applied it to the pre-existing ValueError path too — the variable is TRTLLM_-prefixed, only ever set deliberately, and read at precisely the moment this PR argues information gets lost. The proxy also reads the knob at the top of _start_executor_workers, before mpi_session.submit(), so a bad value now fails while there are still no ranks to orphan.
2. The 0/0 report — the wording is gone, not reworded.
You called out that the suggested wording still printed 0/0, so the report is now conditional and that string can no longer be emitted. With no futures it reads:
whether a rank has exited cannot be told from here: this MPI session hands back no worker futures, so the proxy has no liveness signal during startup and this report can neither confirm nor rule out a crashed rank. The session's
check_worker_error()channel is authoritative for that
The docstring cites RemoteMpiCommSessionClient.submit() returning [] and points at the pre_shutdown() comment you referenced as the in-tree precedent.
Two notes for the thread. check_worker_error() is deliberately not called from the report — reading it consumes the death notice _check_remote_worker_death() acts on. And it is worse than "under-informed": _error_monitor_thread starts at proxy.py:217, after _start_executor_workers returns at :205, so during the init wait the proxy has zero liveness visibility on that session type, not merely a stale view.
Verification. 27 passed, 15 warnings in 0.41s, exit 0. Three mutations, each reverted after:
| Mutation | Result |
|---|---|
drop the math.isfinite guard |
8 failed, 19 passed — killed by the non-finite and never-arms/never-reaches-loop tests |
restore the silent ValueError fallback |
1 failed, 26 passed |
restore the unconditional {running}/{total} … so no rank has exited |
2 failed, 25 passed |
One thing worth stating rather than hiding: the first draft of one new test hung under mutation instead of failing (killed at 120 s) — with nan, nan > 0 is False so the startup loop never armed its deadline. A hang is not a kill, so the test was rewritten to use a pre-failed future and now fails cleanly. The table above is from the rewritten version.
Not done: no hardware run. Both fixes are validation and string logic on paths the existing fakes already drive, and the empty-mpi_futures branch is established statically from mpi_session.py:610. Happy to run it under TLLM_SPAWN_PROXY_PROCESS=1 if you would rather see it confirmed dynamically.
|
/bot run |
|
PR_Github #62990 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tensorrt_llm/executor/utils.py (1)
326-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd complete Google-style docstrings for the public functions.
float_from_env()does not document its arguments, return value, orValueError.worker_init_stall_warn_sec()has no docstring. AddArgs:,Returns:, andRaises:sections where applicable.As per coding guidelines: “Prefer docstrings for external interfaces, use Google-style docstrings, document public function arguments.”
🤖 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 `@tensorrt_llm/executor/utils.py` around lines 326 - 360, Expand the docstring for float_from_env with Google-style Args, Returns, and Raises sections covering name, default, the returned float, and invalid non-finite or unparsable values. Add a complete Google-style docstring to worker_init_stall_warn_sec describing its returned watchdog warning interval; preserve the existing behavior and explanatory details.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/unittest/executor/test_proxy_worker_startup.py`:
- Around line 15-39: Add tests/unittest/executor/test_proxy_worker_startup.py to
the appropriate CI test list or test database entry so all 20 startup test
functions execute in CI. Do not modify or remove the tests; update only the
existing list configuration used for executor unit-test coverage.
---
Nitpick comments:
In `@tensorrt_llm/executor/utils.py`:
- Around line 326-360: Expand the docstring for float_from_env with Google-style
Args, Returns, and Raises sections covering name, default, the returned float,
and invalid non-finite or unparsable values. Add a complete Google-style
docstring to worker_init_stall_warn_sec describing its returned watchdog warning
interval; preserve the existing behavior and explanatory details.
🪄 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: 02561d6b-c3df-41f0-8041-d72d1483c8f2
📒 Files selected for processing (5)
tensorrt_llm/_utils.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/utils.pytensorrt_llm/executor/worker.pytests/unittest/executor/test_proxy_worker_startup.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tensorrt_llm/_utils.py
- tensorrt_llm/executor/worker.py
- tensorrt_llm/executor/proxy.py
| """Startup handshake between the executor proxy and its MPI workers. | ||
|
|
||
| Every proxy test here drives the real | ||
| ``GenerationExecutorProxy._start_executor_workers``; only the MPI session and | ||
| the init status queue are faked, so no GPU (and no MPI spawn) is needed. The | ||
| worker-side tests drive the real | ||
| ``tensorrt_llm.executor.worker._worker_init_stall_watchdog`` / | ||
| ``_arm_worker_init_stall_watchdog``. | ||
| """ | ||
|
|
||
| import ast | ||
| import pathlib | ||
| import queue | ||
| import sys | ||
| import threading | ||
| import time | ||
| import types | ||
| from concurrent.futures import Future | ||
|
|
||
| import pytest | ||
|
|
||
| from tensorrt_llm._utils import print_all_stacks | ||
| from tensorrt_llm.executor import worker as worker_module | ||
| from tensorrt_llm.executor.proxy import GenerationExecutorProxy | ||
| from tensorrt_llm.executor.utils import WORKER_INIT_STALL_WARN_ENV, worker_init_stall_warn_sec |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any test-db or qa list collects the executor unittest directory.
fd -t f -e yml -e yaml . tests/integration/test_lists | xargs rg -n 'unittest/executor|unittest\b.*executor'Repository: NVIDIA/TensorRT-LLM
Length of output: 3457
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target test file ---'
fd -t f 'test_proxy_worker_startup.py' .
printf '%s\n' '--- exact module references ---'
rg -n -F 'unittest/executor/test_proxy_worker_startup.py' tests/integration/test_lists || true
printf '%s\n' '--- executor directory entries ---'
rg -n -F 'unittest/executor' tests/integration/test_lists || true
printf '%s\n' '--- target test functions ---'
target=$(fd -t f 'test_proxy_worker_startup.py' . | head -n 1)
if [ -n "$target" ]; then
rg -n '^def test_|^async def test_' "$target"
fi
printf '%s\n' '--- nearby list syntax ---'
rg -n -C 2 'unittest/(executor|_torch/executor)' tests/integration/test_lists/test-db | head -n 180Repository: NVIDIA/TensorRT-LLM
Length of output: 11788
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- test-list files ---'
git ls-files tests/integration/test_lists | sed -n '1,160p'
printf '%s\n' '--- target path in all test lists ---'
rg -n -F 'unittest/executor/test_proxy_worker_startup.py' tests/integration/test_lists || true
printf '%s\n' '--- target basename in all test lists ---'
rg -n -F 'test_proxy_worker_startup' tests/integration/test_lists || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 4965
Add the new startup tests to a CI test list.
Test coverage summary:
- Added 20 test functions.
- No test functions were modified or removed.
- No
test-db/orqa/list files were modified. - No list entry includes
tests/unittest/executor/test_proxy_worker_startup.py. - Coverage verdict: insufficient.
🤖 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/executor/test_proxy_worker_startup.py` around lines 15 - 39,
Add tests/unittest/executor/test_proxy_worker_startup.py to the appropriate CI
test list or test database entry so all 20 startup test functions execute in CI.
Do not modify or remove the tests; update only the existing list configuration
used for executor unit-test coverage.
Source: Path instructions
|
PR_Github #62990 [ run ] completed with state
|
The proxy's worker-startup handshake exits only on a leader status
message or on worker death, so a rank that is alive but wedged during
initialization (e.g. inside an NCCL bootstrap collective) leaves the
proxy spinning in worker_init_status_queue.poll(1) with nothing said.
The failure surfaces only as an outer harness timeout, with a
client-side stack that says nothing beyond "waiting on a queue" --
neither which rank is stuck nor whether any rank is stuck at all.
This does not add a deadline. Initialization that is slow but healthy
(a very large checkpoint loading from a cold mount) must not be killed,
so the loop still exits only on ready, init error, or a dead rank. What
changes is that the wait is no longer silent:
* every worker rank arms a watchdog thread before the first collective
and, while its own initialization is still pending, logs its rank/pid
and dumps all of its thread stacks (reusing print_all_stacks) every
TRTLLM_WORKER_INIT_STALL_WARN_SEC seconds (default 600);
* the proxy logs a matching stall report naming the elapsed time and,
where it can see them, how many worker tasks are still running, which
distinguishes a stalled initialization from the already-covered crash.
During init the proxy has an IPC channel to the rank-0 leader only, so a
wedged non-leader is invisible to it: the only process that can say
where rank N is stuck is rank N. Hence the per-rank self-report.
The leader stays armed until it has delivered the ready signal, not
merely until its constructor returns -- that is when the proxy's wait
actually ends, and a leader wedged in between is precisely the case the
proxy cannot see. Subordinates disarm after construction, since they
then block in block_subordinates() for the life of the job.
Attribution is the whole deliverable here, so the report must not claim
more than it knows. RemoteMpiCommSessionClient.submit() returns [], so
under trtllm-llmapi-launch mpi_futures is empty and counting it would
print "0/0 worker task(s) are still running, so no rank has exited" --
a confident liveness statement made with zero visibility, the same
empty-list trap pre_shutdown() already documents. With no futures the
report now says plainly that it cannot tell whether a rank has exited
and names check_worker_error() as the authoritative channel; it does not
call it, because reading it consumes the death notice
_check_remote_worker_death() acts on.
For the same reason the knob is now validated instead of being coerced.
float("nan") does not raise, and every comparison with nan is False, so
a typo'd TRTLLM_WORKER_INIT_STALL_WARN_SEC=nan slipped past the
"period <= 0" disable check and armed a watchdog whose
Event.wait(nan) returns immediately (measured: ~10us, no exception)
rather than sleeping -- a hot loop dumping every thread's stack at
WARNING on every rank. Event.wait(inf) raises OverflowError inside the
watchdog thread instead, silently killing the reporting. Non-finite and
unparsable values now raise, and the proxy reads the knob before any
rank is spawned: startup is exactly where information gets lost, so
swallowing a misconfiguration here is the wrong default.
Both are emitted at WARNING, not ERROR: a slow load is not a fault, and
this fires on that run too. print_all_stacks() grows an optional log
callable so the dump follows the level of the condition that triggered
it; its default stays logger.error for existing callers.
The knob is an environment variable rather than an LlmArgs field
because the TRTLLM-prefixed environment is already forwarded to spawned
MPI ranks, so a single export arms both sides, and the protected LLM API
surface stays untouched.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
9ccf505 to
c9b8f19
Compare
|
/bot run |
|
PR_Github #63194 [ run ] triggered by Bot. Commit: |
|
PR_Github #63194 [ run ] completed with state |
|
PR_Github #63197 [ run ] triggered by Bot. Commit: |
|
PR_Github #63197 [ run ] completed with state |
|
PR_Github #63198 [ run ] triggered by Bot. Commit: |
|
PR_Github #63198 [ run ] completed with state |
|
PR_Github #63202 [ run ] triggered by Bot. Commit: |
|
PR_Github #63202 [ run ] completed with state |
|
PR_Github #63204 [ run ] triggered by Bot. Commit: |
|
PR_Github #63204 [ run ] completed with state |
|
PR_Github #63206 [ run ] triggered by Bot. Commit: |
|
PR_Github #63206 [ run ] completed with state |
|
PR_Github #63229 [ run ] triggered by Bot. Commit: |
|
PR_Github #63229 [ run ] completed with state |
|
PR_Github #63230 [ run ] triggered by Bot. Commit: |
|
PR_Github #63230 [ run ] completed with state |
|
PR_Github #63231 [ run ] triggered by Bot. Commit: |
|
PR_Github #63231 [ run ] completed with state |
|
/bot run |
|
PR_Github #63326 [ run ] triggered by Bot. Commit: |
The gap
The proxy's worker-startup handshake (
proxy.py:645-654) exits on exactly two conditions: the leader sends a status message, or a worker task completes (i.e. a rank died). A rank that is alive but wedged during initialization — typically inside a bootstrap collective — satisfies neither, so the proxy spins inworker_init_status_queue.poll(1)indefinitely.This is a real sighting, not a hypothesis. Recovered from a
pytest-timeoutstack dump:proxy.py:646→ipc.py:181 zmq_poll, after 60.2 minutes of silence, ended by the harness's--timeout=3600. The client-side stack said nothing beyond "waiting on a queue" — not which rank was stuck, nor whether any rank was stuck at all.Nothing existing covers it. The
HangDetectorarms only insidePyExecutor's loops, andPyExecutorhas not been constructed yet. The proxy's fast-death path keys onfut.done(), and the worker is alive.What this does — and does not — do
It adds no timeout and it kills nothing. The loop still exits only on ready, init error, or a dead rank. Initialization that is slow but healthy (a large checkpoint from a cold mount) must not be killed.
What changes is that the wait is no longer silent:
TRTLLM_WORKER_INIT_STALL_WARN_SECseconds (default 600);During init the proxy has an IPC channel to the rank-0 leader only, so a wedged non-leader is invisible to it. The only process that can say where rank N is stuck is rank N; hence the per-rank self-report.
A bound was designed, implemented, and then removed. An opt-in
TRTLLM_WORKER_INIT_TIMEOUT_SECexisted until hardware testing showed it could not deliver on its promise (see "Out of scope" below). Shipping it would have meant offering a knob that logs "giving up" next to a process still alive and still holding GPU memory. That history is recorded here deliberately.Hardware evidence (2xH100 PCIe, Qwen3-0.6B, tp=2)
(a) Arm and disarm across a healthy startup (
TRTLLM_WORKER_INIT_STALL_WARN_SEC=1): both ranks reported 10 times during init; zero watchdog lines in the 142 log lines afterLLM CONSTRUCTED, spanning a post-init sleep, a served request and a post-request sleep.PY_RC=0.(b) A genuinely wedged rank —
SIGSTOPsent to rank 1 mid-init:One rank names where it is stuck; the other is identified by rank and pid as the one that went silent. That attribution is the entire justification for this change.
(c) Slow but healthy. An unrelated run on a cold JIT cache took over 15 minutes to construct. It reported throughout at WARNING, correctly named the straggler, and was not killed. This is the case a deadline would have destroyed.
Out of scope: a pre-existing teardown weakness
Measured while the bound still existed, and reported because it is useful, not because this PR fixes it. With
TRTLLM_WORKER_INIT_TIMEOUT_SEC=60against aSIGSTOP-ed rank, the give-up fired on time at 60s — butMpiPoolSession.shutdown(wait=True)never returned andshutdown_abort's 60sMPI_Abortescalation never logged at all. At t=600s both ranks were still alive holding 2373 MiB + 2335 MiB; the process required an external kill.This is pre-existing in
shutdown_abort, already reachable from the "worker returned error" path, and no longer reachable from this change now that the bound is gone.Known residual
If the leader raises inside
with worker:before reaching the post-ready disarm, its watchdog keeps logging until the process exits. Daemon thread, noise only.Testing
15 unit tests, no GPU required, driving the real
GenerationExecutorProxy._start_executor_workersand the real watchdog helpers. Ten-mutant campaign, all ten killed, including the leader-disarm regression that hardware originally exposed (the watchdog was disarmed after construction rather than after the ready signal — which would have left a wedged leader silent from every rank while every unit test passed).The leader/subordinate disarm placement is pinned structurally over
worker_main's AST, because behavioural coverage there needs a live MPI world; the assertions carry the invariant in their failure messages.Dev Engineer Review
TRTLLM_WORKER_INIT_STALL_WARN_SEC.print_all_stacksaccepts an optional logging callable and preserves the default logger behavior.ValueErrorbefore worker submission.check_worker_error(). Reports identify when rank liveness cannot be determined.QA Engineer Review
tests/unittest/executor/test_proxy_worker_startup.py.tests/integration/test_lists/.test-db/orqa/coverage entry is listed.