From 34277ab0a758b1cf34577f534f56421fa6a32eb7 Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Tue, 1 Sep 2026 05:11:49 -0700 Subject: [PATCH 1/5] [None][test] Enable warmup request for disagg e2e and ctx_only perf sanity lanes #17098 enabled benchmark_serving's initial test request as a warmup for gen_only lanes; #18011 reverted it, because gen_only does not measure TTFT and the extra ctx->gen handover leaves a stale mSenderFutures entry that the CTX worker's blocking idle KV-transfer poll then waits on. Two other disagg lanes do want it, for two reasons that come to the same thing -- a one-time cold-start cost that otherwise lands inside the measured window: * e2e pays for the KV cache transceiver's lazy connection setup (ZMQ mesh + NIXL metadata registration) on the first handover, so until that has happened once the transfer runs well below steady-state bandwidth. * ctx_only forces osl=1, so the first cold prefill lands directly in the headline TTFT with nothing to amortize it. The initial test request is excluded from the reported metrics and reuses input_requests[0], so it carries the lane's own ISL/OSL -- which is what makes it an effective warmup rather than a token-sized probe. The effect scales as setup_cost/num_requests: measured on GB300 disagg e2e lanes, median TTFT drops ~49% at 8 requests and ~0.26% at 10240, so short lanes gain and long ones are unaffected. warmup is passed to ClientConfig as a constructor argument rather than through client_config_data, so no lane yaml can enable it. b_warmup is reported but is deliberately not a baseline match key -- warmup is a measurement-quality knob, not part of case identity, and forking history would hide the improvement in its own series -- and that is only sound while the value stays fully determined by benchmark_mode. b_warmup records the effective value: to_cmd dispatches to three builders and only the built-in benchmark_serving one has a test request to suppress, so a warmup requested on an agentx or nv_sa lane is recorded as False rather than claiming a warmup that never ran. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- .../integration/defs/perf/test_perf_sanity.py | 47 +++++- .../tools/test_perf_sanity_matching.py | 155 ++++++++++++++++++ 2 files changed, 201 insertions(+), 1 deletion(-) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index f99eaeee2535..cddb6e7787cf 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -1175,6 +1175,7 @@ def __init__( model_name: str, env_vars: str = "", spec_decoding: bool = False, + warmup: bool = False, ): self.model_name = model_name self.concurrency = client_config_data.get("concurrency", 1) @@ -1205,6 +1206,20 @@ def __init__( # agentx_client.py. Reported only -- see the s_benchmark_client note in # to_db_data for why it is not a match key. self.benchmark_client = client_config_data.get("benchmark_client", "") + # Deliberately a constructor argument and NOT a client_config_data key: + # b_warmup is not a baseline match key, which is only sound while the + # value stays fully determined by benchmark_mode. Reading it from + # client_config_data would let any lane yaml enable it and silently fork + # that lane's baseline history. + # + # Recorded as the EFFECTIVE value, the same convention as + # b_disable_overlap_scheduler. Only the built-in benchmark_serving client + # has an initial test request to reuse as a warmup; the agentx and nv_sa + # clients are built by separate to_cmd branches that emit no such flag, + # so a requested warmup there would never run while b_warmup claimed it + # did -- and a later investigator would rule warmup out as a cause it + # never had. + self.warmup = warmup and not (self.benchmark_client or self.use_nv_sa_benchmark) self.env_vars = env_vars # spec_decoding flag is retained for DB matching (b_eos column). --ignore-eos # is now always passed; output-length stability with spec decoding comes from @@ -1311,11 +1326,15 @@ def _to_default_benchmark_cmd(self) -> List[str]: str(self.concurrency * self.iterations), "--max-concurrency", str(self.concurrency), - "--no-test-input", "--percentile-metrics", "ttft,tpot,itl,e2el", "--ignore-eos", ] + # benchmark_serving's initial single-prompt test run is excluded from the + # reported metrics, which is exactly what makes it usable as a warmup + # request. Keep it suppressed unless the lane asked for one. + if not self.warmup: + benchmark_cmd.append("--no-test-input") if dataset_path: benchmark_cmd.append("--dataset-name") benchmark_cmd.append("trtllm_custom") @@ -1369,6 +1388,7 @@ def to_db_data(self) -> dict: "b_streaming": self.streaming, "b_trust_remote_code": self.trust_remote_code, "b_use_nv_sa_benchmark": self.use_nv_sa_benchmark, + "b_warmup": self.warmup, # Reported, not matched. Case identity is keyed on s_test_case_name # (plus GPU type, runtime, branch), and a disagg case name embeds its # config stem, so an agentx lane already forms its own population by @@ -2540,11 +2560,36 @@ def _parse_disagg_config_file(self, config_file_path: str, config_file: str): "accuracy_config": accuracy_data, "only_run_accuracy": only_run_accuracy, } + # Two disagg lanes want benchmark_serving's initial test request, for + # two different reasons that come to the same thing: a one-time + # cold-start cost that otherwise lands inside the measured window. + # + # * e2e pays for the KV cache transceiver's lazy connection setup + # (ZMQ mesh + NIXL metadata registration) on the first ctx->gen + # handover; until that has happened once the transfer runs at a + # fraction of steady-state bandwidth, so every measured request in + # a short lane is charged for it. + # * ctx_only forces osl=1, so the very first (cold) prefill lands + # directly in the headline TTFT with nothing to amortize it. + # + # gen_only is deliberately excluded: it does not measure TTFT, and + # the extra handover leaves a stale mSenderFutures entry that the CTX + # worker's blocking idle KV-transfer poll then waits on (see #18011). + # + # Unlike gen_only there is no concurrency restriction here, because + # the GEN fill gate (TLLM_BENCHMARK_REQ_QUEUES_SIZE) is only injected + # for gen_only lanes, so a lone warmup request cannot stall behind it. + # + # Passed as an argument rather than folded into client_config_data + # above, so that no lane yaml can reach it -- b_warmup is not a + # baseline match key, and that is only sound while the value stays + # fully determined by benchmark_mode. client_config = ClientConfig( client_config_data, model_name, env_vars=client_env_var, spec_decoding=spec_decoding, + warmup=benchmark_mode in ("e2e", "ctx_only"), ) client_configs.append(client_config) diff --git a/tests/unittest/tools/test_perf_sanity_matching.py b/tests/unittest/tools/test_perf_sanity_matching.py index 84904cd225e2..289fa302efc2 100644 --- a/tests/unittest/tools/test_perf_sanity_matching.py +++ b/tests/unittest/tools/test_perf_sanity_matching.py @@ -279,3 +279,158 @@ def test_a_pre_merge_branch_does_not_match_post_merge_history() -> None: pre_merge_data = _benchmark_data(s_branch="github-pr-12345") assert not benchmark_data_matches(history, pre_merge_data, get_test_case_match_keys()) + + +def test_warmup_lets_the_initial_test_request_through() -> None: + """A warmup lane drops --no-test-input, which is what creates the warmup. + + benchmark_serving's initial test request is excluded from the reported + metrics, so it is the cheapest available warmup. It reuses + input_requests[0], hence carries the lane's own ISL and OSL: on a disagg e2e + lane it absorbs the KV cache transceiver's one-time lazy connection setup + (ZMQ mesh + NIXL metadata registration) that otherwise slows the first + measured ctx->gen handover, and on a ctx_only lane it is a full-ISL prefill + that absorbs the first cold prefill out of the reported TTFT. + """ + client_config = _load_client_config() + + cold = client_config(_disagg_client_data(10), "example_model") + warm = client_config(_disagg_client_data(10), "example_model", warmup=True) + + assert "--no-test-input" in cold._to_default_benchmark_cmd() + assert "--no-test-input" not in warm._to_default_benchmark_cmd() + + +def test_warmup_cannot_be_enabled_from_lane_config() -> None: + """A "warmup" key in a lane yaml must not reach ClientConfig.warmup. + + b_warmup is deliberately not a match key (see + test_match_keys_are_name_and_environment_only), so warmed results merge into + the same baseline history as their cold predecessors. That is only sound + while the value stays fully determined by benchmark_mode. Both config + parsers hand the raw yaml client dict straight to ClientConfig, so if warmup + were read from it, any lane -- including an aggregated one -- could enable + warmup for itself and silently fork its own baseline history with no visible + config difference. Hence the constructor argument. + """ + client_config = _load_client_config() + + from_yaml = client_config({**_disagg_client_data(10), "warmup": True}, "example_model") + + assert from_yaml.warmup is False + assert from_yaml.to_db_data()["b_warmup"] is False + assert "--no-test-input" in from_yaml._to_default_benchmark_cmd() + + +def test_warmup_is_suppressed_for_the_non_default_benchmark_clients() -> None: + """b_warmup records the EFFECTIVE value, not the requested one. + + to_cmd dispatches to three builders, and only the built-in + benchmark_serving one has an initial test request to suppress. The agentx + and nv_sa builders emit no equivalent flag, so a requested warmup would not + happen there -- and a b_warmup=True row for a run that never warmed up is + worse than no row at all: it invites a later investigator to rule warmup out + as a cause it never had. Same convention as b_disable_overlap_scheduler, + which also reports what the run actually did. + """ + client_config = _load_client_config() + + nv_sa = client_config( + {**_disagg_client_data(10), "use_nv_sa_benchmark": True}, "example_model", warmup=True + ) + agentx = client_config( + {**_disagg_client_data(10), "benchmark_client": "agentx"}, "example_model", warmup=True + ) + default = client_config(_disagg_client_data(10), "example_model", warmup=True) + + assert nv_sa.warmup is False + assert nv_sa.to_db_data()["b_warmup"] is False + assert agentx.warmup is False + assert agentx.to_db_data()["b_warmup"] is False + assert default.warmup is True + assert default.to_db_data()["b_warmup"] is True + + +def test_warmup_defaults_off_and_is_reported() -> None: + """Every other lane keeps today's behaviour, and the DB records which warmed.""" + client_config = _load_client_config() + + cold = client_config(_disagg_client_data(10), "example_model") + warm = client_config(_disagg_client_data(10), "example_model", warmup=True) + + assert cold.warmup is False + assert cold.to_db_data()["b_warmup"] is False + assert warm.to_db_data()["b_warmup"] is True + + +def test_warmup_is_derived_from_exactly_the_e2e_and_ctx_only_modes() -> None: + """Pin warmup to benchmark_mode, the reason b_warmup can skip the match key. + + _parse_disagg_config_file cannot be called here -- PerfSanityTestConfig's + constructor shells out to nvidia-smi and raises without a GPU -- so the + mapping is asserted against its source, in the same way as + test_disagg_iterations_come_from_multi_round above. The tests above cover + what ClientConfig does with the value; only this one covers which lanes get + it. + + The mode set is asserted exactly, not by substring: a membership test + against a tuple still "contains 'e2e'" after gen_only is added to it, so a + substring check would wave through the one lane #18011 established must not + warm up. + + This is coupled to the shape of the expression on purpose, because the + expression is the contract. If you are here because you refactored it (say + to warmup=_wants_warmup(benchmark_mode)), that is fine -- but the set of + warmed lanes is review-relevant, so move this assertion to the new home of + the mode set rather than deleting it. + """ + repo_root = pathlib.Path(__file__).resolve().parents[3] + module_path = repo_root / "tests" / "integration" / "defs" / "perf" / "test_perf_sanity.py" + tree = ast.parse(module_path.read_text()) + + warmup_kwargs = [ + keyword.value + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "ClientConfig" + for keyword in node.keywords + if keyword.arg == "warmup" + ] + + assert len(warmup_kwargs) == 1, ( + f"expected exactly one ClientConfig(warmup=...) call site, found {len(warmup_kwargs)}; " + "warmup must stay determined by benchmark_mode in one place" + ) + value = warmup_kwargs[0] + assert isinstance(value, ast.Compare) and len(value.ops) == 1, ( + "warmup= is no longer a single comparison against benchmark_mode; b_warmup " + "is not a baseline match key, so a lane-settable warmup would silently " + "fork history" + ) + assert isinstance(value.ops[0], ast.In), "warmup= no longer tests mode membership" + assert "benchmark_mode" in ast.dump(value.left), "warmup= is not derived from benchmark_mode" + container = value.comparators[0] + assert isinstance(container, (ast.Tuple, ast.List, ast.Set)), ( + "the warmup modes are no longer a literal container, so this test can no " + "longer verify which lanes warm up" + ) + assert all(isinstance(elt, ast.Constant) for elt in container.elts) + assert {elt.value for elt in container.elts} == {"e2e", "ctx_only"}, ( + "the set of warmup lanes changed; e2e absorbs the KV transceiver's lazy " + "connection setup and ctx_only absorbs the first cold prefill, while " + "gen_only must stay excluded (#18011: the extra handover leaves a stale " + "mSenderFutures entry the CTX worker waits on)" + ) + + +def test_warmup_is_not_a_match_key() -> None: + """Warmup is a measurement-quality knob, not part of case identity. + + Making b_warmup a match key would fork all ~26 warmed lanes into a second + tracked series and make the improvement invisible in its own history -- a + permanent cost to paper over a one-time step. The four match keys are + identity, hardware, runtime and branch; none of them describes how well the + run was set up. Same rationale as s_benchmark_client. + """ + assert "b_warmup" not in get_test_case_match_keys() From 141c985dd1c8a46f633bd9b64df14f4ecaa21415 Mon Sep 17 00:00:00 2001 From: Chenfei Zhang Date: Tue, 1 Sep 2026 19:46:21 -0700 Subject: [PATCH 2/5] [None][test] Address review: name the warmup mode set and test it by behaviour Review follow-ups on the warmup-request change: * Extract the mode -> warmup decision into WARMUP_BENCHMARK_MODES / wants_warmup(), so the set of warmed lanes has one named home instead of being an expression at the call site. * Replace the AST-shape assertion on that expression with a behavioural test of wants_warmup() over every mode the disagg parser can see. A behaviour-preserving refactor no longer fails with a message claiming the warmup lane set changed. * Keep the one property that genuinely needs the source -- that no second producer hands ClientConfig a warmup value -- as a separate test scoped to the _parse_disagg_config_file FunctionDef rather than walking the module. * Document on the b_warmup column why it is reported but not matched, and the asymmetry that follows: a later revert compares a cold run against a warmed baseline, which this column is how you diagnose. Signed-off-by: Chenfei Zhang --- .../integration/defs/perf/test_perf_sanity.py | 54 +++++--- .../tools/test_perf_sanity_matching.py | 122 +++++++++++------- 2 files changed, 111 insertions(+), 65 deletions(-) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index cddb6e7787cf..c496dd41900c 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -59,6 +59,31 @@ # (agentx_client.py). Any other non-empty value is rejected at parse time. AGENTX_BENCHMARK_CLIENT = "agentx" +# The benchmark modes whose lanes get a warmup request, i.e. whose measured +# window would otherwise be charged for a one-time setup cost: +# * e2e absorbs the KV cache transceiver's lazy connection setup (ZMQ mesh + +# NIXL metadata registration) on the first ctx->gen handover. +# * ctx_only forces osl=1, so the very first (cold) prefill lands directly in +# the headline TTFT with nothing to amortize it. +# gen_only is deliberately absent: it does not measure TTFT, and the extra +# handover leaves a stale mSenderFutures entry that the CTX worker's blocking +# idle KV-transfer poll then waits on (see #18011). +# +# Named rather than inlined at the call site so the set of warmed lanes can be +# asserted by behaviour instead of by the shape of an expression. +WARMUP_BENCHMARK_MODES = ("e2e", "ctx_only") + + +def wants_warmup(benchmark_mode: str) -> bool: + """Whether a lane in this benchmark mode should issue a warmup request. + + The single source of truth for which lanes warm up. b_warmup is not a + baseline match key, and that is only sound while the value stays fully + determined by benchmark_mode rather than being settable per lane. + """ + return benchmark_mode in WARMUP_BENCHMARK_MODES + + BENCH_SERVING_REPO = "https://github.com/kedarpotdar-nv/bench_serving.git" BENCH_SERVING_COMMIT = "f3ea022a5780de5d0babc5fffa53634e2023d28f" BENCH_SERVING_DIR = "/tmp/bench_serving" @@ -1388,6 +1413,16 @@ def to_db_data(self) -> dict: "b_streaming": self.streaming, "b_trust_remote_code": self.trust_remote_code, "b_use_nv_sa_benchmark": self.use_nv_sa_benchmark, + # Reported, not matched -- see test_warmup_is_not_a_match_key. Not + # matching means a warmed lane's first post-merge run compares against + # cold history, i.e. a one-time step in the baseline. That is the same + # shape as any perf improvement and only ever helps a threshold that + # alarms on slowdowns, but the reverse direction is asymmetric: if + # warmup is later reverted or disabled on a lane, a cold run is + # compared against a warmed baseline and reads as a large regression + # (measured: up to ~24% on a ~30s lane, <0.1% on a >1000s one) with no + # match key to explain it. This column is how that is diagnosed -- + # diff b_warmup across the step before hunting for a code cause. "b_warmup": self.warmup, # Reported, not matched. Case identity is keyed on s_test_case_name # (plus GPU type, runtime, branch), and a disagg case name embeds its @@ -2560,21 +2595,8 @@ def _parse_disagg_config_file(self, config_file_path: str, config_file: str): "accuracy_config": accuracy_data, "only_run_accuracy": only_run_accuracy, } - # Two disagg lanes want benchmark_serving's initial test request, for - # two different reasons that come to the same thing: a one-time - # cold-start cost that otherwise lands inside the measured window. - # - # * e2e pays for the KV cache transceiver's lazy connection setup - # (ZMQ mesh + NIXL metadata registration) on the first ctx->gen - # handover; until that has happened once the transfer runs at a - # fraction of steady-state bandwidth, so every measured request in - # a short lane is charged for it. - # * ctx_only forces osl=1, so the very first (cold) prefill lands - # directly in the headline TTFT with nothing to amortize it. - # - # gen_only is deliberately excluded: it does not measure TTFT, and - # the extra handover leaves a stale mSenderFutures entry that the CTX - # worker's blocking idle KV-transfer poll then waits on (see #18011). + # Which modes warm up, and why, is documented on + # WARMUP_BENCHMARK_MODES / wants_warmup above. # # Unlike gen_only there is no concurrency restriction here, because # the GEN fill gate (TLLM_BENCHMARK_REQ_QUEUES_SIZE) is only injected @@ -2589,7 +2611,7 @@ def _parse_disagg_config_file(self, config_file_path: str, config_file: str): model_name, env_vars=client_env_var, spec_decoding=spec_decoding, - warmup=benchmark_mode in ("e2e", "ctx_only"), + warmup=wants_warmup(benchmark_mode), ) client_configs.append(client_config) diff --git a/tests/unittest/tools/test_perf_sanity_matching.py b/tests/unittest/tools/test_perf_sanity_matching.py index 289fa302efc2..d18c037dfeb5 100644 --- a/tests/unittest/tools/test_perf_sanity_matching.py +++ b/tests/unittest/tools/test_perf_sanity_matching.py @@ -69,13 +69,13 @@ def test_matching_ignores_tuning_changes() -> None: assert benchmark_data_matches(previous_data, updated_data, get_test_case_match_keys()) -def _load_client_config() -> type: - """Return the real ClientConfig, without the integration-test packages. +def _load_module() -> types.ModuleType: + """Import test_perf_sanity.py without the integration-test packages. - The derived-name rule is only worth testing against the class that owns it; - re-implementing the f-string here would assert nothing. test_perf_sanity.py - reaches torch and the OpenSearch client through its imports, so those are - stubbed -- ClientConfig.__init__ touches none of them. + Rules under test are only worth asserting against the code that owns them; + re-implementing them here would assert nothing. test_perf_sanity.py reaches + torch and the OpenSearch client through its imports, so those are stubbed -- + ClientConfig.__init__ and wants_warmup touch none of them. """ repo_root = pathlib.Path(__file__).resolve().parents[3] module_path = repo_root / "tests" / "integration" / "defs" / "perf" / "test_perf_sanity.py" @@ -132,7 +132,12 @@ def noop(*args: object, **kwargs: object) -> None: sys.modules.pop(name, None) else: sys.modules[name] = previous - return module.ClientConfig + return module + + +def _load_client_config() -> type: + """Return the real ClientConfig.""" + return _load_module().ClientConfig def _disagg_client_data(multi_round: int) -> dict[str, object]: @@ -366,61 +371,80 @@ def test_warmup_defaults_off_and_is_reported() -> None: def test_warmup_is_derived_from_exactly_the_e2e_and_ctx_only_modes() -> None: """Pin warmup to benchmark_mode, the reason b_warmup can skip the match key. - _parse_disagg_config_file cannot be called here -- PerfSanityTestConfig's - constructor shells out to nvidia-smi and raises without a GPU -- so the - mapping is asserted against its source, in the same way as - test_disagg_iterations_come_from_multi_round above. The tests above cover + Asserted through wants_warmup rather than against the source text: the set + of warmed lanes is the contract, the expression that computes it is not, so + a behaviour-preserving refactor must not fail here. The tests above cover what ClientConfig does with the value; only this one covers which lanes get it. - The mode set is asserted exactly, not by substring: a membership test - against a tuple still "contains 'e2e'" after gen_only is added to it, so a - substring check would wave through the one lane #18011 established must not - warm up. + Every mode the disagg parser can see is checked explicitly, so adding a mode + without deciding whether it warms up fails here rather than silently + inheriting a default. gen_only in particular must stay excluded: #18011 + established that the extra handover leaves a stale mSenderFutures entry that + the CTX worker's blocking idle KV-transfer poll then waits on. + """ + module = _load_module() + + assert module.wants_warmup("e2e") is True + assert module.wants_warmup("ctx_only") is True + assert module.wants_warmup("gen_only") is False + assert module.wants_warmup("gen_only_no_context") is False + assert module.wants_warmup("") is False - This is coupled to the shape of the expression on purpose, because the - expression is the contract. If you are here because you refactored it (say - to warmup=_wants_warmup(benchmark_mode)), that is fine -- but the set of - warmed lanes is review-relevant, so move this assertion to the new home of - the mode set rather than deleting it. + # Pinned as an exact set, not by substring: a membership test against a + # tuple still "contains e2e" after gen_only has been added to it. + assert set(module.WARMUP_BENCHMARK_MODES) == {"e2e", "ctx_only"}, ( + "the set of warmup lanes changed; e2e absorbs the KV transceiver's lazy " + "connection setup and ctx_only absorbs the first cold prefill, while " + "gen_only must stay excluded (#18011)" + ) + + +def test_warmup_reaches_client_config_only_from_the_disagg_parser() -> None: + """No second producer may hand ClientConfig a warmup value. + + Locality, not expression shape: b_warmup is not a baseline match key, which + is only sound while one code path decides warmup for every lane. A second + ClientConfig(warmup=...) call site elsewhere -- notably in the aggregated + parser, which forwards lane yaml keys through verbatim -- would reintroduce + exactly the lane-settable warmup that test_warmup_cannot_be_enabled_from_ + lane_config forbids by value. + + _parse_disagg_config_file cannot be called directly here (PerfSanityTest- + Config's constructor shells out to nvidia-smi and raises without a GPU), so + this one property is checked against the source, scoped to that function. """ repo_root = pathlib.Path(__file__).resolve().parents[3] module_path = repo_root / "tests" / "integration" / "defs" / "perf" / "test_perf_sanity.py" tree = ast.parse(module_path.read_text()) - warmup_kwargs = [ - keyword.value + def warmup_call_sites(node: ast.AST) -> list[ast.AST]: + return [ + call + for call in ast.walk(node) + if isinstance(call, ast.Call) + and isinstance(call.func, ast.Name) + and call.func.id == "ClientConfig" + and any(keyword.arg == "warmup" for keyword in call.keywords) + ] + + disagg_parsers = [ + node for node in ast.walk(tree) - if isinstance(node, ast.Call) - and isinstance(node.func, ast.Name) - and node.func.id == "ClientConfig" - for keyword in node.keywords - if keyword.arg == "warmup" + if isinstance(node, ast.FunctionDef) and node.name == "_parse_disagg_config_file" ] + assert len(disagg_parsers) == 1, "expected exactly one _parse_disagg_config_file" - assert len(warmup_kwargs) == 1, ( - f"expected exactly one ClientConfig(warmup=...) call site, found {len(warmup_kwargs)}; " - "warmup must stay determined by benchmark_mode in one place" + inside = warmup_call_sites(disagg_parsers[0]) + total = warmup_call_sites(tree) + assert len(inside) == 1, ( + f"expected exactly one ClientConfig(warmup=...) in _parse_disagg_config_file, " + f"found {len(inside)}" ) - value = warmup_kwargs[0] - assert isinstance(value, ast.Compare) and len(value.ops) == 1, ( - "warmup= is no longer a single comparison against benchmark_mode; b_warmup " - "is not a baseline match key, so a lane-settable warmup would silently " - "fork history" - ) - assert isinstance(value.ops[0], ast.In), "warmup= no longer tests mode membership" - assert "benchmark_mode" in ast.dump(value.left), "warmup= is not derived from benchmark_mode" - container = value.comparators[0] - assert isinstance(container, (ast.Tuple, ast.List, ast.Set)), ( - "the warmup modes are no longer a literal container, so this test can no " - "longer verify which lanes warm up" - ) - assert all(isinstance(elt, ast.Constant) for elt in container.elts) - assert {elt.value for elt in container.elts} == {"e2e", "ctx_only"}, ( - "the set of warmup lanes changed; e2e absorbs the KV transceiver's lazy " - "connection setup and ctx_only absorbs the first cold prefill, while " - "gen_only must stay excluded (#18011: the extra handover leaves a stale " - "mSenderFutures entry the CTX worker waits on)" + assert len(total) == len(inside), ( + f"ClientConfig(warmup=...) appears {len(total) - len(inside)} time(s) outside " + "_parse_disagg_config_file; warmup must be decided in exactly one place, or " + "b_warmup stops being fully determined by benchmark_mode" ) From b4fd35be3b7da2af268f59ba841e9865d2df18ba Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:12:30 -0700 Subject: [PATCH 3/5] [None][chore] clarify the warmup suppression condition Replace not (benchmark_client or use_nv_sa_benchmark) with a named local and note that benchmark_client defaults to "" rather than None. Review feedback: the original read as though it were always false, since a reader who assumes the default is None concludes the condition never holds. De Morgan equivalent; behaviour unchanged and pinned by test_warmup_is_suppressed_for_the_non_default_benchmark_clients. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- tests/integration/defs/perf/test_perf_sanity.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index c496dd41900c..14f861835d12 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -1244,7 +1244,10 @@ def __init__( # so a requested warmup there would never run while b_warmup claimed it # did -- and a later investigator would rule warmup out as a cause it # never had. - self.warmup = warmup and not (self.benchmark_client or self.use_nv_sa_benchmark) + # NB: benchmark_client defaults to "" (not None), so an ordinary lane is + # falsy here and does get its warmup. + uses_default_benchmark_client = not self.benchmark_client and not self.use_nv_sa_benchmark + self.warmup = warmup and uses_default_benchmark_client self.env_vars = env_vars # spec_decoding flag is retained for DB matching (b_eos column). --ignore-eos # is now always passed; output-length stability with spec decoding comes from From de424f13c6879c1a6f8b51590cf3a88ec50d4173 Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:19:14 -0700 Subject: [PATCH 4/5] [None][chore] suppress warmup on the agentx value, not any non-empty client to_cmd selects the agentx builder with == AGENTX_BENCHMARK_CLIENT, so suppression must use the same condition. Testing truthiness of benchmark_client diverged for any other non-empty value: the aggregated parser does not validate the key, so such a lane had its warmup suppressed while to_cmd still dispatched to the default benchmark_serving client, which supports it. Add a test asserting the emitted command rather than b_warmup, which is driven by self.warmup and so cannot disagree with it. Drop the explanatory comments from test_perf_sanity.py. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- .../integration/defs/perf/test_perf_sanity.py | 62 +------------------ .../tools/test_perf_sanity_matching.py | 39 ++++++++++++ 2 files changed, 42 insertions(+), 59 deletions(-) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index 14f861835d12..fd54b950d1ae 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -59,28 +59,11 @@ # (agentx_client.py). Any other non-empty value is rejected at parse time. AGENTX_BENCHMARK_CLIENT = "agentx" -# The benchmark modes whose lanes get a warmup request, i.e. whose measured -# window would otherwise be charged for a one-time setup cost: -# * e2e absorbs the KV cache transceiver's lazy connection setup (ZMQ mesh + -# NIXL metadata registration) on the first ctx->gen handover. -# * ctx_only forces osl=1, so the very first (cold) prefill lands directly in -# the headline TTFT with nothing to amortize it. -# gen_only is deliberately absent: it does not measure TTFT, and the extra -# handover leaves a stale mSenderFutures entry that the CTX worker's blocking -# idle KV-transfer poll then waits on (see #18011). -# -# Named rather than inlined at the call site so the set of warmed lanes can be -# asserted by behaviour instead of by the shape of an expression. WARMUP_BENCHMARK_MODES = ("e2e", "ctx_only") def wants_warmup(benchmark_mode: str) -> bool: - """Whether a lane in this benchmark mode should issue a warmup request. - - The single source of truth for which lanes warm up. b_warmup is not a - baseline match key, and that is only sound while the value stays fully - determined by benchmark_mode rather than being settable per lane. - """ + """Whether a lane in this benchmark mode should issue a warmup request.""" return benchmark_mode in WARMUP_BENCHMARK_MODES @@ -1231,23 +1214,8 @@ def __init__( # agentx_client.py. Reported only -- see the s_benchmark_client note in # to_db_data for why it is not a match key. self.benchmark_client = client_config_data.get("benchmark_client", "") - # Deliberately a constructor argument and NOT a client_config_data key: - # b_warmup is not a baseline match key, which is only sound while the - # value stays fully determined by benchmark_mode. Reading it from - # client_config_data would let any lane yaml enable it and silently fork - # that lane's baseline history. - # - # Recorded as the EFFECTIVE value, the same convention as - # b_disable_overlap_scheduler. Only the built-in benchmark_serving client - # has an initial test request to reuse as a warmup; the agentx and nv_sa - # clients are built by separate to_cmd branches that emit no such flag, - # so a requested warmup there would never run while b_warmup claimed it - # did -- and a later investigator would rule warmup out as a cause it - # never had. - # NB: benchmark_client defaults to "" (not None), so an ordinary lane is - # falsy here and does get its warmup. - uses_default_benchmark_client = not self.benchmark_client and not self.use_nv_sa_benchmark - self.warmup = warmup and uses_default_benchmark_client + run_agentx_mode = self.benchmark_client == AGENTX_BENCHMARK_CLIENT + self.warmup = warmup and not (run_agentx_mode or self.use_nv_sa_benchmark) self.env_vars = env_vars # spec_decoding flag is retained for DB matching (b_eos column). --ignore-eos # is now always passed; output-length stability with spec decoding comes from @@ -1358,9 +1326,6 @@ def _to_default_benchmark_cmd(self) -> List[str]: "ttft,tpot,itl,e2el", "--ignore-eos", ] - # benchmark_serving's initial single-prompt test run is excluded from the - # reported metrics, which is exactly what makes it usable as a warmup - # request. Keep it suppressed unless the lane asked for one. if not self.warmup: benchmark_cmd.append("--no-test-input") if dataset_path: @@ -1416,16 +1381,6 @@ def to_db_data(self) -> dict: "b_streaming": self.streaming, "b_trust_remote_code": self.trust_remote_code, "b_use_nv_sa_benchmark": self.use_nv_sa_benchmark, - # Reported, not matched -- see test_warmup_is_not_a_match_key. Not - # matching means a warmed lane's first post-merge run compares against - # cold history, i.e. a one-time step in the baseline. That is the same - # shape as any perf improvement and only ever helps a threshold that - # alarms on slowdowns, but the reverse direction is asymmetric: if - # warmup is later reverted or disabled on a lane, a cold run is - # compared against a warmed baseline and reads as a large regression - # (measured: up to ~24% on a ~30s lane, <0.1% on a >1000s one) with no - # match key to explain it. This column is how that is diagnosed -- - # diff b_warmup across the step before hunting for a code cause. "b_warmup": self.warmup, # Reported, not matched. Case identity is keyed on s_test_case_name # (plus GPU type, runtime, branch), and a disagg case name embeds its @@ -2598,17 +2553,6 @@ def _parse_disagg_config_file(self, config_file_path: str, config_file: str): "accuracy_config": accuracy_data, "only_run_accuracy": only_run_accuracy, } - # Which modes warm up, and why, is documented on - # WARMUP_BENCHMARK_MODES / wants_warmup above. - # - # Unlike gen_only there is no concurrency restriction here, because - # the GEN fill gate (TLLM_BENCHMARK_REQ_QUEUES_SIZE) is only injected - # for gen_only lanes, so a lone warmup request cannot stall behind it. - # - # Passed as an argument rather than folded into client_config_data - # above, so that no lane yaml can reach it -- b_warmup is not a - # baseline match key, and that is only sound while the value stays - # fully determined by benchmark_mode. client_config = ClientConfig( client_config_data, model_name, diff --git a/tests/unittest/tools/test_perf_sanity_matching.py b/tests/unittest/tools/test_perf_sanity_matching.py index d18c037dfeb5..76e8b680acb0 100644 --- a/tests/unittest/tools/test_perf_sanity_matching.py +++ b/tests/unittest/tools/test_perf_sanity_matching.py @@ -356,6 +356,45 @@ def test_warmup_is_suppressed_for_the_non_default_benchmark_clients() -> None: assert default.to_db_data()["b_warmup"] is True +def test_warmup_is_suppressed_by_the_same_condition_to_cmd_dispatches_on() -> None: + """Only the agentx value suppresses warmup -- not any non-empty string. + + Suppression exists because the agentx and nv_sa builders have no initial test + request to un-suppress. to_cmd selects them by `== AGENTX_BENCHMARK_CLIENT` + and `use_nv_sa_benchmark`, so warmup must be suppressed on exactly that + condition. Testing `not self.benchmark_client` (truthiness) instead looks + equivalent and is not: an unrecognised value is falsy-negative there, so + warmup gets suppressed while to_cmd still falls through to the default + builder -- a lane that asked to warm up, can warm up, and silently does not. + Unrecognised values do reach ClientConfig: only _parse_disagg_config_file + rejects them, and the aggregated parser passes its yaml dict through + unvalidated. + + Asserted on the emitted command, not on self.warmup: b_warmup and + --no-test-input are both driven by self.warmup, so comparing them to each + other is circular and holds under either condition. + """ + module = _load_module() + client_config = module.ClientConfig + + unrecognised = client_config( + {**_disagg_client_data(10), "benchmark_client": "some-future-client"}, + "example_model", + warmup=True, + ) + cmd = unrecognised.to_cmd() + + assert any("benchmark_serving" in arg for arg in cmd), ( + "an unrecognised benchmark_client no longer falls through to the default " + "builder; this test's premise needs rechecking" + ) + assert "--no-test-input" not in cmd, ( + "warmup was suppressed for a lane that runs the default benchmark_serving " + "client anyway -- suppression must test == AGENTX_BENCHMARK_CLIENT, not the " + "truthiness of benchmark_client" + ) + + def test_warmup_defaults_off_and_is_reported() -> None: """Every other lane keeps today's behaviour, and the DB records which warmed.""" client_config = _load_client_config() From 93ff91bc8f0b58f9015a6800d74cb1128be699f6 Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:20:29 -0700 Subject: [PATCH 5/5] [None][chore] drop the wants_warmup docstring Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- tests/integration/defs/perf/test_perf_sanity.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index fd54b950d1ae..2f51f75b4f79 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -63,7 +63,6 @@ def wants_warmup(benchmark_mode: str) -> bool: - """Whether a lane in this benchmark mode should issue a warmup request.""" return benchmark_mode in WARMUP_BENCHMARK_MODES