From cac7325bc134f6c53ad4c8bca8728012aa22a21c Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Sun, 30 Aug 2026 05:27:34 -0700 Subject: [PATCH 1/3] [None][test] key perf-sanity case identity on test case name A perf test case was identified by a wide tuple of its config values -- 27 fields for aggregated runs (s_gpu_type, s_runtime + 15 server + 10 client), up to 46 for disaggregated. Most of those fields are tunables: knobs like l_max_batch_size, s_kv_cache_dtype, s_spec_decoding_type, l_force_num_accepted_tokens, l_iterations and b_streaming are adjusted specifically to improve perf on the same test. Changing one forked the case into a brand-new identity with an empty baseline, so the tuning's effect -- the reason for the change -- became invisible, and the case had to accrue three fresh data points before it could be gated again. The remaining fields are fixed parameters (l_isl, l_osl, l_concurrency, s_model_name, l_gpus, l_tp/ep/pp/cp, l_gpus_per_node). Those never change for a given case and are already encoded in s_test_case_name, so keying on them adds nothing. Key on the test case name instead, plus the three things the name does not carry: s_gpu_type (the same case name runs on more than one GPU type), s_runtime (the same case name runs on both aggr_server and multi_node_aggr_server) and s_branch. Validated against the live 90-day index (11,418 documents): grouping by name yields zero groups in which any fixed parameter varies (0/123 aggr, 0/85 multi-node aggr, 0/110 disagg), so the coarsening never merges two different tests. It does merge 37 groups that the old key had split on a tunable -- every one of them on l_force_num_accepted_tokens. 35 of the 37 keep an unchanged regression band; 2 shift, which is the expected one-off transient as a rejoined case's rolling baseline re-settles. s_benchmark_mode is deliberately not a match key. It is null on every aggregated record, and on disaggregated records it exactly equals the test case name's prefix (3036/3036 documents), so it carries no information the name does not. Adding it would also break matching against records written before the field existed, since benchmark_data_matches treats None and "e2e" as different values. Pre-merge branch substitution: s_branch is now a match key, and history queries are restricted to b_is_post_merge documents. A pre-merge run records s_branch = "github-pr-", which has no post-merge history, so matching on it would return nothing and turn every pre-merge regression check into a silent no-op. process_and_upload_test_results therefore resolves a baseline branch (PERF_BASELINE_BRANCH, default "main") and substitutes it into a lookup-only copy of the data before get_common_values -- which must come first, since it feeds the OpenSearch must clause and would otherwise filter the history away before matching runs. The uploaded documents keep their true s_branch. Callers that do not key on s_branch (module perf, visual gen) are untouched. This completes the branch half of #18127, which fixed s_branch derivation but noted that s_branch was still absent from match_keys, leaving release-branch points joining main's baseline. Two pre-existing issues are resolved as side effects: - The gen_only filter dropping the ctx/gen transceiver backend from the key compared against "gen_s_cache_transceiver_backend" while the prefixing helper produces "s_gen_cache_transceiver_backend", so it never matched and was dead code. It is deleted along with the block it lived in. - world_size is read by jenkins/scripts/perf/submit.py to size the Slurm allocation but never parsed into ServerConfig, so a 1-GPU and a 4-GPU variant of the same config shared one baseline. Their names differ, so keying on the name separates them. world_size is still unparsed and would matter again if anything keyed on l_gpus. match_mode: "scenario" becomes inert -- not forking a case when knobs drift is now the default for every case. The option is still parsed so existing yamls stay valid; removing it from the ~30 configs that set it is left as a follow-up to keep this change to one concern. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- .../defs/perf/README_test_perf_sanity.md | 37 +++++++- .../defs/perf/perf_regression_utils.py | 27 ++++-- .../integration/defs/perf/test_perf_sanity.py | 40 +------- tests/test_common/perf_sanity_matching.py | 52 ++-------- .../tools/test_perf_sanity_matching.py | 95 +++++++++++-------- 5 files changed, 115 insertions(+), 136 deletions(-) diff --git a/tests/integration/defs/perf/README_test_perf_sanity.md b/tests/integration/defs/perf/README_test_perf_sanity.md index 144d24f6d795..01f0e907bb3d 100644 --- a/tests/integration/defs/perf/README_test_perf_sanity.md +++ b/tests/integration/defs/perf/README_test_perf_sanity.md @@ -63,10 +63,39 @@ If the mean cannot be parsed for a `gen_only` run, `check_test_failure` raises ` ### Match Keys -Match keys differ by deployment mode: - -- **Aggregated**: `["s_gpu_type", "s_runtime"]` + `ServerConfig.to_match_keys()` + `ClientConfig.to_match_keys()` -- **Disaggregated**: `["s_gpu_type", "s_runtime", "s_benchmark_mode", "l_num_ctx_servers", "l_num_gen_servers"]` + prefixed ctx/gen `ServerConfig.to_match_keys()` + `ClientConfig.to_match_keys()` +Match keys are the fields that identify a test case when looking up its history to +build a baseline. They are the same for every deployment mode +(`get_test_case_match_keys()` in `tests/test_common/perf_sanity_matching.py`): + +| Key | Why | +|-----|-----| +| `s_test_case_name` | `-` for aggregated, `--` for disaggregated. Already encodes every fixed parameter of the case: model, parallelism, ISL/OSL, concurrency. | +| `s_gpu_type` | The same case name runs on more than one GPU type. | +| `s_runtime` | The same case name runs on both `aggr_server` and `multi_node_aggr_server`. | +| `s_branch` | Release branches keep their own baseline rather than blending into `main`'s. | + +A case is identified by its **name**, not by the config values it happens to run +with today. Server/client config fields such as `l_max_batch_size`, +`s_kv_cache_dtype`, `s_spec_decoding_type`, `l_force_num_accepted_tokens`, +`l_iterations` or `b_streaming` are *tunables* — they are adjusted to improve perf +on the same test. Keying on them forked the case and reset its baseline, hiding the +very effect the tuning was meant to have. They are still recorded on each document +for filtering and analysis; they just do not participate in matching. + +`s_benchmark_mode` is deliberately excluded: it is null on every aggregated record +and exactly equals the test case name's prefix on every disaggregated one, so it +adds no information while breaking matching against records written before the +field existed (`benchmark_data_matches` treats `None` and `"e2e"` as different). + +`match_mode: scenario` in the server yamls is now inert — not forking a case on a +config change is the default for every case. + +**Pre-merge branch substitution.** Since `s_branch` is a match key and history +queries are restricted to post-merge documents, a pre-merge run (whose `s_branch` +is `github-pr-`) would match nothing and its regression check would silently +pass. `process_and_upload_test_results` therefore looks history up against a +baseline branch — `PERF_BASELINE_BRANCH`, default `main` — using a lookup-only copy +of the data. The uploaded documents keep their true `s_branch`. ## Overview diff --git a/tests/integration/defs/perf/perf_regression_utils.py b/tests/integration/defs/perf/perf_regression_utils.py index bf69d0fb0a13..3f5a4e98e54a 100644 --- a/tests/integration/defs/perf/perf_regression_utils.py +++ b/tests/integration/defs/perf/perf_regression_utils.py @@ -1,4 +1,4 @@ -# SPDX-FileCopyrightText: Copyright (c) 2022-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2022-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -494,15 +494,24 @@ def process_and_upload_test_results( data.update(extra_fields) add_id(data) - # Step 3: Find common values to narrow query scope - common_values_dict = get_common_values(new_data_dict, match_keys) + # Step 3: For pre-merge, look history up against the baseline branch + lookup_data_dict = new_data_dict + if not is_post_merge and "s_branch" in match_keys: + baseline_branch = os.environ.get("PERF_BASELINE_BRANCH", "main") + lookup_data_dict = { + cmd_idx: {**data, "s_branch": baseline_branch} + for cmd_idx, data in new_data_dict.items() + } - # Step 4: Query history data + # Step 4: Find common values to narrow query scope + common_values_dict = get_common_values(lookup_data_dict, match_keys) + + # Step 5: Query history data latest_history_data_dict, latest_baseline_threshold_dict, history_data_dict = get_history_data( - new_data_dict, match_keys, common_values_dict + lookup_data_dict, match_keys, common_values_dict ) - # Step 5: Compute regression info + # Step 6: Compute regression info prepare_regressive_test_cases( latest_history_data_dict, latest_baseline_threshold_dict, @@ -513,17 +522,17 @@ def process_and_upload_test_results( regression_metrics, ) - # Step 6: For post-merge, embed baseline fields + # Step 7: For post-merge, embed baseline fields if is_post_merge: add_baseline_fields_to_post_merge_data( latest_baseline_threshold_dict, new_data_dict, maximize_metrics, minimize_metrics ) - # Step 7: Upload to DB + # Step 8: Upload to DB if upload_to_db: post_new_perf_data(new_data_dict) - # Step 8: Check regression (auto-detect fail behavior if not specified) + # Step 9: Check regression (auto-detect fail behavior if not specified) if fail_on_regression is None: fail_on_regression = not is_post_merge check_perf_regression(new_data_dict, fail_on_regression=fail_on_regression) diff --git a/tests/integration/defs/perf/test_perf_sanity.py b/tests/integration/defs/perf/test_perf_sanity.py index 84e76a1aba95..6fe156d597ea 100644 --- a/tests/integration/defs/perf/test_perf_sanity.py +++ b/tests/integration/defs/perf/test_perf_sanity.py @@ -31,7 +31,7 @@ import yaml from test_common.error_utils import report_error from test_common.http_utils import fail_if_proc_died, wait_for_endpoint_ready -from test_common.perf_sanity_matching import get_client_match_keys, get_server_match_keys +from test_common.perf_sanity_matching import get_test_case_match_keys from defs.common import wait_for_reported_addr from defs.trt_test_alternative import print_info @@ -841,9 +841,6 @@ def to_cmd( def to_env(self) -> Dict[str, str]: return to_env_dict(self.env_vars) - def to_match_keys(self) -> List[str]: - return get_server_match_keys(self.match_mode) - def to_db_data(self) -> dict: """Convert ServerConfig to database data.""" db_data = { @@ -1220,9 +1217,6 @@ def _to_default_benchmark_cmd(self) -> List[str]: def to_env(self) -> Dict[str, str]: return to_env_dict(self.env_vars) - def to_match_keys(self) -> List[str]: - return get_client_match_keys() - def to_db_data(self) -> dict: """Convert ClientConfig to database data.""" # b_eos retained for baseline-matching continuity. spec-decoding runs are now @@ -2608,13 +2602,10 @@ def add_prefix(key: str, prefix_name: str) -> str: rest = key[2:] return f"{type_prefix}{prefix_name}_{rest}" - def add_list_prefix(config_list: List, prefix_name: str) -> List: - return [add_prefix(key, prefix_name) for key in config_list] - def add_dict_prefix(config_dict: dict, prefix_name: str) -> dict: return {add_prefix(key, prefix_name): value for key, value in config_dict.items()} - match_keys = [] + match_keys = get_test_case_match_keys() if self.runtime == "aggr_server": new_data_dict = {} @@ -2659,11 +2650,6 @@ def add_dict_prefix(config_dict: dict, prefix_name: str) -> dict: new_data_dict[cmd_idx] = new_data cmd_idx += 1 - if not match_keys: - match_keys.extend(["s_gpu_type", "s_runtime"]) - match_keys.extend(server_config.to_match_keys()) - match_keys.extend(client_config.to_match_keys()) - elif self.runtime == "multi_node_disagg_server": # Only BENCHMARK node uploads if self.server_configs[0][2].disagg_serving_type != "BENCHMARK": @@ -2725,28 +2711,6 @@ def add_dict_prefix(config_dict: dict, prefix_name: str) -> dict: new_data_dict[cmd_idx] = new_data cmd_idx += 1 - if not match_keys: - match_keys.extend( - [ - "s_gpu_type", - "s_runtime", - "s_benchmark_mode", - "l_num_ctx_servers", - "l_num_gen_servers", - ] - ) - if num_ctx_servers > 0: - match_keys.extend(add_list_prefix(ctx_config.to_match_keys(), "ctx")) - if num_gen_servers > 0: - gen_match_keys = add_list_prefix(gen_config.to_match_keys(), "gen") - if disagg_config.benchmark_mode == "gen_only": - gen_match_keys = [ - k - for k in gen_match_keys - if k != "gen_s_cache_transceiver_backend" - ] - match_keys.extend(gen_match_keys) - match_keys.extend(client_config.to_match_keys()) else: return diff --git a/tests/test_common/perf_sanity_matching.py b/tests/test_common/perf_sanity_matching.py index 15de45c6cd76..0d2eb34fd730 100644 --- a/tests/test_common/perf_sanity_matching.py +++ b/tests/test_common/perf_sanity_matching.py @@ -17,53 +17,17 @@ from collections.abc import Iterable, Mapping -_SCENARIO_SERVER_MATCH_KEYS = ( - "s_model_name", - "l_gpus", +_TEST_CASE_MATCH_KEYS = ( + "s_test_case_name", + "s_gpu_type", + "s_runtime", + "s_branch", ) -_CONFIG_SERVER_MATCH_KEYS = ( - "s_model_name", - "l_tp", - "l_ep", - "l_pp", - "l_cp", - "l_gpus_per_node", - "l_max_batch_size", - "b_enable_attention_dp", - "s_serving_backend", - "s_kv_cache_dtype", - "s_cache_transceiver_backend", - "s_spec_decoding_type", - "l_num_nextn_predict_layers", - "l_force_num_accepted_tokens", - "l_load_balancer_num_slots", -) - -_CLIENT_MATCH_KEYS = ( - "l_concurrency", - "l_iterations", - "l_isl", - "l_osl", - "d_random_range_ratio", - "s_backend", - "b_use_chat_template", - "b_streaming", - "b_use_nv_sa_benchmark", - "b_eos", -) - - -def get_server_match_keys(match_mode: str) -> list[str]: - """Return database fields that identify an equivalent server run.""" - if match_mode == "scenario": - return list(_SCENARIO_SERVER_MATCH_KEYS) - return list(_CONFIG_SERVER_MATCH_KEYS) - -def get_client_match_keys() -> list[str]: - """Return database fields that identify an equivalent client workload.""" - return list(_CLIENT_MATCH_KEYS) +def get_test_case_match_keys() -> list[str]: + """Return database fields that identify an equivalent perf test case.""" + return list(_TEST_CASE_MATCH_KEYS) def benchmark_data_matches( diff --git a/tests/unittest/tools/test_perf_sanity_matching.py b/tests/unittest/tools/test_perf_sanity_matching.py index a6e9ab39d42f..18f139492bf3 100644 --- a/tests/unittest/tools/test_perf_sanity_matching.py +++ b/tests/unittest/tools/test_perf_sanity_matching.py @@ -13,17 +13,15 @@ # See the License for the specific language governing permissions and # limitations under the License. -from test_common.perf_sanity_matching import ( - benchmark_data_matches, - get_client_match_keys, - get_server_match_keys, -) +from test_common.perf_sanity_matching import benchmark_data_matches, get_test_case_match_keys def _benchmark_data(**overrides: object) -> dict[str, object]: data: dict[str, object] = { + "s_test_case_name": "example_model_fp8_tp8-con32_iter10_1k1k", "s_gpu_type": "b200", "s_runtime": "aggr_server", + "s_branch": "main", "s_model_name": "example_model", "l_gpus": 8, "l_tp": 8, @@ -39,63 +37,78 @@ def _benchmark_data(**overrides: object) -> dict[str, object]: return data -def _benchmark_match_keys(match_mode: str) -> list[str]: - return [ +def test_match_keys_are_name_and_environment_only() -> None: + assert get_test_case_match_keys() == [ + "s_test_case_name", "s_gpu_type", "s_runtime", - *get_server_match_keys(match_mode), - *get_client_match_keys(), + "s_branch", ] -def test_scenario_matching_ignores_recipe_tuning_changes() -> None: +def test_matching_ignores_tuning_changes() -> None: previous_data = _benchmark_data() updated_data = _benchmark_data( - l_tp=4, - l_ep=2, l_max_batch_size=64, s_kv_cache_dtype="auto", + l_iterations=20, + l_force_num_accepted_tokens=3, ) - match_keys = _benchmark_match_keys("scenario") - assert get_server_match_keys("scenario") == ["s_model_name", "l_gpus"] - assert {"l_concurrency", "l_isl", "l_osl"}.issubset(match_keys) - assert benchmark_data_matches(previous_data, updated_data, match_keys) + assert benchmark_data_matches(previous_data, updated_data, get_test_case_match_keys()) -def test_config_matching_keeps_recipe_tuning_keys() -> None: +def test_matching_distinguishes_test_case_name() -> None: previous_data = _benchmark_data() - updated_data = _benchmark_data( - l_tp=4, - l_ep=2, - l_max_batch_size=64, - s_kv_cache_dtype="auto", - ) + updated_data = _benchmark_data(s_test_case_name="example_model_fp8_tp4-con32_iter10_1k1k") - assert not benchmark_data_matches( - previous_data, - updated_data, - _benchmark_match_keys("config"), - ) + assert not benchmark_data_matches(previous_data, updated_data, get_test_case_match_keys()) -def test_scenario_matching_distinguishes_gpu_count() -> None: +def test_matching_distinguishes_gpu_type() -> None: previous_data = _benchmark_data() - updated_data = _benchmark_data(l_gpus=4) + updated_data = _benchmark_data(s_gpu_type="gb200") - assert not benchmark_data_matches( - previous_data, - updated_data, - _benchmark_match_keys("scenario"), - ) + assert not benchmark_data_matches(previous_data, updated_data, get_test_case_match_keys()) + + +def test_matching_distinguishes_runtime() -> None: + previous_data = _benchmark_data() + updated_data = _benchmark_data(s_runtime="multi_node_aggr_server") + assert not benchmark_data_matches(previous_data, updated_data, get_test_case_match_keys()) -def test_scenario_matching_distinguishes_client_workload() -> None: + +def test_matching_distinguishes_branch() -> None: previous_data = _benchmark_data() - updated_data = _benchmark_data(l_concurrency=64) + updated_data = _benchmark_data(s_branch="release/1.3.0") + + assert not benchmark_data_matches(previous_data, updated_data, get_test_case_match_keys()) + - assert not benchmark_data_matches( - previous_data, - updated_data, - _benchmark_match_keys("scenario"), +def test_benchmark_mode_is_not_a_match_key() -> None: + assert "s_benchmark_mode" not in get_test_case_match_keys() + history = _benchmark_data( + s_test_case_name="e2e-example_disagg-con32_iter10_1k1k", + s_runtime="multi_node_disagg_server", + s_benchmark_mode=None, ) + new = _benchmark_data( + s_test_case_name="e2e-example_disagg-con32_iter10_1k1k", + s_runtime="multi_node_disagg_server", + s_benchmark_mode="e2e", + ) + + assert benchmark_data_matches(history, new, get_test_case_match_keys()) + + +def test_pre_merge_branch_substitution_finds_baseline() -> None: + history = _benchmark_data(s_branch="main") + pre_merge_data = _benchmark_data(s_branch="github-pr-12345") + match_keys = get_test_case_match_keys() + + assert not benchmark_data_matches(history, pre_merge_data, match_keys) + + lookup_data = {**pre_merge_data, "s_branch": "main"} + assert benchmark_data_matches(history, lookup_data, match_keys) + assert pre_merge_data["s_branch"] == "github-pr-12345" From b6d8836533b5ed176490c2e05086484d07fba7b3 Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Sun, 30 Aug 2026 19:55:13 -0700 Subject: [PATCH 2/3] test: cover the pre-merge baseline-branch routing directly The pre-merge branch substitution in process_and_upload_test_results had no test that exercised the function. The case in test_perf_sanity_matching.py named test_pre_merge_branch_substitution_finds_baseline re-implemented the substitution by hand -- it built {**pre_merge_data, "s_branch": "main"} and asserted benchmark_data_matches -- so it passed no matter what the production routing did. That gap matters because the failure is silent and green: if the queries stop seeing the baseline branch, a pre-merge run matches no history (get_history_data only returns post-merge records) and every pre-merge regression check becomes a no-op. Add six cases to tests/unittest/others/test_perf_regression_branch.py, which already loads perf_regression_utils with the integration packages stubbed. They call process_and_upload_test_results with only the three OpenSearch seams replaced -- get_common_values, get_history_data, post_new_perf_data -- and assert the first two observe the baseline branch while the uploaded document keeps its real github-pr- branch. get_common_values is checked explicitly because it folds single-valued match keys into the query's must-clause, so substituting only for get_history_data would still filter the history away. Also covered: PERF_BASELINE_BRANCH, no substitution for post-merge, and no substitution when s_branch is not a match key. Verified by mutation: removing the routing, substituting for only one of the two queries, mutating in place instead of copying, ignoring the env override, applying the substitution to post-merge, and dropping the match_keys guard each fail at least one case. Rename the hand-rolled case to describe what it actually asserts -- that a PR branch does not match main's history, the precondition making the substitution necessary -- and point it at the new tests. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- .../others/test_perf_regression_branch.py | 162 ++++++++++++++++-- .../tools/test_perf_sanity_matching.py | 16 +- 2 files changed, 161 insertions(+), 17 deletions(-) diff --git a/tests/unittest/others/test_perf_regression_branch.py b/tests/unittest/others/test_perf_regression_branch.py index ffd7f3dae6e0..ee36221f5378 100644 --- a/tests/unittest/others/test_perf_regression_branch.py +++ b/tests/unittest/others/test_perf_regression_branch.py @@ -12,12 +12,22 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Tests that s_branch is read from globalVars, never scraped from the job URL. - -The Jenkins folder segment in a job URL (/job/LLM/job//) names the -folder the job definition lives in, which is "main" even for a release-branch -post-merge build. Recovering a branch from it therefore yields the constant -"main"; the pipeline publishes the real branch in globalVars["build_branch"]. +"""Tests for how perf_regression_utils handles s_branch. + +Two concerns, both of which fail open (a green run with no regression check) if +they break: + +1. s_branch is read from globalVars, never scraped from the job URL. The Jenkins + folder segment in a job URL (/job/LLM/job//) names the folder the job + definition lives in, which is "main" even for a release-branch post-merge + build. Recovering a branch from it therefore yields the constant "main"; the + pipeline publishes the real branch in globalVars["build_branch"]. + +2. A pre-merge run looks its history up against a baseline branch. s_branch is + part of the case identity, and get_history_data only ever returns post-merge + records, so a pre-merge run querying its own "github-pr-" branch matches + nothing and its regression check silently becomes a no-op. The queries must + see the baseline branch while the uploaded document keeps the real one. """ import importlib.util @@ -36,8 +46,10 @@ def _load_perf_regression_utils(): """Load the real module without importing the integration-test packages. ``defs/__init__.py`` pre-imports ``torch._inductor`` and - ``open_search_db_utils`` pulls in the OpenSearch client. ``get_job_info`` - reaches neither, and requiring them would turn this into a GPU-image test. + ``open_search_db_utils`` pulls in the OpenSearch client. Neither + ``get_job_info`` nor the branch routing in + ``process_and_upload_test_results`` reaches them, and requiring them would + turn this into a GPU-image test. """ defs_pkg = types.ModuleType("defs") defs_pkg.__path__ = [] @@ -73,7 +85,8 @@ def _load_perf_regression_utils(): return module -get_job_info = _load_perf_regression_utils().get_job_info +_perf_regression_utils = _load_perf_regression_utils() +get_job_info = _perf_regression_utils.get_job_info # A real post-merge build of release/1.3.0rc22.post1, whose job URL still says # ".../job/LLM/job/main/job/L0_PostMerge/...". @@ -82,7 +95,14 @@ def _load_perf_regression_utils(): _GITHUB_PR_JOB_URL = f"{_JENKINS}/job/LLM/job/main/job/L0_MergeRequest_PR/1234/" _GITLAB_MR_JOB_URL = f"{_JENKINS}/job/LLM/job/main/job/L0_MergeRequest/77/" -_INHERITED_CI_VARS = ("globalVars", "gitlabCommit", "BUILD_ID", "BUILD_URL", "JOB_NAME") +_INHERITED_CI_VARS = ( + "globalVars", + "gitlabCommit", + "BUILD_ID", + "BUILD_URL", + "JOB_NAME", + "PERF_BASELINE_BRANCH", +) @pytest.fixture(autouse=True) @@ -176,3 +196,125 @@ def test_unparsable_global_vars_does_not_raise(monkeypatch): info = get_job_info() assert info["s_branch"] == "" assert info["s_job_url"] == "" + + +# --------------------------------------------------------------------------- # +# Pre-merge history lookup routes through a baseline branch +# --------------------------------------------------------------------------- # + +_MATCH_KEYS = ["s_test_case_name", "s_gpu_type", "s_runtime", "s_branch"] + + +def _new_data_dict(): + return { + 0: { + "s_test_case_name": "example_model_fp8_tp8-con32_iter10_1k1k", + "s_gpu_type": "b200", + "s_runtime": "aggr_server", + "d_output_token_throughput": 1234.5, + } + } + + +def _run_pipeline(monkeypatch, build_branch, job_url, match_keys=None): + """Run the real pipeline, recording the s_branch each seam observes. + + Only the three OpenSearch seams are replaced. Everything between them -- + get_job_info, the enrichment loop, the branch routing, the regression pass -- + is the production code path, so a regression in the wiring shows up here. + """ + observed = {} + + def fake_get_common_values(data_dict, keys): + observed["common_values"] = {idx: d["s_branch"] for idx, d in data_dict.items()} + return {} + + def fake_get_history_data(data_dict, keys, common_values_dict): + observed["history_query"] = {idx: d["s_branch"] for idx, d in data_dict.items()} + observed["history_query_names"] = { + idx: d["s_test_case_name"] for idx, d in data_dict.items() + } + return {}, {}, {} + + def fake_post_new_perf_data(data_dict): + observed["uploaded"] = {idx: d["s_branch"] for idx, d in data_dict.items()} + + monkeypatch.setattr(_perf_regression_utils, "get_common_values", fake_get_common_values) + monkeypatch.setattr(_perf_regression_utils, "get_history_data", fake_get_history_data) + monkeypatch.setattr(_perf_regression_utils, "post_new_perf_data", fake_post_new_perf_data) + + payload = { + "build_branch": build_branch, + "action_info": {"parents": [{"url": job_url, "build_number": "7"}]}, + } + monkeypatch.setenv("globalVars", json.dumps(payload)) + + new_data_dict = _new_data_dict() + _perf_regression_utils.process_and_upload_test_results( + new_data_dict, + match_keys if match_keys is not None else _MATCH_KEYS, + maximize_metrics=["d_output_token_throughput"], + minimize_metrics=[], + regression_metrics=["d_output_token_throughput"], + ) + observed["new_data_dict"] = {idx: d["s_branch"] for idx, d in new_data_dict.items()} + return observed + + +def test_pre_merge_history_is_queried_against_the_baseline_branch(monkeypatch): + """The queries see "main"; the uploaded document keeps "github-pr-". + + get_common_values must see the substituted branch too: it folds + single-valued match keys into the OpenSearch must-clause, so substituting + only for get_history_data would let the query filter the history away. + """ + observed = _run_pipeline(monkeypatch, "github-pr-18408", _GITHUB_PR_JOB_URL) + + assert observed["common_values"] == {0: "main"} + assert observed["history_query"] == {0: "main"} + assert observed["uploaded"] == {0: "github-pr-18408"} + assert observed["new_data_dict"] == {0: "github-pr-18408"} + + +def test_pre_merge_baseline_branch_is_configurable(monkeypatch): + monkeypatch.setenv("PERF_BASELINE_BRANCH", "release/1.3.0rc22.post1") + observed = _run_pipeline(monkeypatch, "github-pr-18408", _GITHUB_PR_JOB_URL) + + assert observed["history_query"] == {0: "release/1.3.0rc22.post1"} + assert observed["uploaded"] == {0: "github-pr-18408"} + + +def test_substitution_only_replaces_the_branch(monkeypatch): + """The lookup copy is the real data with one field changed, not a stub.""" + observed = _run_pipeline(monkeypatch, "github-pr-18408", _GITHUB_PR_JOB_URL) + + assert observed["history_query_names"] == {0: "example_model_fp8_tp8-con32_iter10_1k1k"} + + +def test_post_merge_history_is_queried_against_its_own_branch(monkeypatch): + """A release-branch post-merge run must not be rebased onto main.""" + observed = _run_pipeline(monkeypatch, "release/1.3.0rc22.post1", _RELEASE_BUILD_JOB_URL) + + assert observed["common_values"] == {0: "release/1.3.0rc22.post1"} + assert observed["history_query"] == {0: "release/1.3.0rc22.post1"} + assert observed["uploaded"] == {0: "release/1.3.0rc22.post1"} + + +def test_post_merge_ignores_the_baseline_branch_override(monkeypatch): + monkeypatch.setenv("PERF_BASELINE_BRANCH", "main") + observed = _run_pipeline(monkeypatch, "release/1.3.0rc22.post1", _RELEASE_BUILD_JOB_URL) + + assert observed["history_query"] == {0: "release/1.3.0rc22.post1"} + + +def test_no_substitution_when_branch_is_not_a_match_key(monkeypatch): + """Callers keying on other fields keep their own semantics untouched.""" + observed = _run_pipeline( + monkeypatch, + "github-pr-18408", + _GITHUB_PR_JOB_URL, + match_keys=["s_test_case_name", "s_gpu_type"], + ) + + assert observed["history_query"] == {0: "github-pr-18408"} + assert observed["uploaded"] == {0: "github-pr-18408"} diff --git a/tests/unittest/tools/test_perf_sanity_matching.py b/tests/unittest/tools/test_perf_sanity_matching.py index 18f139492bf3..a28e6f7fdd1f 100644 --- a/tests/unittest/tools/test_perf_sanity_matching.py +++ b/tests/unittest/tools/test_perf_sanity_matching.py @@ -102,13 +102,15 @@ def test_benchmark_mode_is_not_a_match_key() -> None: assert benchmark_data_matches(history, new, get_test_case_match_keys()) -def test_pre_merge_branch_substitution_finds_baseline() -> None: +def test_a_pre_merge_branch_does_not_match_post_merge_history() -> None: + """Branch is identity, so a PR run cannot match main's history unaided. + + This is the precondition that makes the baseline-branch substitution in + process_and_upload_test_results necessary; the substitution itself is tested + against that function in + tests/unittest/others/test_perf_regression_branch.py. + """ history = _benchmark_data(s_branch="main") pre_merge_data = _benchmark_data(s_branch="github-pr-12345") - match_keys = get_test_case_match_keys() - assert not benchmark_data_matches(history, pre_merge_data, match_keys) - - lookup_data = {**pre_merge_data, "s_branch": "main"} - assert benchmark_data_matches(history, lookup_data, match_keys) - assert pre_merge_data["s_branch"] == "github-pr-12345" + assert not benchmark_data_matches(history, pre_merge_data, get_test_case_match_keys()) From 978c212e6f1bb1a6e8b0ef6424949971d027cb8b Mon Sep 17 00:00:00 2001 From: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> Date: Sun, 30 Aug 2026 20:27:54 -0700 Subject: [PATCH 3/3] test: pin the case-name boundary for l_iterations, annotate new tests The README listed l_iterations among the tunables that no longer fork a case. That is only true where the client name is pinned by the yaml. On the disagg path it is wrong: benchmark.multi_round becomes the client's "iterations" (test_perf_sanity.py:2270), ClientConfig derives con_iter_isl_osl when no name is given (:1119), and that name is the client half of s_test_case_name -- which is a match key. So changing multi_round renames the case, and a renamed case has no history and no pre-merge regression check. Measured: all 190 client blocks under tests/scripts/perf-sanity/ set an explicit name, so aggregated configs keep their history across an iterations change (93 of those names carry a now-stale iter as a result); disagg configs set none, so every disagg case takes the derived name. This is by design rather than a gap to close -- iterations sets how long the measurement runs, so iter10 and iter12 do not measure the same quantity and should not share a baseline -- and it is not a regression, since l_iterations was itself a match key before this change. Reworking name generation would instead rename every disagg case and discard the history this PR exists to preserve. So: correct the README to state the actual boundary, and pin it with tests against the real ClientConfig rather than a re-implemented f-string. Since PerfSanityTestConfig's constructor shells out to nvidia-smi and cannot run on a CPU node, the multi_round -> iterations link is asserted over the parser's AST. Verified by mutation: dropping iterations from the derived name, restoring l_iterations to the match key, and unwiring multi_round each fail a test. Also annotate the new functions in test_perf_regression_branch.py, matching the style of the sibling matching tests. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com> --- .../defs/perf/README_test_perf_sanity.md | 30 +++- .../others/test_perf_regression_branch.py | 39 +++-- .../tools/test_perf_sanity_matching.py | 165 ++++++++++++++++++ 3 files changed, 217 insertions(+), 17 deletions(-) diff --git a/tests/integration/defs/perf/README_test_perf_sanity.md b/tests/integration/defs/perf/README_test_perf_sanity.md index 01f0e907bb3d..25c07594452c 100644 --- a/tests/integration/defs/perf/README_test_perf_sanity.md +++ b/tests/integration/defs/perf/README_test_perf_sanity.md @@ -76,11 +76,31 @@ build a baseline. They are the same for every deployment mode A case is identified by its **name**, not by the config values it happens to run with today. Server/client config fields such as `l_max_batch_size`, -`s_kv_cache_dtype`, `s_spec_decoding_type`, `l_force_num_accepted_tokens`, -`l_iterations` or `b_streaming` are *tunables* — they are adjusted to improve perf -on the same test. Keying on them forked the case and reset its baseline, hiding the -very effect the tuning was meant to have. They are still recorded on each document -for filtering and analysis; they just do not participate in matching. +`s_kv_cache_dtype`, `s_spec_decoding_type`, `l_force_num_accepted_tokens` or +`b_streaming` are *tunables* — they are adjusted to improve perf on the same test. +Keying on them forked the case and reset its baseline, hiding the very effect the +tuning was meant to have. They are still recorded on each document for filtering +and analysis; they just do not participate in matching. + +**A value that feeds the case name still forks the case.** Dropping a field from +the match key only stops it forking if the name does not encode it, and +`l_iterations` is the one field where that distinction bites: + +- Aggregated configs all set an explicit client `name:` (190 of 190 blocks in + `tests/scripts/perf-sanity/`), so changing `iterations` there keeps the case and + its history. 93 of those names contain a stale `iter` for that reason. +- Disagg configs set no client name, so `ClientConfig` derives + `con_iter_isl_osl`. `benchmark.multi_round` becomes `iterations`, + which lands in that name and therefore in `s_test_case_name` — so changing + `multi_round` renames the case, and the renamed case starts with no history. + +That is intended rather than a gap to close: `iterations` sets how long the +measurement runs, and a longer run amortizes warmup differently, so `iter10` and +`iter12` do not measure the same quantity and should not share a baseline. It is +also not a regression — `l_iterations` was previously a match key in its own +right, so it forked the case before this change too. What matters is that renaming +a case silently costs it its history and its next pre-merge regression check, so +`multi_round` should be treated as a workload parameter, not a knob to sweep. `s_benchmark_mode` is deliberately excluded: it is null on every aggregated record and exactly equals the test case name's prefix on every disaggregated one, so it diff --git a/tests/unittest/others/test_perf_regression_branch.py b/tests/unittest/others/test_perf_regression_branch.py index ee36221f5378..e7e89e1adc63 100644 --- a/tests/unittest/others/test_perf_regression_branch.py +++ b/tests/unittest/others/test_perf_regression_branch.py @@ -205,7 +205,7 @@ def test_unparsable_global_vars_does_not_raise(monkeypatch): _MATCH_KEYS = ["s_test_case_name", "s_gpu_type", "s_runtime", "s_branch"] -def _new_data_dict(): +def _new_data_dict() -> dict[int, dict[str, object]]: return { 0: { "s_test_case_name": "example_model_fp8_tp8-con32_iter10_1k1k", @@ -216,27 +216,38 @@ def _new_data_dict(): } -def _run_pipeline(monkeypatch, build_branch, job_url, match_keys=None): +def _run_pipeline( + monkeypatch: pytest.MonkeyPatch, + build_branch: str, + job_url: str, + match_keys: list[str] | None = None, +) -> dict[str, dict[int, str]]: """Run the real pipeline, recording the s_branch each seam observes. Only the three OpenSearch seams are replaced. Everything between them -- get_job_info, the enrichment loop, the branch routing, the regression pass -- is the production code path, so a regression in the wiring shows up here. """ - observed = {} + observed: dict[str, dict[int, str]] = {} - def fake_get_common_values(data_dict, keys): + def fake_get_common_values( + data_dict: dict[int, dict[str, object]], keys: list[str] + ) -> dict[str, object]: observed["common_values"] = {idx: d["s_branch"] for idx, d in data_dict.items()} return {} - def fake_get_history_data(data_dict, keys, common_values_dict): + def fake_get_history_data( + data_dict: dict[int, dict[str, object]], + keys: list[str], + common_values_dict: dict[str, object], + ) -> tuple[dict[int, object], dict[int, object], dict[int, object]]: observed["history_query"] = {idx: d["s_branch"] for idx, d in data_dict.items()} observed["history_query_names"] = { idx: d["s_test_case_name"] for idx, d in data_dict.items() } return {}, {}, {} - def fake_post_new_perf_data(data_dict): + def fake_post_new_perf_data(data_dict: dict[int, dict[str, object]]) -> None: observed["uploaded"] = {idx: d["s_branch"] for idx, d in data_dict.items()} monkeypatch.setattr(_perf_regression_utils, "get_common_values", fake_get_common_values) @@ -261,7 +272,9 @@ def fake_post_new_perf_data(data_dict): return observed -def test_pre_merge_history_is_queried_against_the_baseline_branch(monkeypatch): +def test_pre_merge_history_is_queried_against_the_baseline_branch( + monkeypatch: pytest.MonkeyPatch, +) -> None: """The queries see "main"; the uploaded document keeps "github-pr-". get_common_values must see the substituted branch too: it folds @@ -276,7 +289,7 @@ def test_pre_merge_history_is_queried_against_the_baseline_branch(monkeypatch): assert observed["new_data_dict"] == {0: "github-pr-18408"} -def test_pre_merge_baseline_branch_is_configurable(monkeypatch): +def test_pre_merge_baseline_branch_is_configurable(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("PERF_BASELINE_BRANCH", "release/1.3.0rc22.post1") observed = _run_pipeline(monkeypatch, "github-pr-18408", _GITHUB_PR_JOB_URL) @@ -284,14 +297,16 @@ def test_pre_merge_baseline_branch_is_configurable(monkeypatch): assert observed["uploaded"] == {0: "github-pr-18408"} -def test_substitution_only_replaces_the_branch(monkeypatch): +def test_substitution_only_replaces_the_branch(monkeypatch: pytest.MonkeyPatch) -> None: """The lookup copy is the real data with one field changed, not a stub.""" observed = _run_pipeline(monkeypatch, "github-pr-18408", _GITHUB_PR_JOB_URL) assert observed["history_query_names"] == {0: "example_model_fp8_tp8-con32_iter10_1k1k"} -def test_post_merge_history_is_queried_against_its_own_branch(monkeypatch): +def test_post_merge_history_is_queried_against_its_own_branch( + monkeypatch: pytest.MonkeyPatch, +) -> None: """A release-branch post-merge run must not be rebased onto main.""" observed = _run_pipeline(monkeypatch, "release/1.3.0rc22.post1", _RELEASE_BUILD_JOB_URL) @@ -300,14 +315,14 @@ def test_post_merge_history_is_queried_against_its_own_branch(monkeypatch): assert observed["uploaded"] == {0: "release/1.3.0rc22.post1"} -def test_post_merge_ignores_the_baseline_branch_override(monkeypatch): +def test_post_merge_ignores_the_baseline_branch_override(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("PERF_BASELINE_BRANCH", "main") observed = _run_pipeline(monkeypatch, "release/1.3.0rc22.post1", _RELEASE_BUILD_JOB_URL) assert observed["history_query"] == {0: "release/1.3.0rc22.post1"} -def test_no_substitution_when_branch_is_not_a_match_key(monkeypatch): +def test_no_substitution_when_branch_is_not_a_match_key(monkeypatch: pytest.MonkeyPatch) -> None: """Callers keying on other fields keep their own semantics untouched.""" observed = _run_pipeline( monkeypatch, diff --git a/tests/unittest/tools/test_perf_sanity_matching.py b/tests/unittest/tools/test_perf_sanity_matching.py index a28e6f7fdd1f..84904cd225e2 100644 --- a/tests/unittest/tools/test_perf_sanity_matching.py +++ b/tests/unittest/tools/test_perf_sanity_matching.py @@ -13,6 +13,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +import ast +import importlib.util +import pathlib +import sys +import types + from test_common.perf_sanity_matching import benchmark_data_matches, get_test_case_match_keys @@ -47,6 +53,11 @@ def test_match_keys_are_name_and_environment_only() -> None: def test_matching_ignores_tuning_changes() -> None: + """Tunables do not fork a case *while the name is held constant*. + + That proviso is the whole story for l_iterations, which reaches the case name + on the disagg path -- see test_iterations_fork_a_case_through_the_derived_name. + """ previous_data = _benchmark_data() updated_data = _benchmark_data( l_max_batch_size=64, @@ -58,6 +69,160 @@ 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. + + 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. + """ + repo_root = pathlib.Path(__file__).resolve().parents[3] + module_path = repo_root / "tests" / "integration" / "defs" / "perf" / "test_perf_sanity.py" + + def stub(name: str, **attrs: object) -> types.ModuleType: + module = types.ModuleType(name) + for key, value in attrs.items(): + setattr(module, key, value) + return module + + def noop(*args: object, **kwargs: object) -> None: + return None + + defs_pkg = stub("defs") + defs_pkg.__path__ = [] + perf_pkg = stub("defs.perf") + perf_pkg.__path__ = [] + stubs = { + "defs": defs_pkg, + "defs.perf": perf_pkg, + "defs.common": stub("defs.common", wait_for_reported_addr=noop), + "defs.trt_test_alternative": stub( + "defs.trt_test_alternative", print_info=noop, print_warning=noop + ), + "defs.conftest": stub( + "defs.conftest", + get_llm_root=lambda *a, **k: "/repo", + llm_models_root=lambda *a, **k: "/models", + ), + "defs.perf._model_paths": stub("defs.perf._model_paths", MODEL_PATH_DICT={}), + "defs.perf.open_search_db_utils": stub( + "defs.perf.open_search_db_utils", + add_id=noop, + get_history_data=noop, + post_new_perf_data=noop, + ), + "defs.perf.perf_regression_utils": stub( + "defs.perf.perf_regression_utils", + process_and_upload_test_results=noop, + get_job_info=noop, + _percentile=noop, + ), + } + + saved = {name: sys.modules.get(name) for name in stubs} + sys.modules.update(stubs) + try: + spec = importlib.util.spec_from_file_location("defs.perf.test_perf_sanity", module_path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + finally: + for name, previous in saved.items(): + if previous is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = previous + return module.ClientConfig + + +def _disagg_client_data(multi_round: int) -> dict[str, object]: + """The client dict a disagg config builds (test_perf_sanity.py:2266-2277). + + Notably it carries no "name", so ClientConfig derives one. + """ + return { + "concurrency": 12, + "iterations": multi_round, + "isl": 50000, + "osl": 2048, + } + + +def test_disagg_iterations_come_from_multi_round() -> None: + """Pin the yaml key that feeds "iterations" on the disagg path. + + _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 instead of re-stated in a docstring. + Without this, renaming the yaml key would leave the tests below green while + the documented behaviour silently changed. + """ + 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()) + + iterations_values = [ + value + for node in ast.walk(tree) + if isinstance(node, ast.Dict) + for key, value in zip(node.keys, node.values) + if isinstance(key, ast.Constant) and key.value == "iterations" + ] + + assert iterations_values, 'no dict literal builds an "iterations" entry' + assert any("multi_round" in ast.dump(value) for value in iterations_values), ( + '"iterations" is no longer derived from benchmark.multi_round; ' + "update README_test_perf_sanity.md and the tests below" + ) + + +def test_iterations_fork_a_case_through_the_derived_name() -> None: + """benchmark.multi_round renames a disagg case, so it still forks history. + + multi_round becomes the client's "iterations" (test_perf_sanity.py:2270), which + lands in the derived name (:1119) and hence in s_test_case_name. Dropping + l_iterations from the match key therefore does not make it fork-free on this + path. Intended -- iterations sets the measurement length, so iter10 and iter12 + do not measure the same quantity -- but it costs the renamed case its history, + so it is pinned here rather than left as an assumption. + """ + client_config = _load_client_config() + + ten = client_config(_disagg_client_data(10), "example_model") + twelve = client_config(_disagg_client_data(12), "example_model") + + assert ten.name == "con12_iter10_isl50000_osl2048" + assert twelve.name == "con12_iter12_isl50000_osl2048" + + stem = "e2e-gb300_example_model_50k2k_con12_ctx1_dep4_gen6_tep4-" + assert not benchmark_data_matches( + _benchmark_data(s_test_case_name=stem + ten.name, l_iterations=10), + _benchmark_data(s_test_case_name=stem + twelve.name, l_iterations=12), + get_test_case_match_keys(), + ) + + +def test_an_explicit_client_name_keeps_the_case_across_an_iterations_change() -> None: + """Aggregated configs all name their clients, so tuning keeps the history. + + This is the path where removing l_iterations from the key pays off: the name is + pinned by the yaml, so a changed iterations count stays on one curve. + """ + client_config = _load_client_config() + + ten = client_config({**_disagg_client_data(10), "name": "con1024_iter10_1k1k"}, "example_model") + twenty = client_config( + {**_disagg_client_data(20), "name": "con1024_iter10_1k1k"}, "example_model" + ) + + assert ten.name == twenty.name == "con1024_iter10_1k1k" + assert benchmark_data_matches( + _benchmark_data(s_test_case_name="example_model_fp8_tp8-" + ten.name, l_iterations=10), + _benchmark_data(s_test_case_name="example_model_fp8_tp8-" + twenty.name, l_iterations=20), + get_test_case_match_keys(), + ) + + def test_matching_distinguishes_test_case_name() -> None: previous_data = _benchmark_data() updated_data = _benchmark_data(s_test_case_name="example_model_fp8_tp4-con32_iter10_1k1k")