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..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 @@ -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", @@ -1642,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: @@ -2534,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: @@ -2578,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( @@ -3332,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: @@ -3367,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)) @@ -3480,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