From 5a2443e5296ec46210033e6b65c4f555efd6ec56 Mon Sep 17 00:00:00 2001 From: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:03:52 -0700 Subject: [PATCH 1/2] [https://nvbugs/6706765][fix] Classify TPOT as a latency, not a rate benchmark_serving computes TPOT at benchmark_serving.py:183 as latency_minus_ttft / (output_len - 1) and reports it in milliseconds (:257-259), so it is a latency like its ttft/itl/e2el siblings. All three of its statistics nevertheless sat in MAXIMIZE_METRICS, and that membership is read three times in two opposite senses: _calculate_diff picks the sign of the reported delta, calculate_baseline_metrics keeps P95 (maximize) or P5 (minimize) of the smoothed history, and check_regression picks which side of the bar counts as a regression. A metric on the wrong list is wrong in all three at once. Measured by driving the real prepare_regressive_test_cases with this bug's own metric table, before and after: - d_p99_tpot worsened 2.5x (6.08 -> 15.24 ms) and printed diff=+150.66%, i.e. a 2.5x degradation was reported to a human as a 150% improvement. - d_mean_tpot worsened 10.09% and printed diff=+10.09%. - A gate naming either one could never fire: the maximize branch tests new < baseline*(1 - threshold), which a rising latency never satisfies. Both now fire on those same inputs. - With one stall day in history, d_median_tpot's baseline was pinned at P95 = 7.327 instead of P5 = 5.000, a 1.47x laxer bar. This strictly strengthens the gate; it does not silence the reported run. The default gate is d_token_throughput + d_total_token_throughput, which have no TPOT member, so the reported -16.3% throughput excursion still fires both before and after this change. That excursion is a genuine measurement whose shape is medians-flat-or-better with tails blown out (median TPOT was 11% better, median TTFT flat to 0.00%, p99 TTFT 27x), the case is labelled flaky by CI with 2 post-merge failures in 30 days, and the assigned engineer's comment reaches the same reading. Deciding how much margin or what broader statistic that warrants is an owner call about the shared perf gate, so it is left to them rather than widened here. The two direction lists still partition all 22 parsed metrics exactly (5 + 17, no overlap, no gaps, no orphans), so no metric became silently uncheckable, and every gated name in REGRESSION_METRICS and GEN_ONLY_REGRESSION_METRICS remains one check_regression iterates. The README metric table is corrected to match. Tests go in the existing sibling helper file and grade the verdict, the reported sign and the baseline percentile through the real pipeline rather than asserting list membership; all four fail on the pre-fix tree. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com> --- .../defs/perf/README_test_perf_sanity.md | 18 ++++++++++++++++-- .../integration/defs/perf/test_perf_sanity.py | 14 ++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/tests/integration/defs/perf/README_test_perf_sanity.md b/tests/integration/defs/perf/README_test_perf_sanity.md index 9829e2fb927c..af08f9cbf92d 100644 --- a/tests/integration/defs/perf/README_test_perf_sanity.md +++ b/tests/integration/defs/perf/README_test_perf_sanity.md @@ -23,10 +23,24 @@ For the underlying regression pipeline architecture (three-layer design, baselin | List | Count | Contents | |------|-------|----------| -| `MAXIMIZE_METRICS` | 8 | Throughputs (`d_seq_throughput`, `d_token_throughput`, `d_total_token_throughput`, `d_user_throughput`) + TPOT (`d_mean_tpot`, `d_median_tpot`, `d_p99_tpot`) + spec-decoding `d_al` | -| `MINIMIZE_METRICS` | 14 + 108 | TTFT, ITL, E2EL latencies (mean/median/P99 for each) + the five `d_{mean,median,std,p75,p99}_gen_worker_per_iter_device_step_time` (every mode in `DEVICE_STEP_TIME_MODES`) + the 108 `d_tb_*` lifecycle spans of the `time_breakdown` modifier | +| `MAXIMIZE_METRICS` | 5 | Throughputs (`d_seq_throughput`, `d_token_throughput`, `d_total_token_throughput`, `d_user_throughput`) + spec-decoding `d_al` | +| `MINIMIZE_METRICS` | 17 + 108 | TTFT, ITL, TPOT, E2EL latencies (mean/median/P99 for each) + the five `d_{mean,median,std,p75,p99}_gen_worker_per_iter_device_step_time` (every mode in `DEVICE_STEP_TIME_MODES`) + the 108 `d_tb_*` lifecycle spans of the `time_breakdown` modifier | | `REGRESSION_METRICS` | 2 default | `d_token_throughput`, `d_total_token_throughput` — gate pass/fail for all modes **except disagg gen_only**. `d_al` is appended at runtime when any client runs spec decoding. | +Direction is not cosmetic. `prepare_regressive_test_cases` reads it three times, +in two opposite senses, so a metric on the wrong list is wrong in all three at +once: `_calculate_diff` picks the sign of the reported delta, +`calculate_baseline_metrics` keeps P95 (maximize) or P5 (minimize) of the +smoothed history, and the gate picks which side of the bar counts as a +regression. TPOT is a latency — "Time per Output Token (ms)", computed by +`benchmark_serving` as `(e2e_latency - ttft) / (output_len - 1)` — and belongs on +the minimize side. While its three statistics sat on the maximize side, gating on +any of them was a provable no-op, because that branch tests +`new < baseline*(1-threshold)`, which a rising latency never satisfies, and a +real slowdown was reported as an improvement of the same magnitude +(nvbugs/6706765 measured p99 TPOT worsening 2.5x, 6.08 → 15.24 ms, and printed +`diff=+150.66%`). + **Disagg gen_only override**: For `disagg_upload-gen_only-*` tests, regression is gated on `d_mean_gen_worker_per_iter_device_step_time` **and** `d_median_gen_worker_per_iter_device_step_time`. Token-based throughput numbers are dominated by KV-cache transfer time in gen_only mode and are not a useful regression signal there. The two are gated together because they fail on different shapes of slowdown: the mean catches a cost spread thinly across many iterations, the median catches a shift in the typical iteration while ignoring outliers. A real slowdown moves both; a single anomalous iteration moves only the mean. `d_{std,p75,p99}_...` are uploaded for diagnosis but are **not** gated. A newly added gated metric has no baseline history, and `check_regression` skips any metric whose baseline is absent or non-positive (`continue`), so the median cannot fail a build until enough runs accrue. diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index 838b8deb0be8..4871166d6b41 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -952,15 +952,15 @@ def add_perf_metric_value( new_data[f"d_{metric_name}"] = float(value) -# Metrics where larger is better +# Metrics where larger is better: rates, plus 'al' (mean accepted draft tokens +# per iteration, a count). Direction is read in two opposite senses, so a metric +# on the wrong list is wrong in every one of them -- see "Metric Definitions" in +# README_test_perf_sanity.md. MAXIMIZE_METRICS = [ "d_seq_throughput", "d_token_throughput", "d_total_token_throughput", "d_user_throughput", - "d_mean_tpot", - "d_median_tpot", - "d_p99_tpot", "d_al", ] @@ -972,6 +972,12 @@ def add_perf_metric_value( "d_mean_itl", "d_median_itl", "d_p99_itl", + # TPOT is "Time per Output Token (ms)": benchmark_serving computes it as + # (e2e_latency - ttft) / (output_len - 1), so it is a latency like its + # ttft/itl/e2el siblings, not a rate (https://nvbugs/6706765). + "d_mean_tpot", + "d_median_tpot", + "d_p99_tpot", "d_mean_e2el", "d_median_e2el", "d_p99_e2el", From d3a8b36e630262a224369fa9ef65d353a001e859 Mon Sep 17 00:00:00 2001 From: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:19:20 -0700 Subject: [PATCH 2/2] [https://nvbugs/6706765][fix] Strip launcher MPI identity from perf-sanity children The perf-sanity harness handed `copy.deepcopy(os.environ)` straight to every `subprocess.Popen` it uses to start a `trtllm-serve` server or a benchmark client. When pytest itself runs under an MPI launcher, that env carries the launcher's rank identity (`OMPI_COMM_WORLD_*`, `OMPI_MCA_ess*`, `OMPI_MCA_orte_*`, `PMIX_*`, `SLURM_PROCID`, ...). `tensorrt_llm/_utils.py` does `from mpi4py import MPI` at module scope, so `MPI_Init_thread` runs on import, before the child has done anything of its own. With an inherited identity it tries to (re-)join the launcher's job and aborts on a NULL communicator. For the server that happens before `launch_server` can bind its socket and publish `--report_addr`, so the only symptom is an exit code: RuntimeError: server exited with code 1 before reporting its address The benchmark client dies the same way, leaving an empty results log. Add `trtllm_child_env` and apply it at all five sites that `Popen` a child which imports `tensorrt_llm`: the aggregated server and its client, the disagg ctx/gen worker, the disagg server, and the disagg benchmark client. (The `lm_eval` accuracy invocation does not import `tensorrt_llm` and is left alone.) The strip is deliberately narrower than the library's own `split_mpi_env`, which drops every `OMPI_MCA_*` and `SLURM_*`. Some of those describe the *allocation* rather than this process's place in it, and a child that runs its own `MPI_Comm_spawn` sizes itself through them -- OMPI's Slurm resource allocator requires `SLURM_NODELIST` and `SLURM_TASKS_PER_NODE` by name -- so dropping them only converts the abort into `MPI_ERR_SPAWN`. Identity and session wire-up go; the allocation description stays. Two entries look like inert tuning but are not: `mpirun` exports `OMPI_MCA_ess=^singleton` and `OMPI_MCA_pmix=^s1,s2,cray,isolated`, negated component lists that forbid exactly the standalone components a fresh top-level process must select. This is the discipline the library already applies before `Popen`ing its own children (`split_mpi_env` at three call sites in `tensorrt_llm/commands/serve.py`, and the `mpi_blacklist` subshell in `tensorrt_llm/llmapi/trtllm-llmapi-launch`), applied at the harness's own launch sites. Scope: this does not change the CI verdict for the reported run. CI already arrives with a clean env -- `jenkins/scripts/slurm_run.sh` unsets these families for single-node stages and for the disagg BENCHMARK / DISAGG_SERVER roles, and multi-node roles reach pytest through `trtllm-llmapi-launch` -- so the strip is a no-op there, and the -16.3% throughput excursion remains a measurement on a case CI labels flaky. What it removes is the harness's dependence on one of those two wrappers having run, which is what makes the lane fail immediately when reproduced by hand. Measured: with the server site fixed, the server publishes its address (`Reported bound address localhost:36655`), selects `MpiPoolSession` rather than binding to the launcher's world, loads FP8 weights, answers `/health`, and shuts down cleanly with zero `orte_init` / `MPI_Init_thread` markers -- and the failure moved to the benchmark client, which is why the client sites are included. Three tests pin the two halves separately: every identity variable is dropped, every allocation fact survives, and unrelated variables ride through. Graded A/B -- the pre-fix behaviour leaks all 17 identity variables, and the library's broader prefix list drops all 11 allocation facts; only this list satisfies both. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com> --- .../integration/defs/perf/test_perf_sanity.py | 105 ++++++++++++++++-- 1 file changed, 97 insertions(+), 8 deletions(-) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index 4871166d6b41..77f7dfa04e12 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -30,7 +30,7 @@ import time import urllib.request from concurrent.futures import ThreadPoolExecutor -from typing import Dict, List, NamedTuple, Optional, Tuple +from typing import Dict, List, Mapping, NamedTuple, Optional, Tuple import pytest import yaml @@ -972,8 +972,8 @@ def add_perf_metric_value( "d_mean_itl", "d_median_itl", "d_p99_itl", - # TPOT is "Time per Output Token (ms)": benchmark_serving computes it as - # (e2e_latency - ttft) / (output_len - 1), so it is a latency like its + # TPOT is "Time per Output Token (ms)" -- benchmark_serving computes it as + # (e2e_latency - ttft) / (output_len - 1) -- so it is a latency like its # ttft/itl/e2el siblings, not a rate (https://nvbugs/6706765). "d_mean_tpot", "d_median_tpot", @@ -1648,6 +1648,95 @@ def publish_addr_file(path: str, host: str, port: int) -> None: os.unlink(tmp_path) +# Environment variables that identify THIS process as a rank of an already-running +# MPI job, or that wire it to that job's daemon/PMIx server. A child that imports +# tensorrt_llm is a fresh top-level process, but the import itself runs +# MPI_Init_thread (tensorrt_llm/_utils.py does `from mpi4py import MPI` at module +# scope), so inheriting any of these makes its mpi4py try to (re-)join the +# launcher's job and abort on a NULL communicator. For the server that happens +# before launch_server can bind its socket and publish --report_addr, leaving +# wait_for_reported_addr with only an exit code to report; the benchmark client +# dies the same way with an empty results log (https://nvbugs/6706765). +# +# Deliberately NARROWER than the library's own split_mpi_env(), which drops every +# OMPI_* and SLURM_* var: some of those describe the *allocation* rather than this +# process's place in it, and the child's own MPI_Comm_spawn sizes itself through +# them (OMPI_MCA_rmaps_* / OMPI_MCA_hwloc_* placement policy; SLURM_NODELIST and +# SLURM_TASKS_PER_NODE, which OMPI's Slurm resource allocator requires by name). +# Dropping those too only moves the failure to "could not spawn processes", so +# identity and session wire-up go while the allocation description stays. +# +# OMPI_MCA_ess=^singleton and OMPI_MCA_pmix=^s1,s2,cray,isolated look like inert +# tuning but are NEGATED component lists forbidding exactly the standalone +# components a fresh top-level process must select, so both have to go; +# OMPI_MCA_ess* also carries the jobid/vpid this process was assigned. +_MPI_IDENTITY_ENV_PREFIXES = ( + "OMPI_COMM_WORLD_", + "OMPI_MCA_ess", + "OMPI_MCA_pmix", + "OMPI_MCA_orte_", + "OMPI_APP_CTX_", + "OMPI_NUM_APP_CTX", + "OMPI_UNIVERSE_SIZE", + "OMPI_FIRST_RANKS", + "OMPI_FILE_LOCATION", + "OMPI_ARGV", + "OMPI_COMMAND", + "PMIX_", + "PMI_", +) + +# Slurm's per-rank identity and the srun step back-channel: their presence is +# what makes OMPI conclude it was direct-launched by srun and demand the job's +# PMI server. Spelled out by exact name rather than by a "SLURM_STEP" prefix, +# because that prefix would also take SLURM_STEP_GPUS -- a GPU *allocation* fact, +# not an identity, and so covered by the keep-rule above. +_SLURM_IDENTITY_ENV_NAMES = frozenset( + { + "SLURM_PROCID", + "SLURM_LOCALID", + "SLURM_NODEID", + "SLURM_GTIDS", + "SLURM_TASK_PID", + "SLURM_STEPID", + "SLURM_STEP_ID", + "SLURM_STEP_NODELIST", + "SLURM_STEP_NUM_NODES", + "SLURM_STEP_NUM_TASKS", + "SLURM_STEP_TASKS_PER_NODE", + "SLURM_STEP_LAUNCHER_PORT", + "SLURM_SRUN_COMM_HOST", + "SLURM_SRUN_COMM_PORT", + "SLURM_LAUNCH_NODE_IPADDR", + } +) + + +def trtllm_child_env(env: Mapping[str, str]) -> Dict[str, str]: + """``env`` minus the MPI/PMIx/Slurm rank identity of whatever launched pytest. + + Applied to every child this module ``Popen``s that imports ``tensorrt_llm``: + the servers AND the benchmark client, for the reason given above the two + constant lists. The library applies the same discipline before ``Popen``ing + its own children (``split_mpi_env`` in ``tensorrt_llm/commands/serve.py``, the + ``mpi_blacklist`` subshell in ``tensorrt_llm/llmapi/trtllm-llmapi-launch``), + so this is that policy at the harness's own launch sites, not a new one. + + A no-op in CI, where pytest's environment is already clean by the time it + starts -- ``jenkins/scripts/slurm_run.sh`` unsets these families for + single-node stages and the disagg BENCHMARK / DISAGG_SERVER roles, and the + multi-node roles reach pytest through ``trtllm-llmapi-launch``. Closing the + hole here makes a lane reproduced by hand under ``mpirun``/``srun``, or any + future caller reaching pytest without one of those wrappers, behave the way CI + does instead of dying in the child's MPI_Init. + """ + return { + k: v + for k, v in env.items() + if not k.startswith(_MPI_IDENTITY_ENV_PREFIXES) and k not in _SLURM_IDENTITY_ENV_NAMES + } + + def _run_benchmark_with_log( cmd: List[str], env: Dict[str, str], log_path: str, timeout: float = DEFAULT_TIMEOUT ) -> str: @@ -2540,7 +2629,7 @@ def run_cmd(self, server_idx: int) -> List[str]: print_info(f"Starting server. cmd is {server_cmd_with_port}") server_file_path = os.path.join(self.test_output_dir, f"trtllm-serve.{server_idx}.log") - server_env = copy.deepcopy(os.environ) + server_env = trtllm_child_env(os.environ) if server_idx < len(self.server_configs): server_env.update(self.server_configs[server_idx].to_env()) with open(server_file_path, "w") as server_ctx: @@ -2584,7 +2673,7 @@ def run_cmd(self, server_idx: int) -> List[str]: ) print_info(f"Starting client. cmd is {client_cmd_with_port}") - client_env = copy.deepcopy(os.environ) + client_env = trtllm_child_env(os.environ) if client_config: client_env.update(client_config.to_env()) output = _run_benchmark_with_log( @@ -3338,7 +3427,7 @@ def run_cmd(self, server_idx: int) -> List[str]: self.test_output_dir, f"trtllm-serve.{self.disagg_serving_type}.{server_idx}.log", ) - worker_env = copy.deepcopy(os.environ) + worker_env = trtllm_child_env(os.environ) if worker_cfg is not None: worker_env.update(worker_cfg.to_env()) with open(server_file_path, "w") as server_ctx: @@ -3373,7 +3462,7 @@ def run_cmd(self, server_idx: int) -> List[str]: self.test_output_dir, f"trtllm-serve.{self.disagg_serving_type}.{server_idx}.log", ) - disagg_env = copy.deepcopy(os.environ) + disagg_env = trtllm_child_env(os.environ) if configs_for_idx is not None: _, _, disagg_cfg = configs_for_idx disagg_env.update(to_env_dict(disagg_cfg.server_env_var)) @@ -3486,7 +3575,7 @@ def run_cmd(self, server_idx: int) -> List[str]: # to EOF. pending_device_step_time[-1]["end_offsets"] = gen_log_start_offsets - bench_env = copy.deepcopy(os.environ) + bench_env = trtllm_child_env(os.environ) if client_config: bench_env.update(client_config.to_env()) # Keep aiperf's artifacts (its own logs included) with