Repository navigation
[None][feat] Add per-rank perf time events capture (TRTLLM_PERF_TIME_EVENTS_PATH) - #16900
chenfeiz0326 wants to merge 10 commits into
Conversation
…EVENTS_PATH)
Add a single master switch, TRTLLM_PERF_TIME_EVENTS_PATH (an output
directory, symmetric with TRTLLM_KVCACHE_TIME_OUTPUT_PATH), that turns
on per-request perf-timing capture and writes per-rank time-event files
live from inside the executor -- mirroring how the disaggregation
transceiver already writes per-rank KV-transfer CSVs. No HTTP
/perf_metrics scrape, no server-teardown race, no per-worker
return_perf_metrics / --save-request-time-breakdown fiddling.
What the switch does:
* Force-enables capture independent of return_perf_metrics and turns on
an extended capture mode (capture_extended). All new behavior is
gated on capture_extended, so existing return_perf_metrics=True users
with the env unset see NO behavior or output change.
* Adds the previously-missing per-iteration batch context to each
per-iteration metric dict: iter_batch_size, num_ctx_requests,
num_gen_requests, context_token_number, generation_token_number,
per-request req_context_token_number / req_generation_token_number,
plus a per-iteration starvation count (num_capacity_fitting vs
num_scheduled). These ride inside the existing time_breakdown_metrics
payload, so there is no new payload shape and no create_response
change.
* Writes {dir}/time_events_rank{N}_pid{P}.jsonl live, one JSONL line
per finished request.
Off critical path by design (overhead requirement): the executor loop
only builds a dict and does a non-blocking queue.Queue.put_nowait; a
lazily-started daemon thread does the json.dumps + write + flush;
PyExecutor.shutdown() drains and joins it. queue.Full -> warn + drop,
never block.
An OPTIONAL offline aggregator
(python -m tensorrt_llm.serve.scripts.perf_time_events) stitches the
per-rank files together with the KV-transfer CSVs into one combined
JSON/HTML and computes derived signals (inter-step / inter-chunk gaps,
per-iteration starvation). It reuses time_breakdown's RequestTimeBreakdown
for HTML and is stdlib-only on the parse/merge/JSON path (torch-free;
plotly pulled in lazily only for --html), so it stays under the
GPU-free import contract.
Python only -- no C++. The KV sub-events already exist in Python for the
target PYTHON-runtime (disagg NIXL) workload, so they are only ingested.
Flagged Python-only compromises (see docs + PR):
* Category 1b starvation is a per-iteration COUNT, not per-request
attribution -- _schedule() returns only num_fitting_reqs in Python.
* _executor_loop_pp never calls save_timing_to_requests, so the
extended fields are absent under pipeline parallelism (known gap).
* Per-rank files are per-server; disagg ctx<->gen correlation reuses
the request's disagg ctx_request_id when available, else it is an
aggregator-side join enhancement.
Tests (GPU-free):
* tests/unittest/_torch/executor/test_perf_metrics_events.py --
append_step_metrics merge + token counts (and the disabled-shape
guard), _compute_iter_batch_context, and the off-thread per-rank
writer (writes one line, no-ops when metrics absent / disabled).
* tests/unittest/others/test_perf_time_events.py -- aggregator
parse/merge/join/derive, stdlib-only.
Docs: docs/source/developer-guide/perf-analysis.md gains a
"Perf Time Events (per-rank live capture)" section documenting the
switch, the JSONL record shape, the three known limitations, and the
aggregator recipe.
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds opt-in per-rank, router, and client performance event capture, extended executor timing context, environment-based server activation, and an offline JSONL/KV aggregation CLI with tests and documentation. ChangesPerf time-events workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant OpenAIServer
participant PyExecutor
participant PerfTimeEventsMerger
Client->>Router: send request and record client timestamps
Router->>OpenAIServer: dispatch context and generation requests
OpenAIServer->>PyExecutor: enable performance metrics
PyExecutor->>OpenAIServer: return response with timing events
PerfTimeEventsMerger->>PerfTimeEventsMerger: merge worker, router, client, and KV artifacts
PerfTimeEventsMerger->>Client: write combined JSON or HTML timeline
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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: 4
🧹 Nitpick comments (10)
tensorrt_llm/serve/scripts/perf_time_events/__init__.py (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort
__all__to satisfy Ruff RUF022.As per static analysis, Ruff flags "`__all__` is not sorted" (RUF022).♻️ Suggested fix
__all__ = [ "PerfTimeEventsMerger", - "parse_event_dir", - "parse_kv_csv_dir", "main", + "parse_event_dir", + "parse_kv_csv_dir", ]🤖 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/serve/scripts/perf_time_events/__init__.py` around lines 26 - 33, Sort the entries in __all__ alphabetically to satisfy Ruff RUF022, while preserving the existing exported symbols from the perf_time_events package.Source: Linters/SAST tools
tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py (7)
1-1: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCSV header contract with the KV-transfer producer is hand-duplicated, not imported — drift wouldn't be caught. Both the parser and its test independently hard-code
_NATIVE_TASK_HEADER/_GEN_SUMMARY_HEADER, each claiming to mirror_torch/disaggregation/native/perf_logger.py, but neither imports from that module, so a real header change there would silently fall through to the generic/unknown-layout branch instead of failing a test.
tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py#L108-177: import the header constants from_torch/disaggregation/native/perf_logger.py(or a shared constants module) instead of re-declaring them here.tests/unittest/others/test_perf_time_events.py#L1-213: assert against the same imported constants (or add a dedicated test that importsperf_logger.py's header and compares it toperf_time_events.py's copy) so an actual producer-side header change trips this suite.🤖 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/serve/scripts/perf_time_events/perf_time_events.py` at line 1, Replace the locally duplicated _NATIVE_TASK_HEADER and _GEN_SUMMARY_HEADER declarations in perf_time_events.py with imports from _torch/disaggregation/native/perf_logger.py or a shared constants module. Update test_perf_time_events.py to use those same producer-owned constants, ensuring parser expectations and tests detect future header changes.
205-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
-> Noneto__init__.As per coding guidelines, "Annotate every function, use
Nonefor non-returning functions."🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 205 - 209, Update the __init__ method signature to include an explicit -> None return annotation, leaving its existing initialization logic unchanged.
347-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
-> Nonetomain(). Otherwise the CLI wiring, error path (Line 383-384), and merge/write/html sequencing look correct.As per coding guidelines, "Annotate every function, use
Nonefor non-returning functions."🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 347 - 390, Annotate the CLI entry-point function main with an explicit -> None return type, preserving its existing argument parsing, validation, merge, and output sequencing.
61-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing return type annotation on
_iter_jsonl.Generator function lacks an annotation.
As per coding guidelines, "Annotate every function, use
Nonefor non-returning functions... prefer built-in generic types."♻️ Suggested fix
-def _iter_jsonl(path: str): +def _iter_jsonl(path: str) -> "Iterator[dict[str, Any]]":(add
from collections.abc import Iteratorto imports)🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 61 - 71, The generator function _iter_jsonl lacks a return type annotation. Import Iterator from collections.abc and annotate _iter_jsonl’s return type as Iterator using the appropriate parsed JSON value type, while preserving its existing behavior.
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse PEP 585 generics instead of
typing.Dict/List/Optional.This is a Python 3.10+ codebase;
Dict[str, Any],List[...],Optional[...]are used throughout this file (parse_event_dir,parse_kv_csv_dir,PerfTimeEventsMergerfields/methods) wheredict[str, Any],list[...],X | Noneare preferred.As per coding guidelines, "Annotate every function... prefer built-in generic types and
|."♻️ Example (apply pattern file-wide)
-from typing import Any, Dict, List, Optional +from typing import AnyThen replace
Dict[K, V]→dict[K, V],List[X]→list[X],Optional[X]→X | Nonethroughout the file.🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 30 - 36, Update type annotations throughout perf_time_events.py, including parse_event_dir, parse_kv_csv_dir, and PerfTimeEventsMerger, to use PEP 585 built-in generics: replace Dict with dict, List with list, and Optional[T] with T | None; remove the corresponding typing imports while preserving all annotation semantics.
211-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
merge()reuses staleself.kvif called again withoutkv_csv_dir.
self.kvis only overwrittenif kv_csv_dir:(line 228-229); a secondmerge()call on the same instance withoutkv_csv_dirwill silently keep joining against the previous run's KV data. Not an issue for the current single-call CLI usage (main()callsmerge()once), but worth a docstring note or an explicit reset if the class is intended to be reused across merges.🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 211 - 316, Update merge() so each invocation clears or reinitializes self.kv when kv_csv_dir is not provided, preventing joins from using KV data loaded by a previous call. Preserve loading parsed KV data when kv_csv_dir is supplied and keep the existing task_events, gen_summary, and cpp_events processing unchanged.
329-344: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFactor out the timing-row build into
RequestTimeBreakdown.write_html()duplicates the parse/duration loop and ties this path toparser/config.metrics; a small public helper would keep the--htmlflow on one contract and reduce drift.🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 329 - 344, Move the timing-row construction loop from write_html into a small public helper on RequestTimeBreakdown that parses each record and adds all configured metric durations. Update write_html to call that helper and pass its result to create_timing_diagram, removing direct parser/config.metrics access from write_html.tests/unittest/others/test_perf_time_events.py (1)
1-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest coverage summary.
Added tests:
test_globs_and_concatenates,test_skips_malformed_lines(parse_event_dir);test_native_task_and_gen_summary,test_cpp_send_recv_keyed_on_request_id(parse_kv_csv_dir);test_join_by_request_and_ctx_id,test_unjoined_kv_events_reported,test_derived_inter_step_gaps_and_starved,test_write_combined_json,test_merge_perf_json_input(PerfTimeEventsMerger).This file lives under
tests/unittest/others/, nottests/integration/, so it is collected directly by the pytest unittest run rather than throughtests/integration/test_lists/{test-db,qa}entries — no test-list additions are needed for this file.Coverage verdict: sufficient for the parse/merge/JSON path exercised here (globbing, malformed-line skipping, native/C++ CSV parsing, request/ctx-id joins, unjoined reporting, derived gaps/starvation, JSON round-trip, perf-json input). Gaps worth noting:
write_html()is untested (reasonable given the GPU-free/torch-free constraint) andmain()'s argument-parsing/error path (Line 383-384 inperf_time_events.py) has no test. Also,_NATIVE_TASK_HEADER/_GEN_SUMMARY_HEADERhere are independently hand-copied from the same source the module's own copies claim to mirror (_torch/disaggregation/native/perf_logger.py) rather than shared/imported, so this test only catches drift between the module and the test, not real drift against the actual producer.As per path instructions, "Always produce a test coverage summary, even if no issues are found."
🤖 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_time_events.py` around lines 1 - 213, Strengthen the CSV header tests in TestParseKvCsvDir by deriving or validating _NATIVE_TASK_HEADER and _GEN_SUMMARY_HEADER against the actual producer definitions in perf_logger.py, rather than relying on independently copied literals. Preserve the existing native task and generation summary parsing assertions while ensuring producer-header drift causes a test failure.Source: Path instructions
tests/unittest/_torch/executor/test_perf_metrics_events.py (1)
1-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary.
Added
TestAppendStepMetricsExtended,TestComputeIterBatchContext, andTestPerRankWriter, covering the new extended-capture merge logic,_compute_iter_batch_context, and the JSONL writer's enqueue/no-op gating. This is a new file undertests/unittest/...and does not require entries intests/integration/test_lists/(that path gates integration tests, not this unit suite). Coverage verdict: sufficient for the happy-path behavior exercised here; consider adding a regression test that simulates a writer-thread failure (e.g. patchopen()to raise inside_writer_loop) and assertsclose()returns promptly instead of blocking, covering the hang risk flagged inperf_metrics_manager.py.As per path instructions, "Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM" and "Always produce a test coverage summary, even if no issues are found."
🤖 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/_torch/executor/test_perf_metrics_events.py` around lines 1 - 249, Add a regression test in TestPerRankWriter that forces open() to fail inside PerfMetricsManager._writer_loop, enqueues a request event, and verifies PerfMetricsManager.close() returns promptly without hanging. Ensure the test exercises the writer-thread failure path and uses a bounded wait or timeout while preserving cleanup of the manager.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 `@tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py`:
- Around line 415-450: Update close() to enqueue _WRITER_STOP with a bounded or
non-blocking operation so shutdown cannot hang when _writer_loop has exited or
the queue is full. Replace the broad except Exception with handling for the
specific queue exception(s), and log enqueue failures while preserving the
existing thread join and cleanup behavior.
- Around line 322-340: Update the per-request generation metric in the extended
metrics block to use the same draft-token source as _compute_iter_batch_context,
including its getattr(req, "num_draft_tokens", 0) fallback, instead of
get_draft_token_length(request). Ensure req_generation_token_number remains 1
plus that shared draft-token value so batch and per-request totals stay
consistent.
In `@tensorrt_llm/serve/openai_server.py`:
- Around line 590-603: Move the TRTLLM_PERF_TIME_EVENTS_PATH-based
initialization of self.perf_metrics and self.perf_metrics_lock outside the
return_perf_metrics conditional in the surrounding initialization flow. Ensure
the fallback runs whenever the environment variable is set and self.perf_metrics
is None, so /perf_metrics has a live deque even when
generator.args.return_perf_metrics is false.
In `@tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py`:
- Around line 108-177: The parser duplicates producer CSV header definitions
through _NATIVE_TASK_HEADER and _GEN_SUMMARY_HEADER, allowing header changes to
desynchronize routing. Update parse_kv_csv_dir to import and reuse the
producer’s shared header constants, or move both definitions into a shared
module consumed by the parser and writer, while preserving the existing native
task and generation-summary classification.
---
Nitpick comments:
In `@tensorrt_llm/serve/scripts/perf_time_events/__init__.py`:
- Around line 26-33: Sort the entries in __all__ alphabetically to satisfy Ruff
RUF022, while preserving the existing exported symbols from the perf_time_events
package.
In `@tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py`:
- Line 1: Replace the locally duplicated _NATIVE_TASK_HEADER and
_GEN_SUMMARY_HEADER declarations in perf_time_events.py with imports from
_torch/disaggregation/native/perf_logger.py or a shared constants module. Update
test_perf_time_events.py to use those same producer-owned constants, ensuring
parser expectations and tests detect future header changes.
- Around line 205-209: Update the __init__ method signature to include an
explicit -> None return annotation, leaving its existing initialization logic
unchanged.
- Around line 347-390: Annotate the CLI entry-point function main with an
explicit -> None return type, preserving its existing argument parsing,
validation, merge, and output sequencing.
- Around line 61-71: The generator function _iter_jsonl lacks a return type
annotation. Import Iterator from collections.abc and annotate _iter_jsonl’s
return type as Iterator using the appropriate parsed JSON value type, while
preserving its existing behavior.
- Around line 30-36: Update type annotations throughout perf_time_events.py,
including parse_event_dir, parse_kv_csv_dir, and PerfTimeEventsMerger, to use
PEP 585 built-in generics: replace Dict with dict, List with list, and
Optional[T] with T | None; remove the corresponding typing imports while
preserving all annotation semantics.
- Around line 211-316: Update merge() so each invocation clears or reinitializes
self.kv when kv_csv_dir is not provided, preventing joins from using KV data
loaded by a previous call. Preserve loading parsed KV data when kv_csv_dir is
supplied and keep the existing task_events, gen_summary, and cpp_events
processing unchanged.
- Around line 329-344: Move the timing-row construction loop from write_html
into a small public helper on RequestTimeBreakdown that parses each record and
adds all configured metric durations. Update write_html to call that helper and
pass its result to create_timing_diagram, removing direct parser/config.metrics
access from write_html.
In `@tests/unittest/_torch/executor/test_perf_metrics_events.py`:
- Around line 1-249: Add a regression test in TestPerRankWriter that forces
open() to fail inside PerfMetricsManager._writer_loop, enqueues a request event,
and verifies PerfMetricsManager.close() returns promptly without hanging. Ensure
the test exercises the writer-thread failure path and uses a bounded wait or
timeout while preserving cleanup of the manager.
In `@tests/unittest/others/test_perf_time_events.py`:
- Around line 1-213: Strengthen the CSV header tests in TestParseKvCsvDir by
deriving or validating _NATIVE_TASK_HEADER and _GEN_SUMMARY_HEADER against the
actual producer definitions in perf_logger.py, rather than relying on
independently copied literals. Preserve the existing native task and generation
summary parsing assertions while ensuring producer-header drift causes a test
failure.
🪄 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: 9fffc183-8f40-4fb5-9cea-64293732be28
📒 Files selected for processing (12)
docs/source/developer-guide/perf-analysis.mdtensorrt_llm/_torch/pyexecutor/llm_request.pytensorrt_llm/_torch/pyexecutor/perf_metrics_manager.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/serve/openai_server.pytensorrt_llm/serve/responses_utils.pytensorrt_llm/serve/scripts/perf_time_events/README.mdtensorrt_llm/serve/scripts/perf_time_events/__init__.pytensorrt_llm/serve/scripts/perf_time_events/__main__.pytensorrt_llm/serve/scripts/perf_time_events/perf_time_events.pytests/unittest/_torch/executor/test_perf_metrics_events.pytests/unittest/others/test_perf_time_events.py
| self.perf_metrics_maxlen = max_perf_metrics | ||
| self.perf_metrics = deque(maxlen=max_perf_metrics) | ||
| self.perf_metrics_lock = asyncio.Lock() | ||
| # TRTLLM_PERF_TIME_EVENTS_PATH also makes the HTTP /perf_metrics | ||
| # endpoint usable for single-server (aggregated) runs, even when | ||
| # return_perf_metrics / perf_metrics_max_requests were not set. The | ||
| # per-rank live files remain the primary artifact; this is | ||
| # convenience. | ||
| if (self.perf_metrics is None | ||
| and len(os.getenv("TRTLLM_PERF_TIME_EVENTS_PATH", "")) > 0): | ||
| self.perf_metrics_maxlen = ( | ||
| self.generator.args.perf_metrics_max_requests or 65536) | ||
| self.perf_metrics = deque(maxlen=self.perf_metrics_maxlen) | ||
| self.perf_metrics_lock = asyncio.Lock() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'TRTLLM_PERF_TIME_EVENTS_PATH' --type=py -C3
rg -n 'return_perf_metrics\s*=' --type=py -C2 -g '!**/test*'Repository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant server code and nearby argument wiring.
fd -a 'openai_server.py' .
fd -a 'args.py' .
fd -a 'llm_args.py' .
fd -a 'config.py' .
# Find all references to the env var and perf-metrics flags.
rg -n 'TRTLLM_PERF_TIME_EVENTS_PATH|return_perf_metrics|perf_metrics_max_requests|perf_metrics' tensorrt_llm -g '*.py' -C 2
# Show the relevant section of openai_server.py with line numbers.
python3 - <<'PY'
from pathlib import Path
p = Path('tensorrt_llm/serve/openai_server.py')
text = p.read_text().splitlines()
for i in range(540, 620):
if i <= len(text):
print(f"{i:4d}: {text[i-1]}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | rg 'openai_server\.py|llm_args|args\.py|config\.py|server' || true
echo
echo "== perf-metrics references =="
rg -n 'TRTLLM_PERF_TIME_EVENTS_PATH|return_perf_metrics|perf_metrics_max_requests|perf_metrics' tensorrt_llm -g '*.py' -C 3 || true
echo
echo "== openai_server.py excerpt =="
python3 - <<'PY'
from pathlib import Path
p = Path('tensorrt_llm/serve/openai_server.py')
text = p.read_text().splitlines()
for i in range(550, 615):
if i <= len(text):
print(f"{i:4d}: {text[i-1]}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
Move the env-var fallback out of the return_perf_metrics gate TRTLLM_PERF_TIME_EVENTS_PATH still won’t populate self.perf_metrics unless self.generator.args.return_perf_metrics is already true. The request path can set sampling_params.return_perf_metrics = True, but that only affects per-request metrics; /perf_metrics still returns [] when the deque is never initialized.
🤖 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/serve/openai_server.py` around lines 590 - 603, Move the
TRTLLM_PERF_TIME_EVENTS_PATH-based initialization of self.perf_metrics and
self.perf_metrics_lock outside the return_perf_metrics conditional in the
surrounding initialization flow. Ensure the fallback runs whenever the
environment variable is set and self.perf_metrics is None, so /perf_metrics has
a live deque even when generator.args.return_perf_metrics is false.
| def parse_kv_csv_dir(kv_csv_dir: str) -> Dict[str, Any]: | ||
| """Parse KV-transfer CSVs into join-ready structures. | ||
|
|
||
| Handles two producers: | ||
|
|
||
| * Python-native transceiver (``perf_logger.py``): | ||
| - ``{instance}_{rank}.csv`` -> task rows keyed by ``unique_rid`` | ||
| (KVSendTask / AuxSendTask / KVRecvTask; recv rows leave the middle | ||
| latency fields blank). | ||
| - ``{instance}_{rank}_gen_transfer_summary.csv`` -> gen summary keyed by | ||
| ``RequestID``. | ||
| * C++ transceiver: ``*_send.csv`` / ``*_recv.csv`` keyed by ctx-phase | ||
| ``RequestID`` (parsed for completeness so mixed dirs still aggregate). | ||
|
|
||
| Returns a dict with ``task_events`` (rid -> list[row]), ``gen_summary`` | ||
| (rid -> row) and ``cpp_events`` (rid -> list[row]). | ||
| """ | ||
| task_events: Dict[str, List[Dict[str, Any]]] = defaultdict(list) | ||
| gen_summary: Dict[str, Dict[str, Any]] = {} | ||
| cpp_events: Dict[str, List[Dict[str, Any]]] = defaultdict(list) | ||
|
|
||
| csv_files = sorted(glob.glob(os.path.join(kv_csv_dir, "*.csv"))) | ||
| for path in csv_files: | ||
| base = os.path.basename(path) | ||
| try: | ||
| with open(path, "r", newline="") as f: | ||
| reader = csv.reader(f) | ||
| try: | ||
| header = next(reader) | ||
| except StopIteration: | ||
| continue | ||
| header = [h.strip() for h in header] | ||
| rows = [row for row in reader if row] | ||
| except OSError as e: | ||
| print(f"WARNING: failed to read KV CSV {path}: {e}") | ||
| continue | ||
|
|
||
| if base.endswith("_gen_transfer_summary.csv") or header == _GEN_SUMMARY_HEADER: | ||
| for row in rows: | ||
| rec = dict(zip(header, row)) | ||
| rid = rec.get("RequestID") | ||
| if rid is not None: | ||
| rec["_source_file"] = base | ||
| gen_summary[str(rid)] = rec | ||
| elif header == _NATIVE_TASK_HEADER: | ||
| for row in rows: | ||
| rec = dict(zip(header, row)) | ||
| rid = rec.get("unique_rid") | ||
| if rid is not None: | ||
| rec["_source_file"] = base | ||
| task_events[str(rid)].append(rec) | ||
| else: | ||
| # C++ (or unknown) layout: try to key on a RequestID-like column. | ||
| rid_key = None | ||
| for candidate in ("RequestID", "request_id", "unique_rid"): | ||
| if candidate in header: | ||
| rid_key = candidate | ||
| break | ||
| for row in rows: | ||
| rec = dict(zip(header, row)) | ||
| rec["_source_file"] = base | ||
| rid = rec.get(rid_key) if rid_key else None | ||
| if rid is not None: | ||
| cpp_events[str(rid)].append(rec) | ||
|
|
||
| return { | ||
| "task_events": dict(task_events), | ||
| "gen_summary": gen_summary, | ||
| "cpp_events": dict(cpp_events), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'timestamp,task_type,unique_rid|_NATIVE_TASK_HEADER|_GEN_SUMMARY_HEADER' -g '*.py' tensorrt_llm/_torch/disaggregationRepository: NVIDIA/TensorRT-LLM
Length of output: 562
🏁 Script executed:
#!/bin/bash
sed -n '1,240p' tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py
printf '\n--- PRODUCER ---\n'
sed -n '1,460p' tensorrt_llm/_torch/disaggregation/native/perf_logger.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 25791
Share the KV CSV headers with the producer
_NATIVE_TASK_HEADER and _GEN_SUMMARY_HEADER are duplicated literals here, so a producer-side header change will silently route native rows into the generic C++/unknown path. Import the producer constants or define them once in a shared module to keep the parser and writer in sync.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 132-132: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, "r", newline="")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.21)
[warning] 147-147: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 154-154: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 167-167: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines
108 - 177, The parser duplicates producer CSV header definitions through
_NATIVE_TASK_HEADER and _GEN_SUMMARY_HEADER, allowing header changes to
desynchronize routing. Update parse_kv_csv_dir to import and reuse the
producer’s shared header constants, or move both definitions into a shared
module consumed by the parser and writer, while preserving the existing native
task and generation-summary classification.
GPU end-to-end verification (OCI-HSG GB200)Posting the promised follow-up. Workload:
Method — single-wheel A/B. ONE instrumented wheel is used for both arms; only H2 — capture works ✅Each instrumented rep wrote 3 per-rank files (12 files / 4 reps, 4108 records,
All 7 extended keys present ( H3 — switch-off is dormant ✅Every baseline rep (env unset) wrote 0 H2b — overhead when the switch is ON: ≈4% (opt-in only)Two independent signals agree, with zero overlap between arms:
So enabling the switch costs ≈4% on this high-step-count decode workload — real Attribution (verified in source) — the ≈4% is the pre-existing per-iteration
(Gen-rank files are ~131 MB each — 256 req × 1023 per-iteration Summary: H2 PASS · H3 PASS · H2b ≈+4% (opt-in capture cost, off-by-default). |
…request lifecycle scalars
Builds on the per-rank live capture (TRTLLM_PERF_TIME_EVENTS_PATH) by adding
the timing sources the executor cannot see, so a perf timeline can span
client-send -> router-dispatch -> worker admission/decode.
Item 4 (executor): enrich each per-rank JSONL record with the C++
RequestPerfMetrics lifecycle scalars (arrival / first_scheduled / first_token /
last_token / kv_cache_transfer start+end / kv_cache_size) via
executor.result.get_metrics_dict. Populated on the serve/disagg path (the
entrypoints force return_perf_metrics under the env); best-effort otherwise
(empty dict -> key omitted, never crashes the writer). These steady-clock
anchors give per-request admission/prefill/decode boundaries the relative
per-iteration time_breakdown_metrics cannot express.
Item 1 (client + router): two new optional capture files, each written by a
stdlib-only, torch-free writer (atexit flush) so the GPU-free import contract
holds:
* TRTLLM_PERF_TIME_EVENTS_ROUTER_PATH -- the disagg router stamps
arrival / ctx_dispatch / gen_dispatch / first_token / resp_done
(steady-clock, same epoch as the workers) keyed by ctx_request_id; gen-only
requests with no ctx id are collected separately.
* TRTLLM_PERF_TIME_EVENTS_CLIENT_PATH -- the benchmark client stamps
send/first_token/completion (perf_counter) + send_wall_time (time.time).
Standalone timeline: response_id is a fresh UUID, not a worker join key.
Aggregator: --router-dir / --client-dir (default to the two new env vars) join
the router record onto each worker record by ctx_request_id (exposed as
router_dispatch) and derive the cross-process spans
router_arrival_to_ctx_dispatch / router_ctx_to_gen_dispatch /
router_to_worker_arrival, plus the worker-side arrival_to_first_schedule /
schedule_to_first_token / decode_duration from request_timing_metrics.
Unmatched router rows (incl. gen-only) surface under unjoined_router_events;
client rows pass through untouched under client_events. Any one input suffices.
Tests (GPU-free): stdlib JsonlEventWriter contract (inert/drain/default=str/
idempotent); router+client parsers; enriched merge (join + all derived spans +
unjoined + standalone client payload); worker request_timing_metrics enrichment
(populated / empty / exception-swallowed).
Docs: document the two env vars, the --router-dir/--client-dir flags, the
lifecycle-scalar enrichment, and the standalone-client / shared-steady-clock
caveats in perf-analysis.md.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
|
Pushed Item 4 — executor per-request lifecycle scalars. Each per-rank JSONL record is enriched with the C++ Item 1 — client + disagg-router capture files (two new optional env vars, each written by a stdlib-only, torch-free writer with
Aggregator — Tests (GPU-free): stdlib writer contract, router+client parsers, enriched merge (join + all derived spans + unjoined + standalone client), and worker enrichment (populated / empty / exception-swallowed). Docs updated in |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/serve/openai_disagg_service.py (1)
386-412: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
on_gen_dispatchhook is not called on the generation-first schedule path.
_send_disagg_request_gen_firstcallshooks.on_ctx_dispatch(request)but neverhooks.on_gen_dispatch(...)before dispatching toself._gen_client.send_request(...). As a result,RawRequestResponseHooks.gen_dispatch_timestays0(→Nonein the emitted record) for any deployment usingschedule_style: generation_first, silently degrading the new router dispatch-timeline for that mode. Since the ctx-first path calls the hook right before gen-server resolution, consider adding an equivalent call here (e.g. right before line 403'sget_next_server/dispatch) for parity.🛠️ Proposed fix
disagg_request_id = await self._coordinator.get_disagg_request_id() if need_ctx: # arrival->here = pre-ctx wait in the orchestrator/fleet. if hooks: hooks.on_ctx_dispatch(request) ctx_server, ctx_server_info = await self._ctx_router.get_next_server( request, req_id=disagg_request_id ) ctx_req = self._get_ctx_request(request, disagg_request_id) + # Mark gen-dispatch start for the gen-first schedule style too. + if hooks: + hooks.on_gen_dispatch(request) gen_req = self._get_gen_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 `@tensorrt_llm/serve/openai_disagg_service.py` around lines 386 - 412, Update _send_disagg_request_gen_first to invoke hooks.on_gen_dispatch(request) before generation-server resolution or dispatch, matching the ctx-first path and ensuring generation-first requests record gen dispatch timing. Keep the existing on_ctx_dispatch call for context routing unchanged.
🧹 Nitpick comments (1)
tests/unittest/others/test_perf_time_events.py (1)
283-332: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative case where
request_idcollides with an unrelatedctx_request_id.This test uses a string ctx id (
"ctxA") and an intrequest_id(10), so it can never exercise the router join'srequest_idfallback (flagged onperf_time_events.pyLine 333). A record withrequest_id=10and a router row keyed"10"for a different request would currently join and emit bogusrouter_*spans.🤖 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_time_events.py` around lines 283 - 332, Extend test_router_join_on_ctx_id_and_derived_spans with a negative case where request_id 10 conflicts with an unrelated router row keyed by "10" while ctx_request_id remains different. Assert the router record is not joined, no bogus router_* derived spans are emitted, and the unrelated router event remains unjoined, covering the request_id fallback without changing the existing ctx_request_id match 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 `@tensorrt_llm/serve/openai_disagg_server.py`:
- Around line 115-143: Preserve ctx_request_id as None in
_maybe_write_router_event when no ctx round-trip occurred; do not substitute
disagg_request_id for it. Keep the existing disagg_request_id field for router
dispatch identity so parse_router_dir() continues routing gen-only records to
_no_ctx.
In `@tensorrt_llm/serve/perf_time_events_writer.py`:
- Around line 73-106: Ensure the `_writer_loop` OSError path transitions the
writer to an inert state when directory creation or file opening fails. Update
`write` and the writer-state handling so records are no longer enqueued after
that failure, and ensure `_writer_thread` cannot remain represented as an
active/draining writer. Preserve the existing warning while making the failure
immediately prevent further queue growth.
In `@tensorrt_llm/serve/scripts/backend_request_func.py`:
- Around line 71-82: Update async_request_openai_chat_completions to assign
send_time and send_wall_time at the same POST-send point as
async_request_openai_completions, and capture the returned response ID into
response_id. Preserve the existing timestamp sources and response-ID extraction
behavior so openai-chat perf-time-events records contain the same client
timeline data.
In `@tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py`:
- Around line 333-341: Update the router dispatch join around router_by_ctx to
look up only the worker record’s ctx_request_id, not both values in rids. Remove
the request_id-first iteration and retain matched_router_ctx tracking and
router_dispatch assignment only when the ctx-key lookup succeeds.
- Around line 105-124: Update the relevant function’s docstring to document the
actual "_no_ctx" sentinel used by merge(), and widen its return annotation/value
type to represent both per-context record dictionaries and the no-context record
list. Remove the unnecessary type: ignore on the "_no_ctx" assignment while
preserving the existing grouping behavior.
In `@tests/unittest/others/test_perf_time_events_writer.py`:
- Around line 30-95: Add tests/unittest/others/test_perf_time_events_writer.py
to the appropriate integration test-list entries under
tests/integration/test_lists/test-db/ and qa/ so all six TestJsonlEventWriter
and TestMakeEnvWriter tests are discovered by integration coverage.
In `@tests/unittest/others/test_perf_time_events.py`:
- Around line 364-395: Add tests/unittest/others/test_perf_time_events.py to the
appropriate CI test-list entry under the existing database or QA test-list
configuration, ensuring the listed test functions and helper coverage run
automatically. Do not modify the test implementations.
---
Outside diff comments:
In `@tensorrt_llm/serve/openai_disagg_service.py`:
- Around line 386-412: Update _send_disagg_request_gen_first to invoke
hooks.on_gen_dispatch(request) before generation-server resolution or dispatch,
matching the ctx-first path and ensuring generation-first requests record gen
dispatch timing. Keep the existing on_ctx_dispatch call for context routing
unchanged.
---
Nitpick comments:
In `@tests/unittest/others/test_perf_time_events.py`:
- Around line 283-332: Extend test_router_join_on_ctx_id_and_derived_spans with
a negative case where request_id 10 conflicts with an unrelated router row keyed
by "10" while ctx_request_id remains different. Assert the router record is not
joined, no bogus router_* derived spans are emitted, and the unrelated router
event remains unjoined, covering the request_id fallback without changing the
existing ctx_request_id match assertions.
🪄 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: a7cf282c-c07e-4a9b-acb4-669c1bb75863
📒 Files selected for processing (14)
docs/source/developer-guide/perf-analysis.mdtensorrt_llm/_torch/pyexecutor/perf_metrics_manager.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/serve/openai_disagg_server.pytensorrt_llm/serve/openai_disagg_service.pytensorrt_llm/serve/perf_time_events_writer.pytensorrt_llm/serve/responses_utils.pytensorrt_llm/serve/scripts/backend_request_func.pytensorrt_llm/serve/scripts/benchmark_serving.pytensorrt_llm/serve/scripts/perf_time_events/__init__.pytensorrt_llm/serve/scripts/perf_time_events/perf_time_events.pytests/unittest/_torch/executor/test_perf_metrics_events.pytests/unittest/others/test_perf_time_events.pytests/unittest/others/test_perf_time_events_writer.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tensorrt_llm/serve/scripts/perf_time_events/init.py
- docs/source/developer-guide/perf-analysis.md
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py
| class TestJsonlEventWriter(unittest.TestCase): | ||
| def test_inert_when_no_dir(self): | ||
| # Falsy events_dir -> inert: enabled False, write a no-op, close safe. | ||
| writer = JsonlEventWriter("", "disagg_router") | ||
| self.assertFalse(writer.enabled) | ||
| writer.write({"source": "disagg_router", "x": 1}) # must not raise | ||
| writer.close() # safe even though no thread ever started | ||
|
|
||
| def test_writes_one_line_per_record_and_drains_on_close(self): | ||
| with tempfile.TemporaryDirectory() as d: | ||
| writer = JsonlEventWriter(d, "disagg_router") | ||
| self.assertTrue(writer.enabled) | ||
| writer.write({"source": "disagg_router", "ctx_request_id": 1}) | ||
| writer.write({"source": "disagg_router", "ctx_request_id": 2}) | ||
| writer.close() # drains + joins the daemon thread | ||
|
|
||
| files = glob.glob(os.path.join(d, "disagg_router_pid*.jsonl")) | ||
| self.assertEqual(len(files), 1) | ||
| self.assertIn(f"pid{os.getpid()}", os.path.basename(files[0])) | ||
| with open(files[0]) as f: | ||
| lines = [json.loads(ln) for ln in f if ln.strip()] | ||
| self.assertEqual([r["ctx_request_id"] for r in lines], [1, 2]) | ||
|
|
||
| def test_non_serializable_value_falls_back_to_str(self): | ||
| # default=str keeps a non-JSON-native value from killing the writer | ||
| # thread (and losing every subsequent record). | ||
| with tempfile.TemporaryDirectory() as d: | ||
| writer = JsonlEventWriter(d, "client") | ||
| writer.write({"source": "client", "weird": {1, 2, 3}}) | ||
| writer.close() | ||
| files = glob.glob(os.path.join(d, "client_pid*.jsonl")) | ||
| self.assertEqual(len(files), 1) | ||
| with open(files[0]) as f: | ||
| lines = [json.loads(ln) for ln in f if ln.strip()] | ||
| self.assertEqual(len(lines), 1) | ||
| # The set was stringified rather than raising TypeError. | ||
| self.assertIsInstance(lines[0]["weird"], str) | ||
|
|
||
| def test_close_is_idempotent(self): | ||
| with tempfile.TemporaryDirectory() as d: | ||
| writer = JsonlEventWriter(d, "disagg_router") | ||
| writer.write({"source": "disagg_router"}) | ||
| writer.close() | ||
| writer.close() # second call is a no-op, must not raise | ||
|
|
||
|
|
||
| class TestMakeEnvWriter(unittest.TestCase): | ||
| def test_inert_when_env_unset(self): | ||
| with patch.dict(os.environ, {}, clear=False): | ||
| os.environ.pop(ROUTER_EVENTS_PATH_ENV, None) | ||
| writer = make_env_writer(ROUTER_EVENTS_PATH_ENV, "disagg_router") | ||
| self.assertFalse(writer.enabled) | ||
|
|
||
| def test_enabled_when_env_set(self): | ||
| with tempfile.TemporaryDirectory() as d: | ||
| with patch.dict(os.environ, {CLIENT_EVENTS_PATH_ENV: d}): | ||
| writer = make_env_writer(CLIENT_EVENTS_PATH_ENV, "client") | ||
| self.assertTrue(writer.enabled) | ||
| writer.write({"source": "client", "client_index": 0}) | ||
| writer.close() | ||
| files = glob.glob(os.path.join(d, "client_pid*.jsonl")) | ||
| self.assertEqual(len(files), 1) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'test_perf_time_events_writer' tests/integration/test_listsRepository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Test-list files:\n'
git ls-files 'tests/integration/test_lists/**' | sed -n '1,200p'
printf '\nOccurrences of test module name in test lists:\n'
rg -n 'test_perf_time_events_writer|test_perf_time|perf_time_events_writer' tests/integration/test_lists || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 4926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant mentions in integration test lists:\n'
rg -n 'unittest|tests/unittest|others/test_perf_time_events_writer|perf_time_events_writer|test_perf_time|pytest|python -m unittest' tests/integration/test_lists || true
printf '\nA few sample entries from list files:\n'
sed -n '1,120p' tests/integration/test_lists/qa/llm_perf_core.yml
printf '\n---\n'
sed -n '1,120p' tests/integration/test_lists/test-db/l0_perf.ymlRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
Add tests/unittest/others/test_perf_time_events_writer.py to the integration test lists
Changed tests: test_inert_when_no_dir, test_writes_one_line_per_record_and_drains_on_close, test_non_serializable_value_falls_back_to_str, test_close_is_idempotent, test_inert_when_env_unset, test_enabled_when_env_set. None of these appear in tests/integration/test_lists/test-db/ or qa/, so this module won’t be picked up by integration coverage. Coverage verdict: insufficient.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 48-48: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(files[0])
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 61-61: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(files[0])
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/unittest/others/test_perf_time_events_writer.py` around lines 30 - 95,
Add tests/unittest/others/test_perf_time_events_writer.py to the appropriate
integration test-list entries under tests/integration/test_lists/test-db/ and
qa/ so all six TestJsonlEventWriter and TestMakeEnvWriter tests are discovered
by integration coverage.
Source: Path instructions
| def test_client_events_standalone_in_payload(self): | ||
| with tempfile.TemporaryDirectory() as ev, tempfile.TemporaryDirectory() as cl: | ||
| _write( | ||
| os.path.join(ev, "time_events_rank0_pid1.jsonl"), | ||
| json.dumps(_make_record(10)) + "\n", | ||
| ) | ||
| _write( | ||
| os.path.join(cl, "client_pid1.jsonl"), | ||
| json.dumps( | ||
| { | ||
| "source": "client", | ||
| "client_index": 0, | ||
| "send_wall_time": 1.0, | ||
| "ttft": 0.1, | ||
| "latency": 2.0, | ||
| } | ||
| ) | ||
| + "\n", | ||
| ) | ||
| merger = PerfTimeEventsMerger() | ||
| merger.merge(event_dir=ev, client_dir=cl) | ||
| self.assertEqual(len(merger.client_events), 1) | ||
| self.assertEqual(merger.match_stats["num_client_events"], 1) | ||
| # Client records are NOT joined onto request records. | ||
| self.assertNotIn("client", merger.records[0]) | ||
|
|
||
| out = os.path.join(cl, "combined.json") | ||
| merger.write(out) | ||
| with open(out) as f: | ||
| payload = json.load(f) | ||
| self.assertEqual(len(payload["client_events"]), 1) | ||
| self.assertIn("unjoined_router_events", payload) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'test_perf_time_events' tests
rg -n 'unittest/others|test_perf_time_events' tests/integration/test_lists -g '*.yml' -g '*.txt' -g '*.yaml' | head -50Repository: NVIDIA/TensorRT-LLM
Length of output: 2491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== test file outline ==\n'
ast-grep outline tests/unittest/others/test_perf_time_events.py --view expanded || true
printf '\n== test file head/tail around relevant tests ==\n'
wc -l tests/unittest/others/test_perf_time_events.py
sed -n '1,260p' tests/unittest/others/test_perf_time_events.py
printf '\n--- later section ---\n'
sed -n '260,460p' tests/unittest/others/test_perf_time_events.py
printf '\n== integration list mentions ==\n'
rg -n 'test_perf_time_events\.py|unittest/others/test_perf_time_events\.py|unittest/others/' tests/integration/test_lists -g '*.yml' -g '*.yaml' -g '*.txt' || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 20601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in $(git ls-files 'tests/integration/test_lists/**/*.yml' 'tests/integration/test_lists/**/*.yaml' 'tests/integration/test_lists/**/*.txt'); do
if rg -n 'unittest/others/|test_perf_time_events\.py' "$f" >/dev/null; then
echo "### $f"
rg -n 'unittest/others/|test_perf_time_events\.py' "$f"
fi
doneRepository: NVIDIA/TensorRT-LLM
Length of output: 1766
Add tests/unittest/others/test_perf_time_events.py to the CI test lists
Changed test functions (all added; none modified or removed): TestParseRouterDir.test_keyed_by_ctx_id_and_no_ctx_collected, TestParseRouterDir.test_ignores_unrelated_files, TestParseClientDir.test_sorted_by_send_wall_time, TestParseClientDir.test_missing_wall_time_sorts_first, TestRouterAndClientMerge.test_router_join_on_ctx_id_and_derived_spans, TestRouterAndClientMerge.test_unjoined_router_events_include_no_ctx_and_leftovers, TestRouterAndClientMerge.test_client_events_standalone_in_payload (plus helper _router_record).
Test-list registration: tests/unittest/others/test_perf_time_events.py is not listed in any provided tests/integration/test_lists/test-db/ or tests/integration/test_lists/qa/ entry. Add it to the appropriate CI list so these tests run automatically.
Coverage verdict: insufficient.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 367-367: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_make_record(10))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 371-379: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"source": "client",
"client_index": 0,
"send_wall_time": 1.0,
"ttft": 0.1,
"latency": 2.0,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 391-391: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(out)
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/unittest/others/test_perf_time_events.py` around lines 364 - 395, Add
tests/unittest/others/test_perf_time_events.py to the appropriate CI test-list
entry under the existing database or QA test-list configuration, ensuring the
listed test functions and helper coverage run automatically. Do not modify the
test implementations.
Source: Path instructions
Add the per-request capacity-scheduler admission event (event 1 of the five per-step/per-chunk perf time events). Previously only a per-iteration starvation count existed for the capacity stage; the other four events (micro-batch admit, forward begin, sampler begin, decode/prefill end) already carried per-request timestamps. - SimpleScheduler stamps `_last_capacity_admit_time` via `steady_clock_now().total_seconds()` right after the capacity scheduler returns the fitting set, before micro-batch scheduling. Gated on a new `capture_admit_time` flag (default False) so the base scheduling path pays nothing. - PyExecutor flips the flag on only under `perf_manager.capture_extended`, and only when the scheduler exposes it (hasattr guard leaves PP / non-Simple schedulers untouched). It reads the stamp in `_prepare_and_schedule_batch` and threads it through `_compute_iter_batch_context` as `capacity_scheduled_time`, riding onto every admitted request's per-chunk / per-step entry. Same steady clock as `scheduled_time`, so the gap `scheduled_time - capacity_scheduled_time` isolates the micro-batch scheduling stage. Key is omitted when the scheduler does not expose the stamp. Adds GPU-free unit tests for the emit gating and the scheduler stamp. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/_torch/executor/test_perf_metrics_events.py`:
- Around line 398-406: Update test_stamp_refreshes_each_round to mock the
module-local steady_clock_now with two distinct sequential values, then assert
_last_capacity_admit_time equals the first value after the initial
schedule_request and the second value after the next call. Replace the
non-deterministic ordering assertion while preserving the existing two-round
setup.
- Around line 205-241: The current tests cover _compute_iter_batch_context() and
SimpleScheduler but not the PyExecutor handoff from _last_capacity_admit_time.
Add an executor-level test in test_perf_metrics_events.py that exercises the
PyExecutor scheduling path, verifies the stored _last_capacity_admit_time is
passed through, and confirms capacity_scheduled_time is emitted; also cover the
absent-value path if needed to preserve omission behavior.
🪄 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: 43ef280c-ebcd-4842-8d3f-0000dcdc422c
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/_torch/pyexecutor/scheduler/scheduler.pytests/unittest/_torch/executor/test_perf_metrics_events.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tensorrt_llm/_torch/pyexecutor/py_executor.py
| def test_admission_timestamps_emitted_when_given(self): | ||
| # Both scheduler-stage timestamps ride into the shared dict; the gap | ||
| # scheduled_time - capacity_scheduled_time isolates the micro-batch stage. | ||
| scheduled = SimpleNamespace( | ||
| num_context_requests=0, | ||
| num_generation_requests=1, | ||
| batch_size=1, | ||
| context_requests=[], | ||
| generation_requests=[SimpleNamespace(num_draft_tokens=0)], | ||
| ) | ||
| fake_self = SimpleNamespace(iter_counter=3) | ||
| ctx = PyExecutor._compute_iter_batch_context( | ||
| fake_self, | ||
| scheduled, | ||
| schedule_time=100.5, | ||
| capacity_admit_time=100.2, | ||
| ) | ||
| assert ctx["scheduled_time"] == 100.5 | ||
| assert ctx["capacity_scheduled_time"] == 100.2 | ||
| assert ctx["scheduled_time"] >= ctx["capacity_scheduled_time"] | ||
|
|
||
| def test_capacity_scheduled_time_omitted_when_absent(self): | ||
| # PP local scheduler / non-SimpleScheduler expose no admit time -> the | ||
| # key must simply not appear (no None leaking into the record). | ||
| scheduled = SimpleNamespace( | ||
| num_context_requests=0, | ||
| num_generation_requests=1, | ||
| batch_size=1, | ||
| context_requests=[], | ||
| generation_requests=[SimpleNamespace(num_draft_tokens=0)], | ||
| ) | ||
| fake_self = SimpleNamespace(iter_counter=1) | ||
| ctx = PyExecutor._compute_iter_batch_context( | ||
| fake_self, scheduled, schedule_time=5.0, capacity_admit_time=None | ||
| ) | ||
| assert ctx["scheduled_time"] == 5.0 | ||
| assert "capacity_scheduled_time" not in ctx |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
test_file='tests/unittest/_torch/executor/test_perf_metrics_events.py'
rg -n "test_perf_metrics_events|${test_file}" tests/integration/test_lists/test-db tests/integration/test_lists/qaRepository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the touched test file and nearby context
test_file='tests/unittest/_torch/executor/test_perf_metrics_events.py'
wc -l "$test_file"
sed -n '1,260p' "$test_file"
printf '\n--- tail ---\n'
sed -n '260,460p' "$test_file"
# Find the relevant integration list files and whether this test file/test names are listed
printf '\n--- list files ---\n'
find tests/integration/test_lists -maxdepth 2 -type f \( -path '*/test-db/*' -o -path '*/qa/*' \) | sort
printf '\n--- file/test-name matches in test-db and qa lists ---\n'
rg -n --hidden --no-messages \
-e 'test_perf_metrics_events|tests/unittest/_torch/executor/test_perf_metrics_events\.py|test_admission_timestamps_emitted_when_given|test_capacity_scheduled_time_omitted_when_absent' \
tests/integration/test_lists/test-db tests/integration/test_lists/qa || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 22019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden \
-e 'def _compute_iter_batch_context|capacity_admit_time|capture_admit_time|_last_capacity_admit_time|scheduled_time' \
tensorrt_llm/_torch/pyexecutor tensorrt_llm/_torch/pyexecutor/scheduler tests/unittest/_torch/executor/test_perf_metrics_events.py
printf '\n--- py_executor slice ---\n'
sed -n '1,260p' tensorrt_llm/_torch/pyexecutor/py_executor.py
printf '\n--- scheduler slice ---\n'
sed -n '1,260p' tensorrt_llm/_torch/pyexecutor/scheduler/scheduler.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 26880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any tests that exercise the production PyExecutor scheduling path
# with capacity admission timestamps / admit-time capture.
rg -n --hidden --glob '!**/.git/**' \
-e 'capacity_scheduled_time|capacity_admit_time|capture_admit_time|_last_capacity_admit_time|_compute_iter_batch_context|PyExecutor' \
tests/unittest tests/integration
printf '\n--- potential executor tests ---\n'
find tests/unittest -path '*pyexecutor*' -o -path '*executor*' | sortRepository: NVIDIA/TensorRT-LLM
Length of output: 37063
Add an executor-level handoff test. The new cases cover _compute_iter_batch_context() and SimpleScheduler in isolation, but not the PyExecutor path that reads _last_capacity_admit_time and emits capacity_scheduled_time; a wiring regression there would still pass these tests.
Coverage summary: test_admission_timestamps_emitted_when_given, test_capacity_scheduled_time_omitted_when_absent, and the SimpleScheduler admit-time tests are in tests/unittest/_torch/executor/test_perf_metrics_events.py; this file is not listed under tests/integration/test_lists/test-db/* or tests/integration/test_lists/qa/*. 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/_torch/executor/test_perf_metrics_events.py` around lines 205
- 241, The current tests cover _compute_iter_batch_context() and SimpleScheduler
but not the PyExecutor handoff from _last_capacity_admit_time. Add an
executor-level test in test_perf_metrics_events.py that exercises the PyExecutor
scheduling path, verifies the stored _last_capacity_admit_time is passed
through, and confirms capacity_scheduled_time is emitted; also cover the
absent-value path if needed to preserve omission behavior.
Source: Path instructions
| def test_stamp_refreshes_each_round(self): | ||
| sched = self._make_scheduler() | ||
| sched.capture_admit_time = True | ||
| sched.schedule_request([], set()) | ||
| first = sched._last_capacity_admit_time | ||
| sched.schedule_request([], set()) | ||
| second = sched._last_capacity_admit_time | ||
| # A new reading every round (monotonic non-decreasing). | ||
| assert second >= first |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the refresh assertion deterministic.
second >= first also passes when both timestamps are identical, so it would not detect a regression that stops refreshing _last_capacity_admit_time. Mock the module-local steady_clock_now with distinct values and assert both expected timestamps.
🤖 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/_torch/executor/test_perf_metrics_events.py` around lines 398
- 406, Update test_stamp_refreshes_each_round to mock the module-local
steady_clock_now with two distinct sequential values, then assert
_last_capacity_admit_time equals the first value after the initial
schedule_request and the second value after the next call. Replace the
non-deterministic ordering assertion while preserving the existing two-round
setup.
…hang Both the worker-side PerfMetricsManager writer and the stdlib JsonlEventWriter (router/client) could hang forever on shutdown: if the daemon writer thread had already exited after failing to open its output file, the bounded queue was full and never drained, so close()'s blocking queue.put(_WRITER_STOP) would block indefinitely on a consumer that was gone. Fix both writers symmetrically: - On an open() failure the writer thread now goes permanently inert -- clears _events_dir (so maybe_write_request_events / write() short-circuit and stop enqueueing) and drops the thread handle (so close() early-returns). - close() snapshots the thread handle first, uses a bounded put(_WRITER_STOP, timeout=5) guarded by except queue.Full, and joins with a timeout, so teardown is bounded regardless of the writer's state. - Open the JSONL files with encoding="utf-8" and dump with default=str so a non-serializable value degrades to its str form instead of killing the writer thread (and losing every later record). Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
…perf batch context The extended per-iteration batch context (_compute_iter_batch_context) counted attention-DP / CUDA-graph padding (is_dummy) requests, inflating exactly the iter_batch_size / num_ctx_requests / num_gen_requests / num_scheduled numbers this feature exists to expose, and skewing the derived starvation gap (num_capacity_fitting - num_scheduled). Filter dummy requests out of every count, matching the sibling _collect_scheduled_batch_stats collector and the acceptance-length aggregation. Also unify draft-token counting. The batch-level generation_token_number used `1 + num_draft_tokens` while the per-request req_generation_token_number used a different expression, so the two could disagree, and the raw num_draft_tokens includes trailing padding zeros. Add a single shared helper get_effective_draft_token_length() (prefers the C++ num_draft_tokens, falls back to counting non-zero py_draft_tokens so padding is never miscounted) and use it in both places so the per-iteration total and the per-request count can never diverge. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
The offline aggregator joined router dispatch records to worker records by iterating over both the worker request_id and its ctx_request_id, so a router record whose ctx_request_id numerically equalled an unrelated worker request_id would false-join. Worse, on the gen-only benchmark path the service hardcodes ctx_request_id=1 for every request (TRTLLM_DISAGG_BENCHMARK_GEN_ONLY), so parse_router_dir's last-write-wins map kept a single surviving row under key "1" and attached it to every worker record. Join strictly on ctx_request_id (the only key the router and worker genuinely share), and in parse_router_dir demote any ctx_request_id reported by more than one distinct request to the non-joinable "_no_ctx" bucket with a WARNING, so those rows surface as unjoined_router_events leftovers instead of false-joining. The KV-transfer join (legitimately keyed on request_id) is unchanged. Add regression tests: the duplicate-ctx-id demotion in parse_router_dir and the end-to-end gen-only ctx_request_id=1 no-false-join through the merger. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
…hmark path async_request_openai_chat_completions did not populate the perf-time-events client fields, so a chat-backend benchmark run produced no client_*.jsonl timeline (only the plain completions path did). Set output.send_time / output.send_wall_time right after the POST timestamp and capture the server response_id once in both the streaming and non-streaming branches, mirroring async_request_openai_completions. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
The two GPU-free unit test files (test_perf_time_events.py, test_perf_time_events_writer.py) were never listed in a test-db yml, so the unittest/others suite (file-by-file, not auto-collected) never ran them. Add both to l0_a10.yml next to the sibling test_time_breakdown.py entry. Also refresh the aggregator README: document the scheduled_time / capacity_scheduled_time admission timestamps (and the latter's omission under the merged KV-cache-manager-v2 scheduler), the dummy-request exclusion, the optional router/client companion timelines and their env vars, the request_timing_metrics record field, the --router-dir / --client-dir args, the updated combined-JSON shape, and the strict-ctx router_dispatch join with its gen-only collision handling. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py (2)
95-150: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftComplete the type contract for the aggregation API.
parse_router_dir()and the merger state useAny, so static checking cannot validate the JSON and router-record contracts.PerfTimeEventsMerger.__init__(),write(), andwrite_html()also omit theirNonereturn annotations.Define concrete event record types and represent the context-indexed records and no-context records with separate typed fields. Add
-> Noneto non-returning methods.As per coding guidelines: “Annotate every function, use
Nonefor non-returning functions, avoidAny.”Also applies to: 285-303, 478-507
🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 95 - 150, Complete the type contract for parse_router_dir and the merger by defining concrete typed router/event record types instead of Any, with separate types or fields for context-indexed records and the "_no_ctx" list. Apply these types to parse_router_dir’s return value and the merger state, and add -> None annotations to PerfTimeEventsMerger.__init__, write, and write_html (and any other non-returning functions in the affected code).Source: Coding guidelines
407-418: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not compute
router_to_worker_arrivalfrom unadjusted event timestamps.
PerfMetricsManagerwrites workerrequest_timing_metricsfrom the worker-local clock. The synchronization offset is applied only by/perf_metrics, not by the per-rank event writer. Apply the offset before writing event records, or omitrouter_to_worker_arrivalunless both timestamps use an aligned clock domain. Document this requirement inREADME.md.🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 407 - 418, The router_to_worker_arrival calculation in PerfMetricsManager must not subtract timestamps from different clock domains; apply the worker synchronization offset before event records are written, or omit this derived metric unless router arrival and worker arrival are aligned. Document the aligned-clock requirement in tensorrt_llm/serve/scripts/perf_time_events/README.md at lines 119-129.
♻️ Duplicate comments (1)
tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py (1)
505-514: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHandle only expected queue-full failures during writer shutdown.
Both writers suppress unrelated exceptions with
except Exception. This hides teardown defects and fails Ruff S110.
tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py#L505-L514: remove the broad handler. Keepqueue.Fullhandling and log a failed shutdown drain.tensorrt_llm/serve/perf_time_events_writer.py#L175-L184: apply the same narrow exception handling and stderr warning.As per coding guidelines, “Catch specific exceptions instead of using broad or bare exception handling such as
except:.”🤖 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/_torch/pyexecutor/perf_metrics_manager.py` around lines 505 - 514, In tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py (lines 505-514), remove the broad except Exception handler and replace it with stderr logging or warning output when the writer queue shutdown drain fails. Keep the queue.Full exception handling unchanged. In tensorrt_llm/serve/perf_time_events_writer.py (lines 175-184), apply the identical narrow exception handling pattern with stderr warning output instead of the broad except Exception handler. Both sites should only catch queue.Full and log/warn on unexpected failures rather than silently suppressing all exceptions during writer thread shutdown.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (4)
tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py (1)
48-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd precise type annotations to the extended capture interfaces.
__init__has no return annotation.iter_batch_context,response, andctx_request_idare untyped._extract_request_timing_metrics()returns a baredict.Use
Nonereturn annotations,|unions, parameterized mappings, and a narrow response protocol or concrete response type. Do not useAny.As per coding guidelines, “Annotate every function, use
Nonefor non-returning functions, avoidAnyand unnecessary type ignores, prefer built-in generic types and|.”Also applies to: 181-203, 363-448
🤖 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/_torch/pyexecutor/perf_metrics_manager.py` around lines 48 - 58, Add explicit annotations to __init__, iter_batch_context, and related extended-capture interfaces, using None for non-returning methods and | unions where values may be absent. Type response with a narrow protocol or existing concrete response type, and annotate ctx_request_id with its actual optional identifier type. Change _extract_request_timing_metrics() from bare dict to a parameterized built-in mapping type, avoiding Any and unnecessary type ignores throughout the affected methods.Source: Coding guidelines
tensorrt_llm/serve/perf_time_events_writer.py (1)
74-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete
JsonlEventWritertype annotations.
__init__has noNonereturn annotation.write()accepts a baredict. Usestr | Noneand a parameterized record type such asdict[str, object]or aTypedDict.As per coding guidelines, “Annotate every function, use
Nonefor non-returning functions, avoidAnyand unnecessary type ignores, prefer built-in generic types and|.”🤖 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/serve/perf_time_events_writer.py` around lines 74 - 107, Add a `-> None` return type annotation to the `__init__` method of JsonlEventWriter. Update the `events_dir` parameter type annotation from `Optional[str]` to `str | None`. In the `write` method, replace the bare `dict` type annotation for the `record` parameter with a more specific type such as `dict[str, object]` or a `TypedDict`, and add a `-> None` return type annotation to the method.Source: Coding guidelines
tensorrt_llm/_torch/pyexecutor/llm_request.py (1)
1237-1256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public helper interface.
Add
ArgsandReturnssections to this docstring. The helper is imported by other executor modules.Proposed documentation update
def get_effective_draft_token_length(request: LlmRequest) -> int: """Return the *effective* number of speculative draft tokens for a request. + + Args: + request: Request that provides speculative draft-token state. + + Returns: + Number of non-padding draft tokens for the current request step. Single source of truth for "how many draft tokens did this request actuallyAs per coding guidelines, “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/_torch/pyexecutor/llm_request.py` around lines 1237 - 1256, Update the get_effective_draft_token_length docstring to document its public interface with an Args section describing the request parameter and a Returns section describing the effective draft-token count. Preserve the existing behavior and explanatory documentation.Source: Coding guidelines
tensorrt_llm/_torch/pyexecutor/py_executor.py (1)
1910-1914: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
_compute_iter_batch_contextparameters and return mapping.
num_fitting_reqsis an optional integer. Both timestamps are optional floats. The mapping contains only integer and float values.Proposed type annotations
- num_fitting_reqs=None, - schedule_time=None, - capacity_admit_time=None) -> dict: + num_fitting_reqs: int | None = None, + schedule_time: float | None = None, + capacity_admit_time: float | None = None + ) -> dict[str, int | float]:🤖 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/_torch/pyexecutor/py_executor.py` around lines 1910 - 1914, Update _compute_iter_batch_context with type annotations: declare num_fitting_reqs as an optional integer, schedule_time and capacity_admit_time as optional floats, and annotate the returned dict as mapping string keys to integer or float values. Preserve the existing behavior and parameter defaults.Sources: Coding guidelines, Learnings
🤖 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/_torch/pyexecutor/py_executor.py`:
- Around line 1993-1994: Update the `num_capacity_fitting` assignment in the
scheduler context to exclude attention-DP dummy requests, using the same
`is_dummy` filtering already applied when computing `num_scheduled`. Keep
`num_scheduled` unchanged and ensure the fitting count reflects only real
requests.
In `@tests/unittest/others/test_perf_time_events.py`:
- Around line 386-410: Extend the regression coverage in
test_gen_only_ctx_id_1_does_not_false_join to add a worker record with
request_id="10" and ctx_request_id="ctxA", plus a router record whose
ctx_request_id="10". Assert the worker has no router_dispatch and the router
remains in merger.unjoined_router_events, preventing matching through the
worker-local request_id while preserving the existing duplicate-context
assertions.
---
Outside diff comments:
In `@tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py`:
- Around line 95-150: Complete the type contract for parse_router_dir and the
merger by defining concrete typed router/event record types instead of Any, with
separate types or fields for context-indexed records and the "_no_ctx" list.
Apply these types to parse_router_dir’s return value and the merger state, and
add -> None annotations to PerfTimeEventsMerger.__init__, write, and write_html
(and any other non-returning functions in the affected code).
- Around line 407-418: The router_to_worker_arrival calculation in
PerfMetricsManager must not subtract timestamps from different clock domains;
apply the worker synchronization offset before event records are written, or
omit this derived metric unless router arrival and worker arrival are aligned.
Document the aligned-clock requirement in
tensorrt_llm/serve/scripts/perf_time_events/README.md at lines 119-129.
---
Duplicate comments:
In `@tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py`:
- Around line 505-514: In tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py
(lines 505-514), remove the broad except Exception handler and replace it with
stderr logging or warning output when the writer queue shutdown drain fails.
Keep the queue.Full exception handling unchanged. In
tensorrt_llm/serve/perf_time_events_writer.py (lines 175-184), apply the
identical narrow exception handling pattern with stderr warning output instead
of the broad except Exception handler. Both sites should only catch queue.Full
and log/warn on unexpected failures rather than silently suppressing all
exceptions during writer thread shutdown.
---
Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/llm_request.py`:
- Around line 1237-1256: Update the get_effective_draft_token_length docstring
to document its public interface with an Args section describing the request
parameter and a Returns section describing the effective draft-token count.
Preserve the existing behavior and explanatory documentation.
In `@tensorrt_llm/_torch/pyexecutor/perf_metrics_manager.py`:
- Around line 48-58: Add explicit annotations to __init__, iter_batch_context,
and related extended-capture interfaces, using None for non-returning methods
and | unions where values may be absent. Type response with a narrow protocol or
existing concrete response type, and annotate ctx_request_id with its actual
optional identifier type. Change _extract_request_timing_metrics() from bare
dict to a parameterized built-in mapping type, avoiding Any and unnecessary type
ignores throughout the affected methods.
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 1910-1914: Update _compute_iter_batch_context with type
annotations: declare num_fitting_reqs as an optional integer, schedule_time and
capacity_admit_time as optional floats, and annotate the returned dict as
mapping string keys to integer or float values. Preserve the existing behavior
and parameter defaults.
In `@tensorrt_llm/serve/perf_time_events_writer.py`:
- Around line 74-107: Add a `-> None` return type annotation to the `__init__`
method of JsonlEventWriter. Update the `events_dir` parameter type annotation
from `Optional[str]` to `str | None`. In the `write` method, replace the bare
`dict` type annotation for the `record` parameter with a more specific type such
as `dict[str, object]` or a `TypedDict`, and add a `-> None` return type
annotation to the method.
🪄 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: 046160f9-ee05-4c61-9393-c052556a8781
📒 Files selected for processing (9)
tensorrt_llm/_torch/pyexecutor/llm_request.pytensorrt_llm/_torch/pyexecutor/perf_metrics_manager.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/serve/perf_time_events_writer.pytensorrt_llm/serve/scripts/backend_request_func.pytensorrt_llm/serve/scripts/perf_time_events/README.mdtensorrt_llm/serve/scripts/perf_time_events/perf_time_events.pytests/integration/test_lists/test-db/l0_a10.ymltests/unittest/others/test_perf_time_events.py
| ctx["num_capacity_fitting"] = num_fitting_reqs | ||
| ctx["num_scheduled"] = iter_batch_size |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 'num_fitting_requests|num_fitting' \
tensorrt_llm/_torch/pyexecutor/scheduler \
tensorrt_llm/_torch/pyexecutor/py_executor.py
rg -n -C4 'is_dummy|is_attention_dp_dummy|is_cuda_graph_dummy' \
tensorrt_llm/_torch/pyexecutor/scheduler \
tensorrt_llm/_torch/pyexecutor/py_executor.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 38655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline tensorrt_llm/_torch/pyexecutor/scheduler/scheduler.py
ast-grep outline tensorrt_llm/_torch/pyexecutor/scheduler/scheduler_v2.py
sed -n '1740,1910p' tensorrt_llm/_torch/pyexecutor/scheduler/scheduler.py
sed -n '180,270p' tensorrt_llm/_torch/pyexecutor/scheduler/scheduler_v2.py
rg -n -C8 'fitting_requests|is_dummy_request|is_cuda_graph_dummy|is_attention_dp_dummy|is_dummy' \
tensorrt_llm/_torch/pyexecutor/scheduler tensorrt_llm/_torch/pyexecutor/py_executor.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dummy flag definitions and assignments ---'
rg -n -C3 'is_dummy\s*=|is_dummy_request\s*=|is_attention_dp_dummy\s*=|is_cuda_graph_dummy\s*=' \
tensorrt_llm --glob '*.py' --glob '*.cpp' --glob '*.h' --glob '*.hpp'
printf '%s\n' '--- dummy creation and scheduling order ---'
rg -n -C12 '_pad_attention_dp_dummy_request|add_dummy_requests|cuda_graph_dummy|is_attention_dp_dummy' \
tensorrt_llm/_torch/pyexecutor/py_executor.py
printf '%s\n' '--- scheduler admission paths ---'
sed -n '3760,3840p' tensorrt_llm/_torch/pyexecutor/py_executor.py
sed -n '2660,2720p' tensorrt_llm/_torch/pyexecutor/py_executor.py
sed -n '500,620p' tensorrt_llm/_torch/pyexecutor/scheduler/scheduler.py
sed -n '250,430p' tensorrt_llm/_torch/pyexecutor/scheduler/scheduler_v2.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 47945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- LlmRequest dummy properties ---'
rg -n -C10 'def is_dummy|is_dummy_request|is_dummy\b' \
tensorrt_llm/_torch/pyexecutor/llm_request.py \
tensorrt_llm/_torch/pyexecutor/py_executor.py \
tensorrt_llm/_torch/pyexecutor/resource_manager.py \
tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py
printf '%s\n' '--- batch-size calculation and dummy filtering ---'
sed -n '1856,1998p' tensorrt_llm/_torch/pyexecutor/py_executor.py
printf '%s\n' '--- dummy request initial state ---'
sed -n '900,970p' tensorrt_llm/_torch/pyexecutor/resource_manager.py
sed -n '2935,2980p' tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py
rg -n -C5 'class LlmRequest|LlmRequest\(' tensorrt_llm/_torch/pyexecutor/llm_request.py | head -120Repository: NVIDIA/TensorRT-LLM
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dummy state assignment ---'
sed -n '2980,3035p' tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py
sed -n '960,1015p' tensorrt_llm/_torch/pyexecutor/resource_manager.py
printf '%s\n' '--- CUDA-graph dummy lifecycle ---'
sed -n '570,625p' tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py
rg -n -C8 'active_requests\.append|add_dummy_requests' \
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py
printf '%s\n' '--- static consistency check ---'
python3 - <<'PY'
from pathlib import Path
executor = Path("tensorrt_llm/_torch/pyexecutor/py_executor.py").read_text()
scheduler = Path("tensorrt_llm/_torch/pyexecutor/scheduler/scheduler.py").read_text()
scheduler_v2 = Path(
"tensorrt_llm/_torch/pyexecutor/scheduler/scheduler_v2.py"
).read_text()
request = Path("tensorrt_llm/_torch/pyexecutor/llm_request.py").read_text()
checks = {
"ADP dummy is appended before scheduling": (
"self.active_requests.append(llm_request)" in executor
and "_pad_attention_dp_dummy_request()" in executor
),
"dummy property includes dummy-request flag": (
"return self.is_attention_dp_dummy or self.is_cuda_graph_dummy or self.is_dummy_request"
in request
),
"V1 fitting count is raw fitting-list length": (
"num_fitting_requests=len(fitting_requests)" in scheduler
),
"V2 fitting count is raw scheduled-list length": (
"num_fitting_requests=(len(scheduled_encoder) + len(scheduled_ctx) + len(scheduled_gen))"
in scheduler_v2
),
"perf scheduled count filters dummy requests": (
"if not self._is_stats_dummy_request(req)" in executor
),
}
for name, result in checks.items():
print(f"{name}: {'YES' if result else 'NO'}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 11739
Exclude attention-DP dummy requests from num_capacity_fitting. The scheduler adds them to the fitting count, while num_scheduled excludes them through is_dummy. This inflates the starvation gap.
🤖 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/_torch/pyexecutor/py_executor.py` around lines 1993 - 1994,
Update the `num_capacity_fitting` assignment in the scheduler context to exclude
attention-DP dummy requests, using the same `is_dummy` filtering already applied
when computing `num_scheduled`. Keep `num_scheduled` unchanged and ensure the
fitting count reflects only real requests.
When perf-time-events capture is enabled, summarize the lifecycle intervals between time events as mean / P50 / P99 (+ min / max / n) and write them to an additional JSONL file via a new `-a/--agg-jsonl` flag on the offline merger (also honoring TRTLLM_PERF_TIME_EVENTS_AGG_PATH). One JSON object per line, one line per metric, in a stable order: the 12 canonical time_breakdown spans, the router dispatch chain, the custom worker spans, and the vLLM-named views (TTFT / TPOT / ITL / E2E). Rows are always emitted (never silently dropped); an interval never validly measured carries `status: not_recorded`, and each row records `clock_safe` so the two cross-clock-domain disagg spans are flagged rather than differenced. The aggregate path is pure stdlib -- it does NOT import time_breakdown / numpy / plotly, so the offline aggregate runs anywhere, not just inside the torch container. Validated on a real GB200 gpt-oss-120b NIXL gen_only capture; three refinements fell out of that data: - rank-dedup by (role, request_id): a tensor-parallel worker writes each request once per rank with lockstep-identical timings, so worker-derived groups dedup first, otherwise n inflates by the TP degree and fakes precision. - _infer_role keys on step_metrics / nonzero kv_cache_size, NOT kv_cache_transfer_start (ctx records carry a nonzero value there while gen records on the sync path carry 0.0), so roles are never misclassified. - the canonical-12 spec is hardcoded and guarded by a drift test that cross-checks time_breakdown.TimingMetricsConfig whenever it is importable. Also registers the previously orphaned test_perf_time_events.py in the l0_a10 CI test list. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py (1)
435-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the cross-server limit of the
request_iddedup key.The key is
request_idonly.request_idis assigned per executor, so two different logical requests handled by two different servers of the same role can share the same value. In that case the second sample is dropped andnunder-reports. The docstring states only the tensor-parallel rank case. Add the cross-server caveat so readers of the aggregate do not treatnas an exact request count.🤖 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/serve/scripts/perf_time_events/perf_time_events.py` around lines 435 - 456, The docstring for _dedup_records_by_role should document that request_id is only unique within an executor, so different servers with the same role may collide and have one sample dropped. Add this cross-server limitation and clarify that the resulting n is not necessarily an exact logical-request count; leave the deduplication behavior unchanged.tests/unittest/others/test_perf_time_events.py (1)
454-880: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary
- Changed test functions. Added:
TestAggregateHelpers.test_percentile_math,test_duration_excludes_missing_and_inverted,test_nz_treats_zero_as_missing,test_infer_role;TestAggregateMetrics.test_all_rows_present_and_ordered,test_empty_input_all_not_recorded,test_gen_arrival_to_first_scheduled_stats,test_rank_duplication_deduped,test_zeros_excluded,test_kv_span_not_recorded_on_zero_setters,test_ctx_forward_span_skips_warmup,test_router_chain_stats,test_router_no_ctx_bucket_included,test_itl_and_tpot_from_step_metrics,test_vllm_ttft_e2e_from_client_events,test_clock_safe_flags,test_canonical_rows_not_recorded_on_per_rank_capture,test_write_agg_jsonl_roundtrip;TestAggregatePathIsDependencyLight.test_no_toplevel_numpy_or_time_breakdown_import,test_canonical_matches_time_breakdown; helpers_row,_gen_record,_ctx_record,_disagg_perf_record, constant_EXPECTED_METRICS. Modified:test_gen_only_ctx_id_1_does_not_false_join(formatting only). Removed: none.- Test-list registration. The PR states the file is registered in
tests/integration/test_lists/test-db/l0_a10.yml. That list file is not supplied in this prompt, so registration is not confirmed here. Notests/integration/test_lists/qa/entry was supplied.- Coverage verdict: needs follow-up. Two gaps remain: no test exercises the
--agg-jsonlCLI option inmain()(onlywrite_agg_jsonlis called directly), and the request-ID collision regression for the router join is still absent.As per path instructions: "Always produce a test coverage summary, even if no issues are found."
🤖 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_time_events.py` around lines 454 - 880, Extend the aggregate test coverage in TestAggregateMetrics to invoke the main() CLI path with --agg-jsonl, verifying it writes the expected JSONL output, and add a regression test for router joins where request IDs collide across sources, ensuring events remain associated with the correct request. Preserve the existing direct write_agg_jsonl and router coverage.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/unittest/others/test_perf_time_events.py`:
- Around line 856-866: Update the module-scope import checks in the AST loop to
apply the existing time_breakdown prohibition to ast.Import aliases as well as
ast.ImportFrom modules, detecting the full dotted import name rather than only
its root for plain imports. Preserve the current numpy and ImportFrom checks.
---
Nitpick comments:
In `@tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.py`:
- Around line 435-456: The docstring for _dedup_records_by_role should document
that request_id is only unique within an executor, so different servers with the
same role may collide and have one sample dropped. Add this cross-server
limitation and clarify that the resulting n is not necessarily an exact
logical-request count; leave the deduplication behavior unchanged.
In `@tests/unittest/others/test_perf_time_events.py`:
- Around line 454-880: Extend the aggregate test coverage in
TestAggregateMetrics to invoke the main() CLI path with --agg-jsonl, verifying
it writes the expected JSONL output, and add a regression test for router joins
where request IDs collide across sources, ensuring events remain associated with
the correct request. Preserve the existing direct write_agg_jsonl and router
coverage.
🪄 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: 1d8faa3f-69bc-4c9c-a41a-5e4bd0a3b903
📒 Files selected for processing (2)
tensorrt_llm/serve/scripts/perf_time_events/perf_time_events.pytests/unittest/others/test_perf_time_events.py
| with open(pte.__file__) as f: | ||
| tree = ast.parse(f.read()) | ||
| banned = ("numpy", "plotly") | ||
| for node in tree.body: # module scope only | ||
| if isinstance(node, ast.Import): | ||
| for alias in node.names: | ||
| self.assertNotIn(alias.name.split(".")[0], banned) | ||
| elif isinstance(node, ast.ImportFrom): | ||
| root = (node.module or "").split(".")[0] | ||
| self.assertNotIn(root, banned) | ||
| self.assertNotIn("time_breakdown", (node.module or "")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the time_breakdown check to plain import statements too.
The docstring states the test guards against a module-scope import of numpy or time_breakdown. The time_breakdown assertion runs only in the ast.ImportFrom branch. A module-scope import tensorrt_llm.serve.scripts.time_breakdown.time_breakdown is an ast.Import whose root name is tensorrt_llm, so it passes both checks and the guard does not fire.
💚 Proposed fix
banned = ("numpy", "plotly")
for node in tree.body: # module scope only
if isinstance(node, ast.Import):
for alias in node.names:
self.assertNotIn(alias.name.split(".")[0], banned)
+ self.assertNotIn("time_breakdown", alias.name)
elif isinstance(node, ast.ImportFrom):
root = (node.module or "").split(".")[0]
self.assertNotIn(root, banned)
self.assertNotIn("time_breakdown", (node.module or ""))📝 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.
| with open(pte.__file__) as f: | |
| tree = ast.parse(f.read()) | |
| banned = ("numpy", "plotly") | |
| for node in tree.body: # module scope only | |
| if isinstance(node, ast.Import): | |
| for alias in node.names: | |
| self.assertNotIn(alias.name.split(".")[0], banned) | |
| elif isinstance(node, ast.ImportFrom): | |
| root = (node.module or "").split(".")[0] | |
| self.assertNotIn(root, banned) | |
| self.assertNotIn("time_breakdown", (node.module or "")) | |
| with open(pte.__file__) as f: | |
| tree = ast.parse(f.read()) | |
| banned = ("numpy", "plotly") | |
| for node in tree.body: # module scope only | |
| if isinstance(node, ast.Import): | |
| for alias in node.names: | |
| self.assertNotIn(alias.name.split(".")[0], banned) | |
| self.assertNotIn("time_breakdown", alias.name) | |
| elif isinstance(node, ast.ImportFrom): | |
| root = (node.module or "").split(".")[0] | |
| self.assertNotIn(root, banned) | |
| self.assertNotIn("time_breakdown", (node.module or "")) |
🤖 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_time_events.py` around lines 856 - 866,
Update the module-scope import checks in the AST loop to apply the existing
time_breakdown prohibition to ast.Import aliases as well as ast.ImportFrom
modules, detecting the full dotted import name rather than only its root for
plain imports. Preserve the current numpy and ImportFrom checks.
The shipped capture emits one compound JSON object per request at
completion (the worker record is gated on `time_breakdown_metrics`, which
`create_response` fills only on the final response). Two problems:
1. Hung requests capture nothing. A request that wedges (KV-transfer
timeout livelock at iter=0, fill-gate livelock, HangDetector MPI_Abort
at 300s) never completes, so the writer emits zero lines for exactly
the requests whose partial timeline would localize the stall.
2. Volume. The compound record carries a per-decode-step series
(~256-1024 entries) per request; the lifecycle signal is buried.
Redesign: write one line per lifecycle event, flushed as it fires, into
each rank's own log. Only the gen-init request's lifecycle is recorded --
decode requests emit no time events (the dense per-decode-step series is
dropped entirely). An offline compiler reads all per-rank worker logs +
router log + client log and pivots them long -> wide into one combined
JSONL with every request's time events. A request that never completes
leaves its partial timeline; the last event present pins the wedge.
Worker/router/client emit lines share one flat envelope:
{role, event, request_id, ctx_request_id, rank, t, pid, **extra}.
Changes:
- perf_time_events_writer.py: add make_event_record (flat envelope),
process-global worker writer + set_worker_rank, and emit_event
(worker-only, inert unless TRTLLM_PERF_TIME_EVENTS_PATH is set). The
sole steady-clock import is quarantined in _lazy_steady_clock_now so
the router/client stay torch-free.
- py_executor.py / seq_slot_manager.py: emit the gen-init lifecycle
events (arrival, gen_init_scheduled, KV-transfer start/end, first/last
token) and ctx events (arrival, first_scheduled, first_token,
ctx_ready_sent) at their lifecycle sites, all first-fire guarded and
env-gated. Adds the missing logger import in seq_slot_manager.py (the
gen-init branch's existing logger.info was a latent NameError).
- openai_disagg_server.py: RawRequestResponseHooks now emits one router
line per hook (arrival/ctx_dispatch/gen_dispatch/first_token/resp_done)
instead of one compound record at resp_done.
- Retire the completion-gated in-class worker writer
(maybe_write_request_events) and the extended per-iteration
batch-context path (capture_extended, _compute_iter_batch_context,
iter_batch_context, SimpleScheduler.capture_admit_time,
get_effective_draft_token_length).
- perf_time_events.py: rework ingestion to one line = one event, group by
(role, request_id), dedup TP ranks, join ctx<->gen<->router on
ctx_request_id, and pivot long -> wide into --events-jsonl
combined_time_events.jsonl (primary output). Stays torch-free.
- Rewrite tests to event-line fixtures + a hung-request test; delete the
retired compound-writer test.
- Update perf-analysis.md + scripts/.../README.md to the per-event model.
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The capture design (per-event append+flush, off-thread writer, offline long→wide compiler) is sound and the aggregator/writer tests are thorough. Three things before merge:
-
The PR description no longer matches the code. It describes
capture_extended, per-iteration batch context (iter_batch_size, starvation counts) riding insidetime_breakdown_metrics,PyExecutor.shutdown()joining the writer, and a test filetests/unittest/_torch/executor/test_pe…— none of which exist in this diff (grep confirms nocapture_extended/num_capacity_fittinganywhere; writer teardown is atexit-only; onlytests/unittest/others/tests are added). Please rewrite the description to match the per-event design the code actually implements — reviewers and future readers will otherwise review a PR that isn't there. -
/perf_metricscrash regression — see the inline comment on[openai_server.py:1389](https://github.com/NVIDIA/TensorRT-LLM/pull/16900/files#diff-2eb783dbd719ea8b2067105279f5a50460168065912d0efd2a3b2cdc0b3a78e5R1389); reproducible with one chat-completions request while the env var is set (and also for pre-existingperf_metrics_max_requests > 0+return_perf_metrics=Falseconfigs). -
Ticket: this is a substantial feature (+3435 lines, new env vars, new user-facing tool); it should carry a JIRA ticket rather than
[None].
Smaller: the chat-completions path never gets the env force-enable (inline), and the aggregator's TP-dedup assumption breaks with multiple ctx/gen server instances (inline). The pipeline-parallel gap is documented — fine as a follow-up. I could not run the new unit tests in my environment; please confirm the l0_a10 stage is green.
| if getattr(self.generator.args, "enable_iter_perf_stats", True): | ||
| self._iteration_stats_wakeup_event.set() | ||
| if self.generator.args.return_perf_metrics: | ||
| if self.generator.args.return_perf_metrics or self.perf_metrics is not None: |
There was a problem hiding this comment.
This widened gate appends items whose perf_metrics is None for any request that didn't set return_perf_metrics — notably every /v1/chat/completions request, since openai_chat has no env force-enable (see comment at line 1957). get_perf_metrics then does metrics.timing_metrics with no None guard (line 1298), so one chat request while TRTLLM_PERF_TIME_EVENTS_PATH is set makes GET /perf_metrics 500 with AttributeError: 'NoneType' object has no attribute 'timing_metrics'. It also regresses the pre-existing perf_metrics_max_requests > 0 + return_perf_metrics=False config, which previously appended nothing. Either skip the append when res.outputs[0].request_perf_metrics is None, or skip None entries in get_perf_metrics.
| # TODO: better way to enable metrics | ||
| if len(os.getenv("TRTLLM_KVCACHE_TIME_OUTPUT_PATH", "")) > 0: | ||
| if (len(os.getenv("TRTLLM_KVCACHE_TIME_OUTPUT_PATH", "")) > 0 | ||
| or len(os.getenv("TRTLLM_PERF_TIME_EVENTS_PATH", "")) > 0): |
There was a problem hiding this comment.
This force-enable exists on the completions path (here), the harmony path (line 2117), and responses ([responses_utils.py:891](https://github.com/NVIDIA/TensorRT-LLM/pull/16900/files#diff-7cdeb7d30f2f1098702aaba2e58365398261a69256a7f5a2b08628b3af05d3e8R891)) — but not in openai_chat. With only TRTLLM_PERF_TIME_EVENTS_PATH set, chat-completions requests reach the executor with return_perf_metrics=False, so every worker emit site is skipped and the per-rank files stay empty — for exactly the endpoint the benchmark client in this PR instruments. Consider adding the same check in openai_chat (or factoring the env check into one helper so the fourth copy can't drift).
|
|
||
| ### Primary output: `combined_time_events.jsonl` | ||
|
|
||
| One line per request. The compiler groups events by `request_id`, dedups the TP |
There was a problem hiding this comment.
Grouping worker events by request_id alone (_pivot_worker_events) collapses TP ranks correctly, but with more than one ctx (or gen) server instance writing into the same directory — a common disagg topology — the per-server request-id counters collide, and first-seen-wins silently merges two different requests' timestamps into one record (also corrupting the ctx_request_id join). pid can't disambiguate since TP ranks are distinct pids too. At minimum document a single-instance-per-role restriction here; a proper fix is a per-server instance token in the record or filename (the KV CSVs already carry an {instance} prefix you could mirror).
| # make close()'s put() block forever on an exited consumer) and drop | ||
| # the thread handle so close() early-returns. | ||
| self._events_dir = None | ||
| self._writer_thread = None |
There was a problem hiding this comment.
Setting self._writer_thread = None on open failure races with a concurrent write() that already passed the _events_dir is None check: _ensure_writer sees no thread and spawns a fresh queue+thread, which retries the failing open and warns again — once per racing write. Consider keeping the dead thread handle (or a _failed flag) so the writer goes permanently inert after the first failure; close() already tolerates a non-draining queue via the timed put.
|
|
||
| **Known limitations:** | ||
|
|
||
| * **Two clock domains.** The steady clocks split into domain A = {router, gen |
There was a problem hiding this comment.
Minor doc accuracy: the A/B domain split isn't intrinsic to the roles — steady-clock epochs are per-host (monotonic since boot), so the split is whatever the deployment's process placement happens to be. On a topology where router and ctx share a host, the doc's stated pairing is wrong in both directions. Worth rephrasing as "timestamps are only comparable between processes on the same host; in the reference deployment that meant {router, gen} vs {ctx, client}" — the aggregator itself is safe since it only differences within one record.
ZhanruiSunCh
left a comment
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part.
crazydemo
left a comment
crazydemo
left a comment
There was a problem hiding this comment.
Review summary - BLOCK
Verdict: Cannot merge: the branch is in conflict (mergeable_state: dirty) and, more importantly, the broadened /perf_metrics extraction guard introduces a reproducible crash regression on the chat-completions path when the new env var is set.
Issues
- [BLOCKER]
tensorrt_llm/serve/openai_server.py:1389- extraction guardor self.perf_metrics is not Noneruns on requests with no per-request perf metrics -> crash (chat-completions path is not force-enabled). - [MINOR]
tensorrt_llm/serve/openai_disagg_service.py:174-on_gen_dispatchwired only on the ctx-first path; generation_first loses router gen_dispatch events. - [NIT]
tensorrt_llm/serve/scripts/perf_time_events/__init__.py:35-__all__not sorted (Ruff RUF022).
QA view
- Test coverage: partial - the aggregator and the stdlib writer are well covered by the two new unit tests; the production instrumentation (py_executor emit sites, seq_slot_manager, and the openai_server:1389 guard = the crash path) has no test. The
test_perf_metrics_events.pyreferenced in the description is absent from this diff. - SM coverage: architecture-independent - pure Python timing capture and stdlib aggregation; no arch-guarded / fp8 / nvfp4 paths.
- Test code: no issues in the added tests themselves (deterministic, torch-free, drift guard present); the gap is what is left untested.
- Test time: small - two CPU-only torch-free files added to l0_a10; fast.
- Needs
/qa-verify: yes - a reproducible crash regression on an untested path, plus GPU e2e verification still in progress.
Possible new issues
- Chat-completions +
TRTLLM_PERF_TIME_EVENTS_PATHset: request enters the extraction block without perf metrics -> crash (also regresses pre-existingperf_metrics_max_requests>0+return_perf_metrics=False). generation_firstdisagg schedule style:gen_dispatch_timestays 0, so routergen_dispatchspans are silently missing.
What I could not verify
- The exact crash site inside
_extract_metricsis past the visible diff window (theitemconstruction beyond line 1392 is not shown), so I am relying on the diff's guard change plus reviewer brnguyen2's reproduction rather than tracing the failing dereference line-by-line. - Whether the merge conflict (
dirty) is trivial or substantive - only the mechanical signal is available. - Runtime overhead of the on-loop dict-build +
put_nowaitand the daemon-thread behavior under real load (the claimed near-zero A/B delta) cannot be confirmed from the diff.
Automated review by NVCortex Lite, run by @crazydemo.
| if getattr(self.generator.args, "enable_iter_perf_stats", True): | ||
| self._iteration_stats_wakeup_event.set() | ||
| if self.generator.args.return_perf_metrics: | ||
| if self.generator.args.return_perf_metrics or self.perf_metrics is not None: |
There was a problem hiding this comment.
[BLOCKER] /perf_metrics extraction guard broadened -> crash on requests without perf metrics
The guard changed from if self.generator.args.return_perf_metrics: to if self.generator.args.return_perf_metrics or self.perf_metrics is not None:. When TRTLLM_PERF_TIME_EVENTS_PATH is set, self.perf_metrics becomes a non-None deque even when the LLM-level return_perf_metrics=False. The env force-enable that flips sampling_params.return_perf_metrics=True is only wired into the completions / responses / request_preprocess paths (lines 1956-1957, 2116-2117 here, and responses_utils:891-892) — the chat-completions path is NOT force-enabled. So a single chat-completions request with the env var set enters this block while its res.outputs[0] carries no per-request perf metrics, and the subsequent item construction dereferences absent timing metrics -> crash. This is reproducible and was independently confirmed by reviewer brnguyen2. It also regresses the pre-existing perf_metrics_max_requests > 0 + return_perf_metrics=False configuration for the same reason. Fix: gate on the request actually having perf metrics (e.g. also check output.request_perf_metrics is not None) before building the item, or ensure the chat-completions path receives the same env force-enable so the metrics are always populated when this block runs.
if (self.generator.args.return_perf_metrics or self.perf_metrics is not None):
output = res.outputs[0]
if getattr(output, "request_perf_metrics", None) is None:
return # nothing to record for this request
...| # Mark gen-dispatch start: ctx-response (or arrival, on the no-ctx | ||
| # path) to here is the pre-generation wait in the orchestrator. | ||
| if hooks: | ||
| hooks.on_gen_dispatch(gen_req) |
There was a problem hiding this comment.
[MINOR] on_gen_dispatch wired only on ctx-first schedule path
on_gen_dispatch is invoked here in _send_disagg_request_ctx_first, but the generation-first schedule path (_send_disagg_request_gen_first, not in this diff) never calls it. For deployments using schedule_style: generation_first, RawRequestResponseHooks.gen_dispatch_time stays 0 -> the router gen_dispatch event and the router:*->gen_dispatch spans are silently absent for that mode. Not a crash, but the new router dispatch-timeline is incomplete for generation_first. Add an equivalent hooks.on_gen_dispatch(...) call before gen-server resolution in the gen-first path for parity.
| parse_router_dir, | ||
| ) | ||
|
|
||
| __all__ = [ |
There was a problem hiding this comment.
[NIT] all not sorted (Ruff RUF022)
Ruff RUF022 flags __all__ as unsorted. Reorder alphabetically (PerfTimeEventsMerger, main, parse_client_dir, parse_event_dir, parse_kv_csv_dir, parse_router_dir) to satisfy the linter. Cosmetic only.
crazydemo
left a comment
crazydemo
left a comment
There was a problem hiding this comment.
Review summary - Approve (non-blocking)
Approving so this is not blocked on me. The points raised in my review comment above are non-blocking — please read them and address what you agree with before merging.
Worth doing before this is relied on: A reproducible /perf_metrics crash regression exists on the chat-completions path with the env var set, and that path plus the executor instrumentation have no automated test. GPU end-to-end verification is still 'in progress' per the PR. A human QA should reproduce the crash, confirm the fix, and validate the l0_a10 stage is green.
Automated review by NVCortex Lite, run by @crazydemo.
JunyiXu-nv
left a comment
JunyiXu-nv
left a comment
There was a problem hiding this comment.
Requesting changes: the branch conflicts with main's perf-metrics and disagg-coordinator refactors (a mechanical rebase also crashes return_perf_metrics runs), and the ctx/gen join plus two timestamps need fixes. Details inline.
| # return_perf_metrics / perf_metrics_max_requests were not set. The | ||
| # per-rank live files remain the primary artifact; this is | ||
| # convenience. | ||
| if (self.perf_metrics is None |
There was a problem hiding this comment.
On main, #16668 removed /perf_metrics and this deque, so this block, get_perf_metrics, the L1389 gate and the compiler's --perf-json input all target code that no longer exists. The branch is about 3 months behind and conflicts in py_executor.py and several other files; main's DisaggTransferCoordinator refactor (#18827, #19128, #19266) moved the code the ctx_ready_sent and gen_kv_transfer_* emits hook into. #16668 also added perf_metrics_output_dir, which already forces return_perf_metrics for every endpoint, chat included. Could you rebase and gate capture on it instead of three new env vars?
| # server sees the rest). Inert unless TRTLLM_PERF_TIME_EVENTS_PATH is set. | ||
| for request in validated_requests: | ||
| if request.return_perf_metrics: | ||
| role = ("gen" if request.is_generation_only_request() |
There was a problem hiding this comment.
Main made LlmRequest.is_generation_only_request a property in #18858. This call and the one at seq_slot_manager.py:61 merge without a conflict, so after the rebase they raise TypeError: 'bool' object is not callable whenever return_perf_metrics is on, with or without the env var (any perf_metrics_output_dir or return_perf_metrics: true run). The call at L7014 sits in a conflict hunk and needs the same change. Please drop the () at all three.
| non-disagg runs or when disagg params are absent. | ||
| """ | ||
| params = getattr(request, "py_disaggregated_params", None) | ||
| return getattr(params, "ctx_request_id", |
There was a problem hiding this comment.
On the ctx worker py_disaggregated_params.ctx_request_id looks always None: _get_ctx_request only sets disagg_request_id, and gen learns ctx_request_id later from the ctx response, where it is the ctx request's own request_id. So every ctx_* line has ctx_request_id: null and the compiler never joins the ctx half. When I feed the writer and compiler those shapes, one request comes out as 2 records (ctx only, and gen + router). Did a real ctx+gen run join for you? For context-only requests, falling back to request.request_id here fixes it (1 record in the same repro).
| if request.return_perf_metrics: | ||
| role = ("gen" if request.is_generation_only_request() | ||
| else "ctx") | ||
| emit_event(role, f"{role}_arrival", |
There was a problem hiding this comment.
*_arrival is stamped here, after _pop_from_waiting_queue (gated by capacity and the fill throttle) and after ADP routing, so it is activation time, not arrival. A request held in waiting_queue because the worker is at max_num_active_requests writes no worker line at all, and arrival->*_scheduled leaves that wait out. That is the kind of stall this tool is for. Was that intended? Stamping when the request enters waiting_queue in _fetch_and_enqueue_requests would cover it.
| if (req.return_perf_metrics | ||
| and not getattr(req, "py_te_gen_kv_transfer_end", False)): | ||
| req.py_te_gen_kv_transfer_end = True | ||
| emit_event("gen", "gen_kv_transfer_end", |
There was a problem hiding this comment.
SEQ_SLOT_MANAGER.prepare_resources at L5909 has already emitted gen_first_scheduled for this request, so gen_kv_transfer_end lands a few microseconds after it. gen:kv_transfer_end->first_scheduled is then inverted, and _span_or_none always drops it. The wait between the transfer finishing and the request getting into a batch is counted in gen:kv_transfer_start->end instead. Suggest stamping it where the request turns DISAGG_GENERATION_TRANS_COMPLETE (after request_and_receive_sync / check_gen_transfer_status).
| """Daemon loop: drain the queue and append one JSON line per record.""" | ||
| path = os.path.join( | ||
| self._events_dir, | ||
| f"{self._filename_prefix}_pid{os.getpid()}.jsonl", |
There was a problem hiding this comment.
{prefix}_pid{P}.jsonl opened in append mode is only unique within one live PID namespace. ctx and gen in separate containers writing to a shared volume can both get time_events_rank0_pid<N>. A restarted container usually gets the same pid back and appends to the previous run's file, and the router's (pid, request_id) keys restart at 0 too. Main's PerfMetricsJsonlWriter names files {kind}-{hostname}-{pid}-{utc timestamp}. Could this do the same?
Description
Adds a single master switch,
TRTLLM_PERF_TIME_EVENTS_PATH(an outputdirectory, symmetric with
TRTLLM_KVCACHE_TIME_OUTPUT_PATH), that turns onper-request perf-timing capture and writes per-rank time-event files live from
inside the executor — exactly mirroring how the disaggregation transceiver
already writes per-rank KV-transfer CSVs (
_torch/disaggregation/native/perf_logger.py).Today, capturing this timing over the HTTP
/perf_metricsendpoint is fragile:the benchmark client must remember
--save-request-time-breakdown, the dequeonly exists when
perf_metrics_max_requests > 0,return_perf_metricsmust beset per worker, and the data must be pulled before the server tears down —
which for a disagg run means racing teardown across separate ctx/gen servers.
This PR replaces that with a live, per-rank, on-disk artifact and no HTTP scrape.
What the switch does
return_perf_metrics, and turns onan extended capture mode (
capture_extended). All new behavior is gatedon
capture_extended, so existingreturn_perf_metrics=Trueusers with theenv unset see no behavior or output change (regression guard: H3 below).
per-iteration metric dict:
iter_batch_size,num_ctx_requests,num_gen_requests,context_token_number,generation_token_number,per-request
req_context_token_number/req_generation_token_number, plus aper-iteration starvation count (
num_capacity_fittingvsnum_scheduled)— the signal that explains gen bubbles / scheduling stalls. These ride
inside the existing
time_breakdown_metricspayload, so there is no newpayload shape and no
create_responsechange.{dir}/time_events_rank{N}_pid{P}.jsonllive, one JSONL line perfinished request (rank+pid in the name avoids ctx-server/gen-server rank-0
collisions on a shared FS).
Off the critical path (overhead requirement)
The executor loop only builds a dict and does a non-blocking
queue.Queue.put_nowait— no file I/O on the loop thread. A lazily-starteddaemon writer thread drains the queue and does the actual
json.dumps + write + flush;PyExecutor.shutdown()drains and joins it.queue.Full→ warn + drop, never block. So the added on-loop cost is a dictbuild + queue append, keeping the A/B overhead delta structurally near-zero.
Optional offline aggregator
python -m tensorrt_llm.serve.scripts.perf_time_eventsstitches the per-rankfiles (+ the KV-transfer CSVs) into one combined JSON/HTML and computes derived
signals (inter-step / inter-chunk gaps, per-iteration starvation). It reuses
time_breakdown'sRequestTimeBreakdownfor HTML and is stdlib-only on theparse/merge/JSON path (torch-free;
plotlypulled in lazily only for--html),so it stays under the GPU-free import contract. It is a convenience over the
per-rank files, not the load-bearing capture path.
Python only — no C++. The KV sub-events already exist in Python for the
target
PYTHON-runtime (disagg NIXL) workload, so they are only ingested.Flagged Python-only compromises
attribution —
_schedule()returns onlynum_fitting_reqsin Python, sowhich specific requests were starved cannot be attributed without a C++ change.
_executor_loop_ppnever callssave_timing_to_requests, so the extendedfields are absent under pipeline parallelism (known gap, documented).
request's disagg
ctx_request_idwhen available, else it is an aggregator-sidejoin enhancement.
Test Coverage
GPU-free unit tests (both run on CPU; the executor-side one
importorskipstorch, the aggregator one is pure stdlib):
tests/unittest/_torch/executor/test_perf_metrics_events.pyappend_step_metricsmerges the batch-context dict + per-request tokencounts for ctx and gen (incl. draft tokens), and the disabled-shape guard
(
capture_extended=False→ none of the new keys, base shape untouched)._compute_iter_batch_contextagainst a fakeScheduledRequests.time_events_rank{N}_pid{P}.jsonlline with the right shape; no-ops whentime_breakdown_metricsis absent (non-final response) and when disabled.tests/unittest/others/test_perf_time_events.py— aggregatorparse_event_dir/parse_kv_csv_dir(native + gen-summary + C++ headers),mergejoin-by-request-and-ctx-id,unjoined_kv_events+ match-rate for anorphan rid, derived
inter_step_gaps/starved, combined-JSON write.Run:
GPU end-to-end verification (
gen_only ... gpt-oss-120b-fp4 ... dep2 ... NIXLon OCI-HSG GB200; no
--capture-nsysdue to the documented nsys-2026 hang onthis exact case) is in progress and will be posted as a follow-up comment:
per-rank files written with the extended keys, an A/B overhead comparison
(env unset vs set), and the regression guard (env unset → no
time_events_*file and unchanged
/perf_metricskeys).PR Checklist
api-compatible/api-breakinglabel needed): the LLM APIis untouched; only an env-gated, additive capture path.
docs/source/developer-guide/perf-analysis.mdgains a"Perf Time Events (per-rank live capture)" section (switch, JSONL record shape,
the three known limitations, aggregator recipe; cross-references
kv-transfer.md).Summary
Adds opt-in live per-rank performance time-event capture controlled by
TRTLLM_PERF_TIME_EVENTS_PATH.Dev Engineer Review
TRTLLM_PERF_TIME_EVENTS_PATH.ctx_request_idvalues.QA Engineer Review
Test changes
Added GPU-free and torch-free coverage in:
tests/unittest/_torch/executor/test_perf_metrics_events.pytests/unittest/others/test_perf_time_events.pytests/unittest/others/test_perf_time_events_writer.pyCI coverage
tests/integration/test_lists/test-db/l0_a10.ymlincludes:unittest/others/test_perf_time_events.pyunittest/others/test_perf_time_events_writer.pyAll added test files have CI test-list coverage. Verdict: sufficient.