From 46353f84de670348afeb72c28b2122f34f733ae5 Mon Sep 17 00:00:00 2001 From: Cam Quilici Date: Fri, 14 Aug 2026 15:51:30 -0500 Subject: [PATCH 1/4] feat(ci): add append-only curve sweeps --- .github/codeowner-signoff-verify-prompt.md | 26 +- .github/workflows/claude-pr-review.yml | 9 + .github/workflows/claude.yml | 2 + CONTRIBUTING.md | 24 ++ docs/PR_REVIEW_CHECKLIST.md | 2 +- utils/matrix_logic/validation.py | 19 ++ utils/process_changelog.py | 269 ++++++++++++++++++++- utils/test_process_changelog.py | 210 ++++++++++++++++ 8 files changed, 551 insertions(+), 10 deletions(-) diff --git a/.github/codeowner-signoff-verify-prompt.md b/.github/codeowner-signoff-verify-prompt.md index f3a43222a1..358aee549a 100644 --- a/.github/codeowner-signoff-verify-prompt.md +++ b/.github/codeowner-signoff-verify-prompt.md @@ -19,7 +19,7 @@ You are an automated merge-gate auditor for InferenceX. A CODEOWNER (`${SIGNOFF_AUTHOR}`) just posted the reviewer sign-off checklist (as a ${SIGNOFF_KIND}) that marks PR #${PR_NUMBER} as ready to merge. Your job is to -INDEPENDENTLY verify the checks below (0-10). Do not trust the reviewer's checkmarks. +INDEPENDENTLY verify the checks below (0-12). Do not trust the reviewer's checkmarks. Re-derive every conclusion from CODEOWNERS, CI runs, the PR diff, the master configs, and the linked recipe yourself. Be rigorous and specific. The checks encode the merge standard in `docs/PR_REVIEW_CHECKLIST.md`. Read it in the checked-out @@ -359,8 +359,28 @@ Verify BOTH: unless the sign-off documents a sanctioned exception. - N/A if the PR has no agentic speculative-decoding changes (state that in one line). +## Check 12 — Append-only changes only add new points to an unchanged curve +APPLICABILITY: this check applies when any new `perf-changelog.yaml` entry contains +`append-only: true`. If none does, report N/A. +- Confirm every new changelog entry in the sweep is append-only; mixed regular and + append-only entries are not allowed. +- Inspect the complete PR diff. Only `perf-changelog.yaml` and + `configs/nvidia-master.yaml` / `configs/amd-master.yaml` may change. FAIL on any + benchmark script, launcher, workflow, recipe, or unrelated file change. +- For every selected config, compare the generated matrix at the PR base and head. + Every base curve and concurrency must remain present, and every non-concurrency + field must be identical. In particular, require the exact same image, model, + framework, runner, topology, server arguments, scenario, duration, and offload + settings. The only permitted semantic difference is one or more newly added + concurrency values on existing curves. +- FAIL if an existing concurrency is rerun or removed, a new recipe/curve is created, + or any non-concurrency setting changes. This prevents cherry-picking points from + different images or recipes into one published curve. +- Treat the repository's append-only matrix validation as supporting evidence, but + verify the diff independently and name the offending field/path when failing. + ## Verdict and output -Decide PASS only if Checks 0-11 ALL pass. A check reported as `N/A` counts as a pass. +Decide PASS only if Checks 0-12 ALL pass. A check reported as `N/A` counts as a pass. Keep the `N/A — ` row so the reviewer sees it was considered. Post EXACTLY ONE summary comment on PR #${PR_NUMBER} using `gh pr comment`. Start the comment with the hidden marker so reruns are identifiable: @@ -386,7 +406,7 @@ single terse line. Rules: restating the checklist, no hedging ("if X then maybe Y"). Make the call. Link the run/recipe instead of describing it. -- If everything is to standard: post the verdict header + the twelve one-line rows +- If everything is to standard: post the verdict header + the thirteen one-line rows - If anything is NOT to standard: the verdict header must be immediately followed by a line that @-mentions the sign-off author as `@${SIGNOFF_AUTHOR}` with the blocking summary. Then the per-check lines, each failing one led by its root diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 09443b5f87..2b5dbde28e 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -134,6 +134,15 @@ jobs: - This is a 🔴 **BLOCKING** issue - Comment: "New `perf-changelog.yaml` entries must be appended to the END of the file. The file is read chronologically (oldest at top, newest at bottom), so inserting in the middle or prepending breaks the ordering. Please move the new entry(ies) to the bottom of the file." + ### Append-only Perf Changelog Safety: + When a new `perf-changelog.yaml` entry contains `append-only: true`, verify the complete PR diff before approving it: + - The only changed files may be `perf-changelog.yaml` and `configs/amd-master.yaml` and/or `configs/nvidia-master.yaml`. + - Every newly added changelog entry must contain `append-only: true`; append-only and regular entries may not be mixed. + - Selected existing curves may only gain concurrency values. The container image and every other generated recipe property (model, framework, topology, arguments, sequence lengths, duration, scenario, and so on) must remain unchanged. + - Existing concurrency values, curves, configs, and scenarios may not be removed, replaced, or newly introduced. + - Eval modifiers (`evals-only`, `all-evals`, `eval-min-prefill-ep`) are not allowed. + If any condition fails, report a 🔴 **BLOCKING** issue. This restriction prevents combining cherry-picked data points produced by different images or benchmark logic into one apparent curve. + ## Terminology: - **STP (Single Token Prediction)**: Standard autoregressive decoding — one token per forward pass. No speculative decoding or MTP. Benchmarks labeled "STP only" use vanilla decoding. - **MTP (Multi-Token Prediction)**: Predicts multiple tokens per forward pass using speculative decoding (e.g., EAGLE, NEXTN). diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index e3c525d5e2..386f07ca07 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -223,6 +223,8 @@ jobs: See `docs/configuration-procedures.md` → "Update an image" and "Append the changelog safely" for entry format and rules. Required whenever you change image tags, env vars, or perf-affecting params in `configs/*-master.yaml` or `benchmarks/*.sh`. Use `XXX` as the PR-link placeholder until the PR exists. + If an entry uses `append-only: true`, do not create or approve changes beyond added concurrency values on existing curves. The PR may change only `perf-changelog.yaml` and the selected master config files; the image and all non-concurrency recipe logic must be identical to the base revision, no existing point/curve/config/scenario may be removed, all added changelog entries must be append-only, and eval modifiers are forbidden. Treat a violation as blocking because it would combine incomparable or cherry-picked results into one curve. + ## Spawning Additional Workers: You CAN spawn additional Claude workers by commenting "@claude" with a specific task. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3662dd9541..0b82340600 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,6 +44,30 @@ A full benchmark sweep is expensive GPU time, and the runners are shared by ever - **This reduces CI queue time for everyone.** Each reused merge frees hours of GPU runner time for other PRs, so please prefer the reuse path over merging without it. A green sweep alone is not enough. The `/reuse-sweep-run` comment must be on record (the sign-off verification checks for it), otherwise `main` silently re-runs the full sweep. - `utils/merge_with_reuse.sh ` is the supported merge path. It posts the command, syncs the branch with `main`, waits for checks, and squash-merges. See the [workflows README](.github/workflows/README.md#reusing-an-approved-pr-full-sweep) for eligibility details. +## Adding points to the latest curve with `append-only` + +When a recipe and image are unchanged and a PR only adds concurrency values to an +existing curve, mark every new changelog entry with `append-only: true`. Sweep setup +compares the generated matrices at the base and head revisions, runs only the newly +added points, and emits metadata that lets InferenceX-app extend the most recent +matching curve instead of presenting the partial run as a separate curve. + +This mode is intentionally narrow. An append-only PR may change only +`perf-changelog.yaml` and the master config files; every selected config must already +exist, its prior concurrency points must remain present, and all generated fields +other than concurrency must be identical. Image, recipe, launcher, topology, duration, +or benchmark-logic changes are rejected. Append-only entries cannot be mixed with +regular entries or eval-selection modifiers in the same sweep. + +```yaml +- config-keys: + - dsv4-fp4-b300-vllm-mtp + description: + - "Add concurrency 192 to the existing curve" + pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX + append-only: true +``` + ## AMD cluster: never leave root-owned files in runner workspaces Multi-node benchmarks on the AMD MI355X TW cluster submit Slurm jobs whose containers often run as **root**. If those containers write files (typically `benchmark_logs/logs/slurm_job-*`) into the GitHub Actions runner workspace and the job is **cancelled** before teardown runs, the root-owned directories are stranded. The runner user cannot delete them, so `actions/checkout` fails with: diff --git a/docs/PR_REVIEW_CHECKLIST.md b/docs/PR_REVIEW_CHECKLIST.md index a264c1e650..4e4d6659ba 100644 --- a/docs/PR_REVIEW_CHECKLIST.md +++ b/docs/PR_REVIEW_CHECKLIST.md @@ -28,6 +28,7 @@ As a PR reviewer and CODEOWNER, I have reviewed this and have: - [ ] Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official [vLLM recipes](https://recipes.vllm.ai/) and/or the [SGLang cookbook](https://docs.sglang.io/cookbook/intro): - [ ] I linked the corresponding upstream PR in the [vLLM recipe repo](https://github.com/vllm-project/recipes) or [SGLang repo](https://github.com/sgl-project/sglang/tree/main/docs_new) and verified that it is **MERGED** before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below. - [ ] Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at [docs/waiver/](https://github.com/SemiAnalysisAI/InferenceX/tree/main/docs/waiver)`.md` — named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. +- [ ] If this PR uses `append-only: true`, verified that it only adds previously unmeasured concurrency points to an existing curve: the image and every non-concurrency recipe/launcher/topology/benchmark setting are unchanged, no prior point is removed or rerun, and no unrelated config or code is changed. - [ ] If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below. ### Additional detail section: @@ -42,4 +43,3 @@ Signed: `FILL_IN_GITHUB_USERNAME` image - diff --git a/utils/matrix_logic/validation.py b/utils/matrix_logic/validation.py index f95c1e63d7..6aa2bd5a7a 100644 --- a/utils/matrix_logic/validation.py +++ b/utils/matrix_logic/validation.py @@ -875,6 +875,14 @@ class ChangelogEntry(BaseModel): pr_link: str = Field(alias="pr-link") evals_only: bool = Field(alias="evals-only", default=False) all_evals: bool = Field(alias="all-evals", default=False) + append_only: bool = Field( + alias="append-only", + default=False, + description=( + "Run only concurrency points added to an otherwise unchanged existing " + "curve, then append them to that curve during dashboard ingestion" + ), + ) eval_min_prefill_ep: Optional[int] = Field( alias="eval-min-prefill-ep", default=None, ge=1, description=( @@ -887,6 +895,17 @@ class ChangelogEntry(BaseModel): description="Restrict to specific scenario types (e.g., ['fixed-seq-len', 'agentic-coding'])" ) + @model_validator(mode="after") + def validate_append_only_mode(self): + """Append-only entries are throughput deltas, never eval-only requests.""" + if self.append_only and ( + self.evals_only or self.all_evals or self.eval_min_prefill_ep is not None + ): + raise ValueError( + "append-only cannot be combined with eval selection fields" + ) + return self + class ChangelogMetadata(BaseModel): """Pydantic model for validating changelog metadata structure.""" diff --git a/utils/process_changelog.py b/utils/process_changelog.py index 91b276ba2f..f1a90bd4d4 100644 --- a/utils/process_changelog.py +++ b/utils/process_changelog.py @@ -1,8 +1,12 @@ import argparse +import copy import json import re import subprocess +import tempfile from collections import defaultdict +from contextlib import contextmanager +from pathlib import Path import yaml from constants import GENERATE_SWEEPS_PY_SCRIPT, MASTER_CONFIGS @@ -14,6 +18,8 @@ ) SCENARIO_TYPES = ("fixed-seq-len", "agentic-coding") +APPEND_ONLY_ALLOWED_FILES = {"perf-changelog.yaml", *MASTER_CONFIGS} +CONCURRENCY_CONFIG_FIELDS = {"conc-list", "conc-start", "conc-end"} def _freeze_config_value(value): @@ -143,6 +149,198 @@ def get_config_keys_from_master( return list(resolved_keys) +def get_changed_files(base_ref: str, head_ref: str) -> set[str]: + """Return repository-relative paths changed by the requested sweep diff.""" + result = subprocess.run( + ["git", "diff", "--name-only", base_ref, head_ref], + capture_output=True, + text=True, + check=True, + ) + return {line for line in result.stdout.splitlines() if line} + + +@contextmanager +def config_files_at_ref(ref: str): + """Materialize the master configs from ``ref`` for matrix generation.""" + with tempfile.TemporaryDirectory(prefix="inferencex-append-only-") as temp_dir: + paths = [] + for config_file in MASTER_CONFIGS: + result = subprocess.run( + ["git", "show", f"{ref}:{config_file}"], + capture_output=True, + check=True, + ) + destination = Path(temp_dir) / config_file + destination.parent.mkdir(parents=True, exist_ok=True) + destination.write_bytes(result.stdout) + paths.append(str(destination)) + yield paths + + +def _matrix_curve_key(entry: dict) -> tuple: + """Identify one curve while deliberately excluding point-level fields.""" + return tuple( + sorted( + (key, _freeze_config_value(value)) + for key, value in entry.items() + if key not in {"conc", "exp-name"} + ) + ) + + +def _matrix_concurrencies(entry: dict) -> tuple[int, ...]: + conc = entry.get("conc") + if isinstance(conc, int): + return (conc,) + if isinstance(conc, list) and conc and all(isinstance(value, int) for value in conc): + return tuple(conc) + raise ValueError(f"append-only matrix entry has invalid concurrency value: {conc!r}") + + +def append_only_delta(base_entries: list[dict], head_entries: list[dict]) -> list[dict]: + """Return only newly added points, rejecting any existing-curve mutation. + + Generated matrix rows are the runtime contract. Grouping them without ``conc`` + and ``exp-name`` catches image, launcher, topology, arguments, duration, and + scenario changes while allowing only concurrency-set expansion. + """ + base_groups: dict[tuple, set[int]] = defaultdict(set) + head_groups: dict[tuple, set[int]] = defaultdict(set) + for entry in base_entries: + base_groups[_matrix_curve_key(entry)].update(_matrix_concurrencies(entry)) + for entry in head_entries: + head_groups[_matrix_curve_key(entry)].update(_matrix_concurrencies(entry)) + + if not base_groups: + raise ValueError("append-only requires an existing curve in the base revision") + + removed_curves = base_groups.keys() - head_groups.keys() + new_curves = head_groups.keys() - base_groups.keys() + if removed_curves or new_curves: + raise ValueError( + "append-only may not add, remove, or modify curve logic; only concurrency " + "points may be added" + ) + + for key, base_concurrencies in base_groups.items(): + removed_points = base_concurrencies - head_groups[key] + if removed_points: + raise ValueError( + "append-only may not remove existing concurrency points: " + f"{sorted(removed_points)}" + ) + + delta: list[dict] = [] + emitted_concurrencies: dict[tuple, set[int]] = defaultdict(set) + for entry in head_entries: + key = _matrix_curve_key(entry) + added = head_groups[key] - base_groups[key] + conc = entry.get("conc") + if isinstance(conc, int): + if conc in added and conc not in emitted_concurrencies[key]: + delta.append(entry) + emitted_concurrencies[key].add(conc) + continue + added_in_source_order = [] + for value in conc: + if value in added and value not in emitted_concurrencies[key]: + added_in_source_order.append(value) + emitted_concurrencies[key].add(value) + if added_in_source_order: + delta_entry = copy.deepcopy(entry) + delta_entry["conc"] = added_in_source_order + delta.append(delta_entry) + + if not delta: + raise ValueError("append-only did not add any concurrency points") + return delta + + +def validate_append_only_scope( + base_ref: str, + head_ref: str, + base_master: dict, + head_master: dict, + selected_config_scenarios: dict[str, set[str]], +) -> None: + """Reject code changes and unrelated config edits in an append-only PR.""" + unexpected_files = get_changed_files(base_ref, head_ref) - APPEND_ONLY_ALLOWED_FILES + if unexpected_files: + raise ValueError( + "append-only PRs may change only perf-changelog.yaml and master config " + f"files; unexpected changes: {sorted(unexpected_files)}" + ) + + selected_configs = selected_config_scenarios.keys() + all_keys = base_master.keys() | head_master.keys() + unrelated_changes = [ + key + for key in all_keys + if key not in selected_configs and base_master.get(key) != head_master.get(key) + ] + if unrelated_changes: + raise ValueError( + "append-only PR changed configs not selected by its changelog entry: " + f"{sorted(unrelated_changes)}" + ) + + for config, allowed_scenarios in selected_config_scenarios.items(): + base_config = base_master[config] + head_config = head_master[config] + if base_config.keys() != head_config.keys(): + raise ValueError( + f"append-only changed top-level fields for config {config!r}" + ) + for field in base_config.keys() - {"scenarios"}: + if base_config[field] != head_config[field]: + raise ValueError( + f"append-only changed non-scenario field {field!r} in {config!r}" + ) + + base_scenarios = base_config.get("scenarios", {}) + head_scenarios = head_config.get("scenarios", {}) + if base_scenarios.keys() != head_scenarios.keys(): + raise ValueError( + f"append-only added or removed a scenario in config {config!r}" + ) + for scenario in base_scenarios: + if scenario not in allowed_scenarios: + if base_scenarios[scenario] != head_scenarios[scenario]: + raise ValueError( + "append-only changed a scenario outside its changelog scope: " + f"{config!r} / {scenario!r}" + ) + continue + _validate_concurrency_only_structure( + base_scenarios[scenario], + head_scenarios[scenario], + f"{config}.{scenario}", + ) + + +def _validate_concurrency_only_structure(base, head, path: str) -> None: + """Require identical config structure except at explicit concurrency fields.""" + if isinstance(base, dict) and isinstance(head, dict): + base_keys = base.keys() - CONCURRENCY_CONFIG_FIELDS + head_keys = head.keys() - CONCURRENCY_CONFIG_FIELDS + if base_keys != head_keys: + raise ValueError(f"append-only changed config structure at {path}") + for key in base_keys: + _validate_concurrency_only_structure(base[key], head[key], f"{path}.{key}") + return + if isinstance(base, list) and isinstance(head, list): + if len(base) != len(head): + raise ValueError(f"append-only changed config structure at {path}") + for index, (base_item, head_item) in enumerate(zip(base, head)): + _validate_concurrency_only_structure( + base_item, head_item, f"{path}[{index}]" + ) + return + if base != head: + raise ValueError(f"append-only changed non-concurrency value at {path}") + + def main(): parser = argparse.ArgumentParser() parser.add_argument("--base-ref", type=str, required=True) @@ -171,6 +369,17 @@ def main(): if not changelog_data: raise ValueError("No valid YAML entries found in the changelog additions.") + parsed_entries = [ChangelogEntry.model_validate(entry) for entry in changelog_data] + has_append_only = any(entry.append_only for entry in parsed_entries) + if has_append_only and not all(entry.append_only for entry in parsed_entries): + raise ValueError( + "append-only entries cannot share a sweep with regular changelog entries" + ) + if has_append_only and (args.all_evals or args.evals_only): + raise ValueError( + "append-only sweeps cannot use all-evals or evals-only modifiers" + ) + final_results = { "single_node": defaultdict(list), "multi_node": defaultdict(list), @@ -194,13 +403,39 @@ def main(): master_config = load_config_files(MASTER_CONFIGS) resolved_entries = [] - for entry_data in changelog_data: - entry = ChangelogEntry.model_validate(entry_data) + for entry in parsed_entries: all_configs = get_config_keys_from_master( entry.config_keys, master_config ) resolved_entries.append((entry, all_configs)) + base_config_context = None + base_config_files = None + if has_append_only: + base_config_context = config_files_at_ref(args.base_ref) + base_config_files = base_config_context.__enter__() + base_master = load_config_files(base_config_files) + selected_config_scenarios: dict[str, set[str]] = defaultdict(set) + for entry, configs in resolved_entries: + for config in configs: + selected_config_scenarios[config].update( + entry.scenario_type or SCENARIO_TYPES + ) + selected_configs = selected_config_scenarios.keys() + missing_from_base = selected_configs - base_master.keys() + if missing_from_base: + raise ValueError( + "append-only requires every selected config to exist in the base " + f"revision; missing: {sorted(missing_from_base)}" + ) + validate_append_only_scope( + args.base_ref, + args.head_ref, + base_master, + master_config, + selected_config_scenarios, + ) + # Process all-evals entries first so their broader eval matrix wins when # the same config appears in multiple changelog entries. resolved_entries.sort(key=lambda item: not item[0].all_evals) @@ -230,7 +465,7 @@ def main(): benchmark_groups[unseen_scenarios].append(config) for scenarios, benchmark_configs in benchmark_groups.items(): - base_cmd = [ + head_cmd = [ "python3", GENERATE_SWEEPS_PY_SCRIPT, "test-config", @@ -241,18 +476,37 @@ def main(): "--no-evals", ] if scenarios != SCENARIO_TYPES: - base_cmd.extend(["--scenario-type", *scenarios]) + head_cmd.extend(["--scenario-type", *scenarios]) try: result = subprocess.run( - base_cmd, + head_cmd, capture_output=True, text=True, check=True, ) + head_results = json.loads(result.stdout) + if entry.append_only: + base_cmd = head_cmd.copy() + config_files_index = base_cmd.index("--config-files") + 1 + base_cmd[ + config_files_index:config_files_index + len(MASTER_CONFIGS) + ] = base_config_files + base_result = subprocess.run( + base_cmd, + capture_output=True, + text=True, + check=True, + ) + head_results = append_only_delta( + json.loads(base_result.stdout), head_results + ) except subprocess.CalledProcessError as e: print(e.stderr) raise - all_benchmark_results.extend(json.loads(result.stdout)) + all_benchmark_results.extend(head_results) + + if entry.append_only: + continue eval_groups = defaultdict(list) for config in all_configs: @@ -299,6 +553,9 @@ def main(): ) all_eval_results.extend(entry_eval_results) + if base_config_context is not None: + base_config_context.__exit__(None, None, None) + if args.trim_conc: all_benchmark_results = trim_conc(all_benchmark_results) diff --git a/utils/test_process_changelog.py b/utils/test_process_changelog.py index dfe677014e..6dbd7fba32 100644 --- a/utils/test_process_changelog.py +++ b/utils/test_process_changelog.py @@ -3,11 +3,38 @@ import json import subprocess import sys +from contextlib import nullcontext from types import SimpleNamespace import process_changelog +def _fixed_matrix_row(conc, *, image="vllm/vllm-openai:v0.16.0"): + return { + "image": image, + "model": "deepseek-ai/DeepSeek-V4-Pro", + "model-prefix": "dsv4", + "precision": "fp4", + "framework": "vllm", + "spec-decoding": "mtp", + "runner": "cluster:b300-nv", + "isl": 8192, + "osl": 1024, + "tp": 8, + "pp": 1, + "dcp-size": 1, + "pcp-size": 1, + "ep": 8, + "dp-attn": True, + "conc": conc, + "max-model-len": 10240, + "exp-name": f"dsv4_conc{conc}", + "disagg": False, + "run-eval": False, + "eval-only": False, + } + + def _scenario_values(command): if "--scenario-type" not in command: return [] @@ -59,6 +86,189 @@ def test_config_key_expansion_is_deterministic_and_deduplicated(): assert result == ["config-b", "config-a"] +def test_append_only_delta_keeps_only_new_single_node_points(): + base = [_fixed_matrix_row(4), _fixed_matrix_row(8)] + head = [*base, _fixed_matrix_row(12)] + + delta = process_changelog.append_only_delta(base, head) + + assert [entry["conc"] for entry in delta] == [12] + + +def test_append_only_delta_slices_multinode_concurrency_lists(): + common = { + "image": "lmsysorg/sglang:v0.5.7", + "model": "deepseek-ai/DeepSeek-V4-Pro", + "model-prefix": "dsv4", + "precision": "fp4", + "framework": "dynamo-sglang", + "conc": [8, 16], + "exp-name": "dsv4-disagg", + } + + delta = process_changelog.append_only_delta( + [common], + [{**common, "conc": [8, 16, 24]}], + ) + + assert delta == [{**common, "conc": [24]}] + + +def test_append_only_delta_deduplicates_new_single_node_points(): + base = [_fixed_matrix_row(4)] + head = [base[0], _fixed_matrix_row(8), _fixed_matrix_row(8)] + + delta = process_changelog.append_only_delta(base, head) + + assert [entry["conc"] for entry in delta] == [8] + + +def test_append_only_delta_deduplicates_multinode_concurrency_lists(): + common = { + "image": "lmsysorg/sglang:v0.5.7", + "model": "deepseek-ai/DeepSeek-V4-Pro", + "framework": "dynamo-sglang", + "conc": [8, 16], + "exp-name": "dsv4-disagg", + } + + delta = process_changelog.append_only_delta( + [common], + [{**common, "conc": [8, 16, 24, 24]}], + ) + + assert delta == [{**common, "conc": [24]}] + + +def test_append_only_delta_rejects_image_changes(): + base = [_fixed_matrix_row(4)] + head = [ + _fixed_matrix_row(4, image="vllm/vllm-openai:v0.16.1"), + _fixed_matrix_row(8, image="vllm/vllm-openai:v0.16.1"), + ] + + try: + process_changelog.append_only_delta(base, head) + except ValueError as error: + assert "curve logic" in str(error) + else: + raise AssertionError("image mutation should reject append-only mode") + + +def test_append_only_scope_rejects_non_concurrency_recipe_changes(monkeypatch): + base = { + "test-config": { + "image": "vllm/vllm-openai:v0.16.0", + "scenarios": { + "agentic-coding": { + "duration": 3600, + "search-space": [{"tp": 8, "conc-list": [1, 4]}], + } + }, + } + } + head = { + "test-config": { + "image": "vllm/vllm-openai:v0.16.0", + "scenarios": { + "agentic-coding": { + "duration": 1800, + "search-space": [{"tp": 8, "conc-list": [1, 4, 8]}], + } + }, + } + } + monkeypatch.setattr(process_changelog, "get_changed_files", lambda *_: set()) + + try: + process_changelog.validate_append_only_scope( + "base", "head", base, head, {"test-config": {"agentic-coding"}} + ) + except ValueError as error: + assert "duration" in str(error) + else: + raise AssertionError("recipe mutation should reject append-only mode") + + +def test_append_only_scope_allows_range_to_list_expansion(monkeypatch): + base = { + "test-config": { + "image": "vllm/vllm-openai:v0.16.0", + "scenarios": { + "fixed-seq-len": { + "search-space": [{"tp": 8, "conc-start": 4, "conc-end": 64}], + } + }, + } + } + head = { + "test-config": { + "image": "vllm/vllm-openai:v0.16.0", + "scenarios": { + "fixed-seq-len": { + "search-space": [{"tp": 8, "conc-list": [4, 16, 32, 64]}], + } + }, + } + } + monkeypatch.setattr(process_changelog, "get_changed_files", lambda *_: set()) + + process_changelog.validate_append_only_scope( + "base", "head", base, head, {"test-config": {"fixed-seq-len"}} + ) + + +def test_append_only_main_runs_only_added_points_and_skips_evals( + monkeypatch, + capsys, +): + added_yaml = """ +- config-keys: + - test-config + description: + - Add one concurrency point without rerunning the curve + pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1 + append-only: true +""" + base_rows = [_fixed_matrix_row(4)] + head_rows = [*base_rows, _fixed_matrix_row(8)] + commands = [] + + monkeypatch.setattr(process_changelog, "get_added_lines", lambda *_: added_yaml) + monkeypatch.setattr(process_changelog, "get_changed_files", lambda *_: set()) + monkeypatch.setattr( + process_changelog, + "config_files_at_ref", + lambda *_: nullcontext(["base-nvidia.yaml", "base-amd.yaml"]), + ) + monkeypatch.setattr( + process_changelog, + "load_config_files", + lambda _: {"test-config": {"image": "vllm/vllm-openai:v0.16.0"}}, + ) + + def fake_run(command, **kwargs): + commands.append(command) + rows = base_rows if "base-nvidia.yaml" in command else head_rows + return SimpleNamespace(stdout=json.dumps(rows)) + + monkeypatch.setattr(subprocess, "run", fake_run) + monkeypatch.setattr(sys, "argv", [ + "process_changelog.py", + "--base-ref", "base", + "--head-ref", "head", + "--changelog-file", "perf-changelog.yaml", + ]) + + process_changelog.main() + + output = json.loads(capsys.readouterr().out) + assert [row["conc"] for row in output["single_node"]["8k1k"]] == [8] + assert output["evals"] == [] + assert output["changelog_metadata"]["entries"][0]["append-only"] is True + assert len(commands) == 2 + + def test_all_evals_skips_benchmarks_and_uses_all_evals_generator_flag( monkeypatch, capsys, From bded8e12f399ba1d4511a788f25a73976f3306cd Mon Sep 17 00:00:00 2001 From: Cam Quilici Date: Fri, 14 Aug 2026 15:58:22 -0500 Subject: [PATCH 2/4] fix(ci): allow point-gated launch script changes --- .github/codeowner-signoff-verify-prompt.md | 20 +++++--- .github/workflows/claude-pr-review.yml | 5 +- .github/workflows/claude.yml | 2 +- CONTRIBUTING.md | 17 ++++--- docs/PR_REVIEW_CHECKLIST.md | 2 +- utils/process_changelog.py | 25 ++++++++-- utils/test_process_changelog.py | 53 ++++++++++++++++++++++ 7 files changed, 104 insertions(+), 20 deletions(-) diff --git a/.github/codeowner-signoff-verify-prompt.md b/.github/codeowner-signoff-verify-prompt.md index 358aee549a..d3256113dd 100644 --- a/.github/codeowner-signoff-verify-prompt.md +++ b/.github/codeowner-signoff-verify-prompt.md @@ -364,18 +364,26 @@ APPLICABILITY: this check applies when any new `perf-changelog.yaml` entry conta `append-only: true`. If none does, report N/A. - Confirm every new changelog entry in the sweep is append-only; mixed regular and append-only entries are not allowed. -- Inspect the complete PR diff. Only `perf-changelog.yaml` and - `configs/nvidia-master.yaml` / `configs/amd-master.yaml` may change. FAIL on any - benchmark script, launcher, workflow, recipe, or unrelated file change. +- Inspect the complete PR diff. Allowed files are `perf-changelog.yaml`, + `configs/nvidia-master.yaml` / `configs/amd-master.yaml`, directly used + `benchmarks/**/*.sh` scripts, and `runners/launch_*.sh` launchers. FAIL on a + workflow, non-shell helper, unrelated recipe, or unrelated file change. - For every selected config, compare the generated matrix at the PR base and head. Every base curve and concurrency must remain present, and every non-concurrency field must be identical. In particular, require the exact same image, model, framework, runner, topology, server arguments, scenario, duration, and offload - settings. The only permitted semantic difference is one or more newly added + settings. The generated matrix may differ only by one or more newly added concurrency values on existing curves. +- For every changed benchmark/launch script, trace the selected config and generated + runtime values into the changed control flow. Every changed line must be reachable + only for the corresponding newly appended points. PASS a script change when its + branch condition is uniquely satisfied by those points. FAIL an unguarded/shared + setup change, a condition also satisfied by an existing point, or any case where + exclusivity cannot be proven from the diff. Do not fail solely because an eligible + script file changed. - FAIL if an existing concurrency is rerun or removed, a new recipe/curve is created, - or any non-concurrency setting changes. This prevents cherry-picking points from - different images or recipes into one published curve. + or a non-concurrency config setting changes. A script-logic change is the narrow + exception above and must be exclusive to the appended points. - Treat the repository's append-only matrix validation as supporting evidence, but verify the diff independently and name the offending field/path when failing. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 2b5dbde28e..9d2801c4b8 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -136,12 +136,13 @@ jobs: ### Append-only Perf Changelog Safety: When a new `perf-changelog.yaml` entry contains `append-only: true`, verify the complete PR diff before approving it: - - The only changed files may be `perf-changelog.yaml` and `configs/amd-master.yaml` and/or `configs/nvidia-master.yaml`. + - Changed files may be `perf-changelog.yaml`, `configs/amd-master.yaml` and/or `configs/nvidia-master.yaml`, directly used `benchmarks/**/*.sh` scripts, and `runners/launch_*.sh` launchers. Other files are forbidden. - Every newly added changelog entry must contain `append-only: true`; append-only and regular entries may not be mixed. - Selected existing curves may only gain concurrency values. The container image and every other generated recipe property (model, framework, topology, arguments, sequence lengths, duration, scenario, and so on) must remain unchanged. + - A benchmark/launch script may change only when every changed line is on a control-flow path uniquely gated to the corresponding newly appended points. Trace the selected config and generated runtime values into the script condition. Confirm the condition cannot be true for any existing point. Unguarded/shared setup changes, or a branch also used by an existing concurrency/config/scenario, are blocking. - Existing concurrency values, curves, configs, and scenarios may not be removed, replaced, or newly introduced. - Eval modifiers (`evals-only`, `all-evals`, `eval-min-prefill-ep`) are not allowed. - If any condition fails, report a 🔴 **BLOCKING** issue. This restriction prevents combining cherry-picked data points produced by different images or benchmark logic into one apparent curve. + If any condition fails, report a 🔴 **BLOCKING** issue. Do not reject a script change merely because the file changed; reject it when the changed path is not exclusive to the appended points or the exclusivity cannot be proven from the diff. ## Terminology: - **STP (Single Token Prediction)**: Standard autoregressive decoding — one token per forward pass. No speculative decoding or MTP. Benchmarks labeled "STP only" use vanilla decoding. diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 386f07ca07..faf1992d76 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -223,7 +223,7 @@ jobs: See `docs/configuration-procedures.md` → "Update an image" and "Append the changelog safely" for entry format and rules. Required whenever you change image tags, env vars, or perf-affecting params in `configs/*-master.yaml` or `benchmarks/*.sh`. Use `XXX` as the PR-link placeholder until the PR exists. - If an entry uses `append-only: true`, do not create or approve changes beyond added concurrency values on existing curves. The PR may change only `perf-changelog.yaml` and the selected master config files; the image and all non-concurrency recipe logic must be identical to the base revision, no existing point/curve/config/scenario may be removed, all added changelog entries must be append-only, and eval modifiers are forbidden. Treat a violation as blocking because it would combine incomparable or cherry-picked results into one curve. + If an entry uses `append-only: true`, allow only added concurrency values on existing curves. The PR may change `perf-changelog.yaml`, selected master config files, directly used `benchmarks/**/*.sh` scripts, and `runners/launch_*.sh` launchers. The image and generated non-concurrency recipe fields must remain identical, no existing point/curve/config/scenario may be removed, all added changelog entries must be append-only, and eval modifiers are forbidden. A changed script path is permitted only when it is uniquely gated to the corresponding newly appended points and cannot execute for any existing point; unguarded or shared-path changes are blocking. ## Spawning Additional Workers: You CAN spawn additional Claude workers by commenting "@claude" with a specific task. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0b82340600..5abcf2c03b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,12 +52,17 @@ compares the generated matrices at the base and head revisions, runs only the ne added points, and emits metadata that lets InferenceX-app extend the most recent matching curve instead of presenting the partial run as a separate curve. -This mode is intentionally narrow. An append-only PR may change only -`perf-changelog.yaml` and the master config files; every selected config must already -exist, its prior concurrency points must remain present, and all generated fields -other than concurrency must be identical. Image, recipe, launcher, topology, duration, -or benchmark-logic changes are rejected. Append-only entries cannot be mixed with -regular entries or eval-selection modifiers in the same sweep. +This mode is intentionally narrow. An append-only PR may change +`perf-changelog.yaml`, the master config files, and directly used benchmark/launch +shell scripts. Every selected config must already exist, its prior concurrency points +must remain present, and all generated fields other than concurrency must be +identical. A script change is allowed only when its changed control-flow path is +exclusively gated to the newly appended points named by the changelog; no changed line +may execute for an existing point. Image, shared launcher logic, topology, duration, +or unrelated benchmark-logic changes are rejected. Append-only entries cannot be +mixed with regular entries or eval-selection modifiers in the same sweep. The matrix +validator enforces the generated config and file scope; the AI reviewer must trace and +verify the script control-flow condition. ```yaml - config-keys: diff --git a/docs/PR_REVIEW_CHECKLIST.md b/docs/PR_REVIEW_CHECKLIST.md index 4e4d6659ba..07ef95300c 100644 --- a/docs/PR_REVIEW_CHECKLIST.md +++ b/docs/PR_REVIEW_CHECKLIST.md @@ -28,7 +28,7 @@ As a PR reviewer and CODEOWNER, I have reviewed this and have: - [ ] Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official [vLLM recipes](https://recipes.vllm.ai/) and/or the [SGLang cookbook](https://docs.sglang.io/cookbook/intro): - [ ] I linked the corresponding upstream PR in the [vLLM recipe repo](https://github.com/vllm-project/recipes) or [SGLang repo](https://github.com/sgl-project/sglang/tree/main/docs_new) and verified that it is **MERGED** before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below. - [ ] Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at [docs/waiver/](https://github.com/SemiAnalysisAI/InferenceX/tree/main/docs/waiver)`.md` — named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. -- [ ] If this PR uses `append-only: true`, verified that it only adds previously unmeasured concurrency points to an existing curve: the image and every non-concurrency recipe/launcher/topology/benchmark setting are unchanged, no prior point is removed or rerun, and no unrelated config or code is changed. +- [ ] If this PR uses `append-only: true`, verified that it only adds previously unmeasured concurrency points to an existing curve: the image and generated non-concurrency recipe/topology settings are unchanged, no prior point is removed or rerun, and any benchmark/launch script change is on a control-flow path that can execute only for the corresponding newly appended points (never an existing point). - [ ] If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below. ### Additional detail section: diff --git a/utils/process_changelog.py b/utils/process_changelog.py index f1a90bd4d4..c970c2dd82 100644 --- a/utils/process_changelog.py +++ b/utils/process_changelog.py @@ -18,10 +18,22 @@ ) SCENARIO_TYPES = ("fixed-seq-len", "agentic-coding") -APPEND_ONLY_ALLOWED_FILES = {"perf-changelog.yaml", *MASTER_CONFIGS} +APPEND_ONLY_ALWAYS_ALLOWED_FILES = {"perf-changelog.yaml", *MASTER_CONFIGS} CONCURRENCY_CONFIG_FIELDS = {"conc-list", "conc-start", "conc-end"} +def is_append_only_allowed_file(filepath: str) -> bool: + """Allow config inputs plus launch scripts whose control flow is AI-reviewed.""" + if filepath in APPEND_ONLY_ALWAYS_ALLOWED_FILES: + return True + path = Path(filepath) + if path.suffix != ".sh": + return False + if path.parts and path.parts[0] == "benchmarks": + return True + return path.parent == Path("runners") and path.name.startswith("launch_") + + def _freeze_config_value(value): """Convert JSON-shaped config values into deterministic hashable values.""" if isinstance(value, dict): @@ -265,11 +277,16 @@ def validate_append_only_scope( selected_config_scenarios: dict[str, set[str]], ) -> None: """Reject code changes and unrelated config edits in an append-only PR.""" - unexpected_files = get_changed_files(base_ref, head_ref) - APPEND_ONLY_ALLOWED_FILES + unexpected_files = { + filepath + for filepath in get_changed_files(base_ref, head_ref) + if not is_append_only_allowed_file(filepath) + } if unexpected_files: raise ValueError( - "append-only PRs may change only perf-changelog.yaml and master config " - f"files; unexpected changes: {sorted(unexpected_files)}" + "append-only PRs may change only perf-changelog.yaml, master configs, " + "and benchmark/launch shell scripts; unexpected changes: " + f"{sorted(unexpected_files)}" ) selected_configs = selected_config_scenarios.keys() diff --git a/utils/test_process_changelog.py b/utils/test_process_changelog.py index 6dbd7fba32..b6243935fd 100644 --- a/utils/test_process_changelog.py +++ b/utils/test_process_changelog.py @@ -218,6 +218,59 @@ def test_append_only_scope_allows_range_to_list_expansion(monkeypatch): ) +def test_append_only_scope_allows_benchmark_and_launch_shell_scripts(monkeypatch): + master = { + "test-config": { + "image": "vllm/vllm-openai:v0.16.0", + "scenarios": { + "fixed-seq-len": { + "search-space": [{"tp": 8, "conc-list": [4, 8]}], + } + }, + } + } + monkeypatch.setattr( + process_changelog, + "get_changed_files", + lambda *_: { + "perf-changelog.yaml", + "configs/nvidia-master.yaml", + "benchmarks/single_node/test.sh", + "benchmarks/multi_node/helpers/server.sh", + "runners/launch_b300-nv.sh", + }, + ) + + process_changelog.validate_append_only_scope( + "base", "head", master, master, {"test-config": {"fixed-seq-len"}} + ) + + +def test_append_only_scope_rejects_shared_or_non_shell_logic(monkeypatch): + master = { + "test-config": { + "image": "vllm/vllm-openai:v0.16.0", + "scenarios": {"fixed-seq-len": {}}, + } + } + monkeypatch.setattr( + process_changelog, + "get_changed_files", + lambda *_: {"runners/slurm_utils.sh", "benchmarks/single_node/helper.py"}, + ) + + try: + process_changelog.validate_append_only_scope( + "base", "head", master, master, {"test-config": {"fixed-seq-len"}} + ) + except ValueError as error: + assert "unexpected changes" in str(error) + assert "runners/slurm_utils.sh" in str(error) + assert "benchmarks/single_node/helper.py" in str(error) + else: + raise AssertionError("shared and non-shell logic should remain out of scope") + + def test_append_only_main_runs_only_added_points_and_skips_evals( monkeypatch, capsys, From 598c9f566b1bb0644f89b5193252d71c0b799b14 Mon Sep 17 00:00:00 2001 From: Cam Quilici Date: Fri, 14 Aug 2026 16:04:27 -0500 Subject: [PATCH 3/4] fix(ci): defer append-only impact checks to review --- .github/codeowner-signoff-verify-prompt.md | 24 ++++---- .github/workflows/claude-pr-review.yml | 6 +- .github/workflows/claude.yml | 2 +- CONTRIBUTING.md | 21 ++++--- docs/PR_REVIEW_CHECKLIST.md | 2 +- utils/process_changelog.py | 42 +------------- utils/test_process_changelog.py | 66 ++-------------------- 7 files changed, 31 insertions(+), 132 deletions(-) diff --git a/.github/codeowner-signoff-verify-prompt.md b/.github/codeowner-signoff-verify-prompt.md index d3256113dd..ccadf40299 100644 --- a/.github/codeowner-signoff-verify-prompt.md +++ b/.github/codeowner-signoff-verify-prompt.md @@ -364,26 +364,24 @@ APPLICABILITY: this check applies when any new `perf-changelog.yaml` entry conta `append-only: true`. If none does, report N/A. - Confirm every new changelog entry in the sweep is append-only; mixed regular and append-only entries are not allowed. -- Inspect the complete PR diff. Allowed files are `perf-changelog.yaml`, - `configs/nvidia-master.yaml` / `configs/amd-master.yaml`, directly used - `benchmarks/**/*.sh` scripts, and `runners/launch_*.sh` launchers. FAIL on a - workflow, non-shell helper, unrelated recipe, or unrelated file change. +- Inspect the complete PR diff without using a file allowlist. Supporting code, + benchmark scripts, launchers, helpers, and other files may change. Their path alone + is never a reason to fail; determine whether each benchmark-affecting change is + behaviorally isolated to the appended points. - For every selected config, compare the generated matrix at the PR base and head. Every base curve and concurrency must remain present, and every non-concurrency field must be identical. In particular, require the exact same image, model, framework, runner, topology, server arguments, scenario, duration, and offload settings. The generated matrix may differ only by one or more newly added concurrency values on existing curves. -- For every changed benchmark/launch script, trace the selected config and generated - runtime values into the changed control flow. Every changed line must be reachable - only for the corresponding newly appended points. PASS a script change when its - branch condition is uniquely satisfied by those points. FAIL an unguarded/shared - setup change, a condition also satisfied by an existing point, or any case where - exclusivity cannot be proven from the diff. Do not fail solely because an eligible - script file changed. +- Trace the selected config and generated runtime values through every affected file + into the changed behavior. The behavior must be reachable only for the corresponding + newly appended points. PASS when the controlling condition is uniquely satisfied by + those points. FAIL an unguarded/shared setup change, a condition also satisfied by an + existing point, or any case where exclusivity cannot be proven from the diff. - FAIL if an existing concurrency is rerun or removed, a new recipe/curve is created, - or a non-concurrency config setting changes. A script-logic change is the narrow - exception above and must be exclusive to the appended points. + or a non-concurrency config setting changes. Other benchmark-affecting changes are + permitted only under the behavioral-isolation rule above. - Treat the repository's append-only matrix validation as supporting evidence, but verify the diff independently and name the offending field/path when failing. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 9d2801c4b8..cf97c1c00d 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -136,13 +136,13 @@ jobs: ### Append-only Perf Changelog Safety: When a new `perf-changelog.yaml` entry contains `append-only: true`, verify the complete PR diff before approving it: - - Changed files may be `perf-changelog.yaml`, `configs/amd-master.yaml` and/or `configs/nvidia-master.yaml`, directly used `benchmarks/**/*.sh` scripts, and `runners/launch_*.sh` launchers. Other files are forbidden. + - Do not use a file allowlist. Supporting code, benchmark scripts, launchers, helpers, and other files may change. Inspect the complete diff and judge whether each benchmark-affecting change is behaviorally isolated to the appended points. - Every newly added changelog entry must contain `append-only: true`; append-only and regular entries may not be mixed. - Selected existing curves may only gain concurrency values. The container image and every other generated recipe property (model, framework, topology, arguments, sequence lengths, duration, scenario, and so on) must remain unchanged. - - A benchmark/launch script may change only when every changed line is on a control-flow path uniquely gated to the corresponding newly appended points. Trace the selected config and generated runtime values into the script condition. Confirm the condition cannot be true for any existing point. Unguarded/shared setup changes, or a branch also used by an existing concurrency/config/scenario, are blocking. + - Benchmark or launch logic may change only when every changed behavior is on a control-flow path uniquely gated to the corresponding newly appended points. Trace the selected config and generated runtime values through every affected file into the condition. Confirm the path cannot be reached by any existing point. Unguarded/shared setup changes, or a branch also used by an existing concurrency/config/scenario, are blocking. - Existing concurrency values, curves, configs, and scenarios may not be removed, replaced, or newly introduced. - Eval modifiers (`evals-only`, `all-evals`, `eval-min-prefill-ep`) are not allowed. - If any condition fails, report a 🔴 **BLOCKING** issue. Do not reject a script change merely because the file changed; reject it when the changed path is not exclusive to the appended points or the exclusivity cannot be proven from the diff. + If any condition fails, report a 🔴 **BLOCKING** issue. Never reject a change merely because of its file path; reject it when its benchmark effect is not exclusive to the appended points or the exclusivity cannot be proven from the diff. ## Terminology: - **STP (Single Token Prediction)**: Standard autoregressive decoding — one token per forward pass. No speculative decoding or MTP. Benchmarks labeled "STP only" use vanilla decoding. diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index faf1992d76..405ec7be07 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -223,7 +223,7 @@ jobs: See `docs/configuration-procedures.md` → "Update an image" and "Append the changelog safely" for entry format and rules. Required whenever you change image tags, env vars, or perf-affecting params in `configs/*-master.yaml` or `benchmarks/*.sh`. Use `XXX` as the PR-link placeholder until the PR exists. - If an entry uses `append-only: true`, allow only added concurrency values on existing curves. The PR may change `perf-changelog.yaml`, selected master config files, directly used `benchmarks/**/*.sh` scripts, and `runners/launch_*.sh` launchers. The image and generated non-concurrency recipe fields must remain identical, no existing point/curve/config/scenario may be removed, all added changelog entries must be append-only, and eval modifiers are forbidden. A changed script path is permitted only when it is uniquely gated to the corresponding newly appended points and cannot execute for any existing point; unguarded or shared-path changes are blocking. + If an entry uses `append-only: true`, allow only added concurrency values on existing curves. Do not enforce a file allowlist: supporting code, benchmark scripts, launchers, helpers, and other files may change when their benchmark effect is exclusive to the corresponding newly appended points. The image and generated non-concurrency recipe fields must remain identical, no existing point/curve/config/scenario may be removed, all added changelog entries must be append-only, and eval modifiers are forbidden. Trace behavior through the complete diff; unguarded changes or paths reachable by an existing point are blocking. ## Spawning Additional Workers: You CAN spawn additional Claude workers by commenting "@claude" with a specific task. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5abcf2c03b..45f0280a4b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,17 +52,16 @@ compares the generated matrices at the base and head revisions, runs only the ne added points, and emits metadata that lets InferenceX-app extend the most recent matching curve instead of presenting the partial run as a separate curve. -This mode is intentionally narrow. An append-only PR may change -`perf-changelog.yaml`, the master config files, and directly used benchmark/launch -shell scripts. Every selected config must already exist, its prior concurrency points -must remain present, and all generated fields other than concurrency must be -identical. A script change is allowed only when its changed control-flow path is -exclusively gated to the newly appended points named by the changelog; no changed line -may execute for an existing point. Image, shared launcher logic, topology, duration, -or unrelated benchmark-logic changes are rejected. Append-only entries cannot be -mixed with regular entries or eval-selection modifiers in the same sweep. The matrix -validator enforces the generated config and file scope; the AI reviewer must trace and -verify the script control-flow condition. +This mode is intentionally narrow, but it is not based on a file allowlist. Supporting +code, benchmark scripts, launchers, and other files may change when their behavioral +effect is exclusive to the newly appended points named by the changelog. No changed +benchmark path may execute for or alter an existing point. Every selected config must +already exist, its prior concurrency points must remain present, and all generated +fields other than concurrency must be identical. Image changes or shared logic changes +that can affect existing points are rejected. Append-only entries cannot be mixed with +regular entries or eval-selection modifiers in the same sweep. The matrix validator +enforces deterministic generated-config invariants; the human and AI reviewers must +inspect the complete diff and verify behavioral isolation. ```yaml - config-keys: diff --git a/docs/PR_REVIEW_CHECKLIST.md b/docs/PR_REVIEW_CHECKLIST.md index 07ef95300c..fd6cfeace8 100644 --- a/docs/PR_REVIEW_CHECKLIST.md +++ b/docs/PR_REVIEW_CHECKLIST.md @@ -28,7 +28,7 @@ As a PR reviewer and CODEOWNER, I have reviewed this and have: - [ ] Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official [vLLM recipes](https://recipes.vllm.ai/) and/or the [SGLang cookbook](https://docs.sglang.io/cookbook/intro): - [ ] I linked the corresponding upstream PR in the [vLLM recipe repo](https://github.com/vllm-project/recipes) or [SGLang repo](https://github.com/sgl-project/sglang/tree/main/docs_new) and verified that it is **MERGED** before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below. - [ ] Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at [docs/waiver/](https://github.com/SemiAnalysisAI/InferenceX/tree/main/docs/waiver)`.md` — named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. -- [ ] If this PR uses `append-only: true`, verified that it only adds previously unmeasured concurrency points to an existing curve: the image and generated non-concurrency recipe/topology settings are unchanged, no prior point is removed or rerun, and any benchmark/launch script change is on a control-flow path that can execute only for the corresponding newly appended points (never an existing point). +- [ ] If this PR uses `append-only: true`, verified that it only adds previously unmeasured concurrency points to an existing curve: the image and generated non-concurrency recipe/topology settings are unchanged, no prior point is removed or rerun, and every benchmark-affecting change in the complete diff can affect only the corresponding newly appended points (never an existing point), regardless of which file contains it. - [ ] If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below. ### Additional detail section: diff --git a/utils/process_changelog.py b/utils/process_changelog.py index c970c2dd82..04f1edc1f7 100644 --- a/utils/process_changelog.py +++ b/utils/process_changelog.py @@ -18,22 +18,9 @@ ) SCENARIO_TYPES = ("fixed-seq-len", "agentic-coding") -APPEND_ONLY_ALWAYS_ALLOWED_FILES = {"perf-changelog.yaml", *MASTER_CONFIGS} CONCURRENCY_CONFIG_FIELDS = {"conc-list", "conc-start", "conc-end"} -def is_append_only_allowed_file(filepath: str) -> bool: - """Allow config inputs plus launch scripts whose control flow is AI-reviewed.""" - if filepath in APPEND_ONLY_ALWAYS_ALLOWED_FILES: - return True - path = Path(filepath) - if path.suffix != ".sh": - return False - if path.parts and path.parts[0] == "benchmarks": - return True - return path.parent == Path("runners") and path.name.startswith("launch_") - - def _freeze_config_value(value): """Convert JSON-shaped config values into deterministic hashable values.""" if isinstance(value, dict): @@ -161,17 +148,6 @@ def get_config_keys_from_master( return list(resolved_keys) -def get_changed_files(base_ref: str, head_ref: str) -> set[str]: - """Return repository-relative paths changed by the requested sweep diff.""" - result = subprocess.run( - ["git", "diff", "--name-only", base_ref, head_ref], - capture_output=True, - text=True, - check=True, - ) - return {line for line in result.stdout.splitlines() if line} - - @contextmanager def config_files_at_ref(ref: str): """Materialize the master configs from ``ref`` for matrix generation.""" @@ -270,25 +246,11 @@ def append_only_delta(base_entries: list[dict], head_entries: list[dict]) -> lis def validate_append_only_scope( - base_ref: str, - head_ref: str, base_master: dict, head_master: dict, selected_config_scenarios: dict[str, set[str]], ) -> None: - """Reject code changes and unrelated config edits in an append-only PR.""" - unexpected_files = { - filepath - for filepath in get_changed_files(base_ref, head_ref) - if not is_append_only_allowed_file(filepath) - } - if unexpected_files: - raise ValueError( - "append-only PRs may change only perf-changelog.yaml, master configs, " - "and benchmark/launch shell scripts; unexpected changes: " - f"{sorted(unexpected_files)}" - ) - + """Reject config edits that would mutate an existing generated curve.""" selected_configs = selected_config_scenarios.keys() all_keys = base_master.keys() | head_master.keys() unrelated_changes = [ @@ -446,8 +408,6 @@ def main(): f"revision; missing: {sorted(missing_from_base)}" ) validate_append_only_scope( - args.base_ref, - args.head_ref, base_master, master_config, selected_config_scenarios, diff --git a/utils/test_process_changelog.py b/utils/test_process_changelog.py index b6243935fd..bc9becf9d6 100644 --- a/utils/test_process_changelog.py +++ b/utils/test_process_changelog.py @@ -155,7 +155,7 @@ def test_append_only_delta_rejects_image_changes(): raise AssertionError("image mutation should reject append-only mode") -def test_append_only_scope_rejects_non_concurrency_recipe_changes(monkeypatch): +def test_append_only_scope_rejects_non_concurrency_recipe_changes(): base = { "test-config": { "image": "vllm/vllm-openai:v0.16.0", @@ -178,11 +178,9 @@ def test_append_only_scope_rejects_non_concurrency_recipe_changes(monkeypatch): }, } } - monkeypatch.setattr(process_changelog, "get_changed_files", lambda *_: set()) - try: process_changelog.validate_append_only_scope( - "base", "head", base, head, {"test-config": {"agentic-coding"}} + base, head, {"test-config": {"agentic-coding"}} ) except ValueError as error: assert "duration" in str(error) @@ -190,7 +188,7 @@ def test_append_only_scope_rejects_non_concurrency_recipe_changes(monkeypatch): raise AssertionError("recipe mutation should reject append-only mode") -def test_append_only_scope_allows_range_to_list_expansion(monkeypatch): +def test_append_only_scope_allows_range_to_list_expansion(): base = { "test-config": { "image": "vllm/vllm-openai:v0.16.0", @@ -211,66 +209,11 @@ def test_append_only_scope_allows_range_to_list_expansion(monkeypatch): }, } } - monkeypatch.setattr(process_changelog, "get_changed_files", lambda *_: set()) - - process_changelog.validate_append_only_scope( - "base", "head", base, head, {"test-config": {"fixed-seq-len"}} - ) - - -def test_append_only_scope_allows_benchmark_and_launch_shell_scripts(monkeypatch): - master = { - "test-config": { - "image": "vllm/vllm-openai:v0.16.0", - "scenarios": { - "fixed-seq-len": { - "search-space": [{"tp": 8, "conc-list": [4, 8]}], - } - }, - } - } - monkeypatch.setattr( - process_changelog, - "get_changed_files", - lambda *_: { - "perf-changelog.yaml", - "configs/nvidia-master.yaml", - "benchmarks/single_node/test.sh", - "benchmarks/multi_node/helpers/server.sh", - "runners/launch_b300-nv.sh", - }, - ) - process_changelog.validate_append_only_scope( - "base", "head", master, master, {"test-config": {"fixed-seq-len"}} + base, head, {"test-config": {"fixed-seq-len"}} ) -def test_append_only_scope_rejects_shared_or_non_shell_logic(monkeypatch): - master = { - "test-config": { - "image": "vllm/vllm-openai:v0.16.0", - "scenarios": {"fixed-seq-len": {}}, - } - } - monkeypatch.setattr( - process_changelog, - "get_changed_files", - lambda *_: {"runners/slurm_utils.sh", "benchmarks/single_node/helper.py"}, - ) - - try: - process_changelog.validate_append_only_scope( - "base", "head", master, master, {"test-config": {"fixed-seq-len"}} - ) - except ValueError as error: - assert "unexpected changes" in str(error) - assert "runners/slurm_utils.sh" in str(error) - assert "benchmarks/single_node/helper.py" in str(error) - else: - raise AssertionError("shared and non-shell logic should remain out of scope") - - def test_append_only_main_runs_only_added_points_and_skips_evals( monkeypatch, capsys, @@ -288,7 +231,6 @@ def test_append_only_main_runs_only_added_points_and_skips_evals( commands = [] monkeypatch.setattr(process_changelog, "get_added_lines", lambda *_: added_yaml) - monkeypatch.setattr(process_changelog, "get_changed_files", lambda *_: set()) monkeypatch.setattr( process_changelog, "config_files_at_ref", From 9893aac7d0fb42df4cc3b85062ddb7b9959c94bb Mon Sep 17 00:00:00 2001 From: Cam Quilici Date: Fri, 14 Aug 2026 18:02:12 -0500 Subject: [PATCH 4/4] feat(ci): support additive append-only recipes --- .github/codeowner-signoff-verify-prompt.md | 22 +- .../workflows/benchmark-multinode-tmpl.yml | 13 +- .github/workflows/benchmark-tmpl.yml | 15 +- .github/workflows/claude-pr-review.yml | 4 +- .github/workflows/claude.yml | 2 +- .github/workflows/e2e-tests.yml | 8 + .github/workflows/run-sweep.yml | 9 + CONTRIBUTING.md | 36 ++- benchmarks/benchmark_lib.sh | 1 + docs/PR_REVIEW_CHECKLIST.md | 2 +- .../aggregation/process_agentic_result.py | 1 + .../test_process_agentic_result.py | 2 + utils/matrix_logic/validation.py | 25 +- utils/process_changelog.py | 213 +++++++++----- utils/process_result.py | 2 + utils/test_process_changelog.py | 268 +++++++++++++++++- utils/test_process_result.py | 2 + 17 files changed, 516 insertions(+), 109 deletions(-) diff --git a/.github/codeowner-signoff-verify-prompt.md b/.github/codeowner-signoff-verify-prompt.md index ccadf40299..d9f910f3ef 100644 --- a/.github/codeowner-signoff-verify-prompt.md +++ b/.github/codeowner-signoff-verify-prompt.md @@ -369,21 +369,25 @@ APPLICABILITY: this check applies when any new `perf-changelog.yaml` entry conta is never a reason to fail; determine whether each benchmark-affecting change is behaviorally isolated to the appended points. - For every selected config, compare the generated matrix at the PR base and head. - Every base curve and concurrency must remain present, and every non-concurrency - field must be identical. In particular, require the exact same image, model, - framework, runner, topology, server arguments, scenario, duration, and offload - settings. The generated matrix may differ only by one or more newly added - concurrency values on existing curves. + Treat the complete base matrix as an immutable subset of the head matrix: every + existing point must remain present with the same image and complete recipe. The + head may add concurrency points or entirely new recipe variants, such as a new + tensor-parallelism value, inside the selected existing config/scenario. Every + addition must retain the target visual curve's one non-null image. - Trace the selected config and generated runtime values through every affected file into the changed behavior. The behavior must be reachable only for the corresponding newly appended points. PASS when the controlling condition is uniquely satisfied by those points. FAIL an unguarded/shared setup change, a condition also satisfied by an existing point, or any case where exclusivity cannot be proven from the diff. -- FAIL if an existing concurrency is rerun or removed, a new recipe/curve is created, - or a non-concurrency config setting changes. Other benchmark-affecting changes are - permitted only under the behavioral-isolation rule above. +- FAIL if any existing point or recipe is rerun, removed, or modified. New configs and + scenarios are out of scope, but new generated recipe variants inside the selected + existing config/scenario are allowed. Other benchmark-affecting changes are permitted + only under the behavioral-isolation rule above. - Treat the repository's append-only matrix validation as supporting evidence, but - verify the diff independently and name the offending field/path when failing. + verify the diff independently and name the offending field/path when failing. Each + config revision is rendered with its own generator, validation code, and runner + metadata, but this does not mechanically prove that launcher or benchmark-script + changes are isolated at runtime. ## Verdict and output Decide PASS only if Checks 0-12 ALL pass. A check reported as `N/A` counts as a pass. diff --git a/.github/workflows/benchmark-multinode-tmpl.yml b/.github/workflows/benchmark-multinode-tmpl.yml index 0a666a6e90..05532b1dd0 100644 --- a/.github/workflows/benchmark-multinode-tmpl.yml +++ b/.github/workflows/benchmark-multinode-tmpl.yml @@ -37,6 +37,11 @@ on: exp-name: required: true type: string + recipe-fingerprint: + description: "Deterministic generated-recipe identity" + required: false + type: string + default: '' isl: required: true type: string @@ -217,6 +222,7 @@ env: # once; sbatch/srun inherit this env so the token reaches the workers. HF_TOKEN: ${{ secrets.INFERENCEX_OFFICIAL_RO_HF_TOKEN }} EXP_NAME: ${{ inputs.exp-name }} + RECIPE_FINGERPRINT: ${{ inputs.recipe-fingerprint }} IMAGE: ${{ inputs.image }} MODEL_PREFIX: ${{ inputs.model-prefix }} MODEL: ${{ inputs.model }} @@ -336,10 +342,13 @@ jobs: env: RUNNER_NAME: ${{ runner.name }} RUNNER_TYPE: ${{ inputs.runner }} - # Hash uniquely on all prefill/decode parallelism fields, worker counts, serving mode, concurrency, and runner. - RESULT_FILENAME: ${{ env.EXP_NAME }}_${{ env.PRECISION }}_${{ env.FRAMEWORK }}_prefill-tp${{ env.PREFILL_TP }}-pp${{ env.PREFILL_PP_SIZE }}-dcp${{ env.PREFILL_DCP_SIZE }}-pcp${{ env.PREFILL_PCP_SIZE }}-ep${{ env.PREFILL_EP }}-dp${{ env.PREFILL_DP_ATTN }}-nw${{ env.PREFILL_NUM_WORKERS }}_decode-tp${{ env.DECODE_TP }}-pp${{ env.DECODE_PP_SIZE }}-dcp${{ env.DECODE_DCP_SIZE }}-pcp${{ env.DECODE_PCP_SIZE }}-ep${{ env.DECODE_EP }}-dp${{ env.DECODE_DP_ATTN }}-nw${{ env.DECODE_NUM_WORKERS }}_disagg-${{ env.DISAGG }}_spec-${{ env.SPEC_DECODING }}_conc${{ join(fromJson(inputs.conc-list), 'x') }}_${{ runner.name }} + RESULT_FILENAME_BASE: ${{ env.EXP_NAME }}_${{ env.PRECISION }}_${{ env.FRAMEWORK }}_prefill-tp${{ env.PREFILL_TP }}-pp${{ env.PREFILL_PP_SIZE }}-dcp${{ env.PREFILL_DCP_SIZE }}-pcp${{ env.PREFILL_PCP_SIZE }}-ep${{ env.PREFILL_EP }}-dp${{ env.PREFILL_DP_ATTN }}-nw${{ env.PREFILL_NUM_WORKERS }}_decode-tp${{ env.DECODE_TP }}-pp${{ env.DECODE_PP_SIZE }}-dcp${{ env.DECODE_DCP_SIZE }}-pcp${{ env.DECODE_PCP_SIZE }}-ep${{ env.DECODE_EP }}-dp${{ env.DECODE_DP_ATTN }}-nw${{ env.DECODE_NUM_WORKERS }}_disagg-${{ env.DISAGG }}_spec-${{ env.SPEC_DECODING }}_conc${{ join(fromJson(inputs.conc-list), 'x') }}_${{ runner.name }} run: | set -x + export RESULT_FILENAME="$RESULT_FILENAME_BASE" + if [ -n "$RECIPE_FINGERPRINT" ]; then + export RESULT_FILENAME="${RESULT_FILENAME}_recipe-${RECIPE_FINGERPRINT:0:16}" + fi # Export RESULT_FILENAME early so it's available for artifact uploads even if cancelled echo "RESULT_FILENAME=${RESULT_FILENAME}" >> "$GITHUB_ENV" diff --git a/.github/workflows/benchmark-tmpl.yml b/.github/workflows/benchmark-tmpl.yml index 6c4fe50fe5..bb3fcc259b 100644 --- a/.github/workflows/benchmark-tmpl.yml +++ b/.github/workflows/benchmark-tmpl.yml @@ -36,6 +36,11 @@ on: exp-name: required: true type: string + recipe-fingerprint: + description: "Deterministic generated-recipe identity" + required: false + type: string + default: '' isl: required: true type: string @@ -154,6 +159,7 @@ env: HF_TOKEN: ${{ secrets.INFERENCEX_OFFICIAL_RO_HF_TOKEN }} HF_HUB_CACHE: '/mnt/hf_hub_cache/' EXP_NAME: ${{ inputs.exp-name }} + RECIPE_FINGERPRINT: ${{ inputs.recipe-fingerprint }} MODEL: ${{ inputs.model }} MODEL_PREFIX: ${{ inputs.model-prefix }} ISL: ${{ inputs.isl }} @@ -269,17 +275,20 @@ jobs: env: RUNNER_NAME: ${{ runner.name }} RUNNER_TYPE: ${{ inputs.runner }} - # Hash uniquely on {EXP_NAME}_{PRECISION}_{FRAMEWORK}_tp{}-pp{}-dcp{}-pcp{}-ep{}-dpa{}_disagg-{}_spec-{}_conc{}_{runner} - RESULT_FILENAME: ${{ env.EXP_NAME }}_${{ env.PRECISION }}_${{ env.FRAMEWORK }}_tp${{ env.TP }}-pp${{ env.PP_SIZE }}-dcp${{ env.DCP_SIZE }}-pcp${{ env.PCP_SIZE }}-ep${{ env.EP_SIZE }}-dpa${{ env.DP_ATTENTION }}_disagg-${{ env.DISAGG }}_spec-${{ env.SPEC_DECODING }}_conc${{ env.CONC }}_${{ runner.name }} + RESULT_FILENAME_BASE: ${{ env.EXP_NAME }}_${{ env.PRECISION }}_${{ env.FRAMEWORK }}_tp${{ env.TP }}-pp${{ env.PP_SIZE }}-dcp${{ env.DCP_SIZE }}-pcp${{ env.PCP_SIZE }}-ep${{ env.EP_SIZE }}-dpa${{ env.DP_ATTENTION }}_disagg-${{ env.DISAGG }}_spec-${{ env.SPEC_DECODING }}_conc${{ env.CONC }}_${{ runner.name }} # Suppress per-job eval markdown from being appended to the step summary. # We'll publish a single combined eval table in the collection job instead. GITHUB_STEP_SUMMARY: '' run: | + export RESULT_FILENAME="$RESULT_FILENAME_BASE" + if [ -n "$RECIPE_FINGERPRINT" ]; then + export RESULT_FILENAME="${RESULT_FILENAME}_recipe-${RECIPE_FINGERPRINT:0:16}" + fi export GPU_COUNT=$((TP * PP_SIZE * PCP_SIZE)) echo "GPU_COUNT=${GPU_COUNT}" >> "$GITHUB_ENV" # Export RESULT_FILENAME early so it's available for artifact uploads even if cancelled - echo "RESULT_FILENAME=${RESULT_FILENAME}" >> $GITHUB_ENV + echo "RESULT_FILENAME=${RESULT_FILENAME}" >> "$GITHUB_ENV" bash ./runners/launch_${RUNNER_NAME%%_*}.sh diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index cf97c1c00d..f74a2d9f3b 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -138,9 +138,9 @@ jobs: When a new `perf-changelog.yaml` entry contains `append-only: true`, verify the complete PR diff before approving it: - Do not use a file allowlist. Supporting code, benchmark scripts, launchers, helpers, and other files may change. Inspect the complete diff and judge whether each benchmark-affecting change is behaviorally isolated to the appended points. - Every newly added changelog entry must contain `append-only: true`; append-only and regular entries may not be mixed. - - Selected existing curves may only gain concurrency values. The container image and every other generated recipe property (model, framework, topology, arguments, sequence lengths, duration, scenario, and so on) must remain unchanged. + - Treat the generated base matrix as an immutable subset of the generated head matrix. Every existing point must remain present with the same image and complete recipe. Additions may include new concurrency values or entirely new recipe variants (for example, a new tensor-parallelism value) inside the selected existing config/scenario, but they must retain the existing visual curve's single non-null image. - Benchmark or launch logic may change only when every changed behavior is on a control-flow path uniquely gated to the corresponding newly appended points. Trace the selected config and generated runtime values through every affected file into the condition. Confirm the path cannot be reached by any existing point. Unguarded/shared setup changes, or a branch also used by an existing concurrency/config/scenario, are blocking. - - Existing concurrency values, curves, configs, and scenarios may not be removed, replaced, or newly introduced. + - No existing point, recipe variant, config, or scenario may be removed or replaced. New configs and scenarios are out of scope for append-only mode; new generated variants inside the selected existing config/scenario are allowed. - Eval modifiers (`evals-only`, `all-evals`, `eval-min-prefill-ep`) are not allowed. If any condition fails, report a 🔴 **BLOCKING** issue. Never reject a change merely because of its file path; reject it when its benchmark effect is not exclusive to the appended points or the exclusivity cannot be proven from the diff. diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 405ec7be07..9f3798aaa9 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -223,7 +223,7 @@ jobs: See `docs/configuration-procedures.md` → "Update an image" and "Append the changelog safely" for entry format and rules. Required whenever you change image tags, env vars, or perf-affecting params in `configs/*-master.yaml` or `benchmarks/*.sh`. Use `XXX` as the PR-link placeholder until the PR exists. - If an entry uses `append-only: true`, allow only added concurrency values on existing curves. Do not enforce a file allowlist: supporting code, benchmark scripts, launchers, helpers, and other files may change when their benchmark effect is exclusive to the corresponding newly appended points. The image and generated non-concurrency recipe fields must remain identical, no existing point/curve/config/scenario may be removed, all added changelog entries must be append-only, and eval modifiers are forbidden. Trace behavior through the complete diff; unguarded changes or paths reachable by an existing point are blocking. + If an entry uses `append-only: true`, require the generated base matrix to remain an immutable subset of the generated head matrix. New concurrency values or new recipe variants may be added inside a selected existing config/scenario, but no existing generated point may be removed or modified, and every addition must retain the target visual curve's single non-null image. Do not enforce a file allowlist: supporting code, benchmark scripts, launchers, helpers, and other files may change when their benchmark effect is exclusive to the corresponding newly appended points. All added changelog entries must be append-only, and eval modifiers are forbidden. Trace behavior through the complete diff; unguarded changes or paths reachable by an existing point are blocking. ## Spawning Additional Workers: You CAN spawn additional Claude workers by commenting "@claude" with a specific task. diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index c973df94e6..d4318a39fa 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -298,6 +298,7 @@ jobs: router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }} kv-p2p-transfer: ${{ matrix.config['kv-p2p-transfer'] || '' }} exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} conc-list: ${{ toJson(matrix.config.conc) }} spec-decoding: ${{ matrix.config.spec-decoding }} disagg: ${{ matrix.config.disagg }} @@ -351,6 +352,7 @@ jobs: router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }} kv-p2p-transfer: ${{ matrix.config['kv-p2p-transfer'] || '' }} exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} conc-list: ${{ toJson(matrix.config.conc) }} spec-decoding: ${{ matrix.config.spec-decoding }} disagg: ${{ matrix.config.disagg }} @@ -391,6 +393,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} runner: ${{ matrix.config.runner }} priority: ${{ matrix.config.priority }} queue-token: ${{ matrix.config['queue-token'] }} @@ -435,6 +438,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} runner: ${{ matrix.config.runner }} priority: ${{ matrix.config.priority }} queue-token: ${{ matrix.config['queue-token'] }} @@ -476,6 +480,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: '0' osl: '0' max-model-len: '0' @@ -532,6 +537,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: '0' osl: '0' max-model-len: '0' @@ -591,6 +597,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: ${{ matrix.config.isl }} osl: ${{ matrix.config.osl }} max-model-len: ${{ matrix.config.max-model-len }} @@ -629,6 +636,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: ${{ matrix.config.isl }} osl: ${{ matrix.config.osl }} max-model-len: ${{ matrix.config.max-model-len }} diff --git a/.github/workflows/run-sweep.yml b/.github/workflows/run-sweep.yml index b8e95f0806..c01093cb8e 100644 --- a/.github/workflows/run-sweep.yml +++ b/.github/workflows/run-sweep.yml @@ -411,6 +411,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: ${{ matrix.config.isl }} osl: ${{ matrix.config.osl }} max-model-len: ${{ matrix.config.max-model-len }} @@ -469,6 +470,7 @@ jobs: router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }} kv-p2p-transfer: ${{ matrix.config['kv-p2p-transfer'] || '' }} exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} conc-list: ${{ toJson(matrix.config.conc) }} spec-decoding: ${{ matrix.config.spec-decoding }} disagg: ${{ matrix.config.disagg }} @@ -533,6 +535,7 @@ jobs: secrets: inherit with: &single-node-inputs exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: ${{ matrix.config.isl }} osl: ${{ matrix.config.osl }} max-model-len: ${{ matrix.config.max-model-len }} @@ -597,6 +600,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} runner: ${{ matrix.config.runner }} priority: ${{ matrix.config.priority }} queue-token: ${{ matrix.config['queue-token'] }} @@ -648,6 +652,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: '0' osl: '0' max-model-len: '0' @@ -713,6 +718,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: ${{ matrix.config.isl }} osl: ${{ matrix.config.osl }} max-model-len: ${{ matrix.config.max-model-len }} @@ -762,6 +768,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} runner: ${{ matrix.config.runner }} priority: ${{ matrix.config.priority }} queue-token: ${{ matrix.config['queue-token'] }} @@ -814,6 +821,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: ${{ matrix.config.isl }} osl: ${{ matrix.config.osl }} max-model-len: ${{ matrix.config.max-model-len }} @@ -879,6 +887,7 @@ jobs: secrets: inherit with: exp-name: ${{ matrix.config.exp-name }} + recipe-fingerprint: ${{ matrix.config['recipe-fingerprint'] || '' }} isl: '0' osl: '0' max-model-len: '0' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 45f0280a4b..fb6241781a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,28 +46,38 @@ A full benchmark sweep is expensive GPU time, and the runners are shared by ever ## Adding points to the latest curve with `append-only` -When a recipe and image are unchanged and a PR only adds concurrency values to an -existing curve, mark every new changelog entry with `append-only: true`. Sweep setup -compares the generated matrices at the base and head revisions, runs only the newly -added points, and emits metadata that lets InferenceX-app extend the most recent -matching curve instead of presenting the partial run as a separate curve. +When a PR only adds generated points to an existing curve, mark every new changelog +entry with `append-only: true`. Additions may introduce new concurrency values or new +recipe variants, such as another tensor-parallelism value. Sweep setup compares the +generated matrices at the base and head revisions, runs only the newly added points, +and emits metadata that lets InferenceX-app extend the most recent matching curve +instead of presenting the partial run as a separate curve. This mode is intentionally narrow, but it is not based on a file allowlist. Supporting code, benchmark scripts, launchers, and other files may change when their behavioral effect is exclusive to the newly appended points named by the changelog. No changed -benchmark path may execute for or alter an existing point. Every selected config must -already exist, its prior concurrency points must remain present, and all generated -fields other than concurrency must be identical. Image changes or shared logic changes -that can affect existing points are rejected. Append-only entries cannot be mixed with -regular entries or eval-selection modifiers in the same sweep. The matrix validator -enforces deterministic generated-config invariants; the human and AI reviewers must -inspect the complete diff and verify behavioral isolation. +benchmark path may execute for or alter an existing point. Every selected config and +scenario must already exist, and every point generated at the base revision must +remain present with the same recipe. The head may contain any additional generated +recipes or points inside that scope, including new topology or other recipe dimensions; +the sweep schedules the generated set difference. Additions must use the same non-null +image and belong to an existing dashboard visual series. Each generated recipe carries +a deterministic fingerprint so two distinct recipes at the same concurrency remain +distinct database points without splitting the visual curve. Removing or modifying an +existing point, or changing shared logic that can affect one, is rejected. Append-only +entries cannot be mixed with regular entries or eval-selection modifiers in the same +sweep. The matrix validator enforces the additive generated-matrix invariant; the human +and AI reviewers must inspect the complete diff and verify behavioral isolation. The +mechanical comparison renders each config revision with its own generator, validation +code, and runner metadata. Launcher and benchmark-script changes still rely on +complete-diff review because matrix equality alone cannot prove their runtime +control-flow isolation. ```yaml - config-keys: - dsv4-fp4-b300-vllm-mtp description: - - "Add concurrency 192 to the existing curve" + - "Add TP8 at concurrency 12 and 16 to the existing curve" pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX append-only: true ``` diff --git a/benchmarks/benchmark_lib.sh b/benchmarks/benchmark_lib.sh index 8cc894940f..26bde59696 100644 --- a/benchmarks/benchmark_lib.sh +++ b/benchmarks/benchmark_lib.sh @@ -1220,6 +1220,7 @@ _write_lm_eval_meta_json() { "framework": "${fw:-unknown}", "precision": "${prec:-unknown}", "spec_decoding": "${SPEC_DECODING:-}", + "recipe_fingerprint": "${RECIPE_FINGERPRINT:-}", "tp": ${TP:-1}, "pp": ${PP_SIZE:-1}, "dcp_size": ${DCP_SIZE:-1}, diff --git a/docs/PR_REVIEW_CHECKLIST.md b/docs/PR_REVIEW_CHECKLIST.md index fd6cfeace8..59f923c815 100644 --- a/docs/PR_REVIEW_CHECKLIST.md +++ b/docs/PR_REVIEW_CHECKLIST.md @@ -28,7 +28,7 @@ As a PR reviewer and CODEOWNER, I have reviewed this and have: - [ ] Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official [vLLM recipes](https://recipes.vllm.ai/) and/or the [SGLang cookbook](https://docs.sglang.io/cookbook/intro): - [ ] I linked the corresponding upstream PR in the [vLLM recipe repo](https://github.com/vllm-project/recipes) or [SGLang repo](https://github.com/sgl-project/sglang/tree/main/docs_new) and verified that it is **MERGED** before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below. - [ ] Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at [docs/waiver/](https://github.com/SemiAnalysisAI/InferenceX/tree/main/docs/waiver)`.md` — named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. -- [ ] If this PR uses `append-only: true`, verified that it only adds previously unmeasured concurrency points to an existing curve: the image and generated non-concurrency recipe/topology settings are unchanged, no prior point is removed or rerun, and every benchmark-affecting change in the complete diff can affect only the corresponding newly appended points (never an existing point), regardless of which file contains it. +- [ ] If this PR uses `append-only: true`, verified that it only adds generated points or recipe variants inside a selected existing config/scenario and existing same-image visual curve: every previously generated point remains present with the same recipe, no prior point is removed or rerun, and every benchmark-affecting change in the complete diff can affect only the corresponding newly appended points (never an existing point), regardless of which file contains it. - [ ] If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below. ### Additional detail section: diff --git a/utils/agentic/aggregation/process_agentic_result.py b/utils/agentic/aggregation/process_agentic_result.py index 5d13fa0f1b..c0d8d70ce1 100644 --- a/utils/agentic/aggregation/process_agentic_result.py +++ b/utils/agentic/aggregation/process_agentic_result.py @@ -216,6 +216,7 @@ def build_agg( "hw": os.environ.get("RUNNER_TYPE", ""), "conc": int(os.environ.get("CONC", "0")), "image": os.environ.get("IMAGE", ""), + "recipe_fingerprint": os.environ.get("RECIPE_FINGERPRINT", ""), "model": os.environ.get("MODEL", ""), "infmax_model_prefix": os.environ.get("MODEL_PREFIX", ""), "framework": framework, diff --git a/utils/agentic/aggregation/test_process_agentic_result.py b/utils/agentic/aggregation/test_process_agentic_result.py index 7f5b0395bf..a567e9aced 100644 --- a/utils/agentic/aggregation/test_process_agentic_result.py +++ b/utils/agentic/aggregation/test_process_agentic_result.py @@ -343,6 +343,7 @@ def _run_processor( "KV_OFFLOADING": "none", "RUNNER_TYPE": "b200-x4", "IMAGE": "test/image:0.1", + "RECIPE_FINGERPRINT": "b" * 64, "SPEC_DECODING": "none", "DISAGG": "false", "IS_MULTINODE": "false", @@ -373,6 +374,7 @@ def test_processor_emits_nested_request_and_server_metrics(tmp_path: Path): result_dir = _write_fixture(tmp_path) output_dir = tmp_path / "out" agg = _run_processor(result_dir, output_dir) + assert agg["recipe_fingerprint"] == "b" * 64 missing = AGG_TOP_LEVEL_KEYS - set(agg.keys()) assert not missing, f"agg JSON missing top-level keys: {sorted(missing)}" assert not (_flat_request_keys(result_dir) & set(agg.keys())) diff --git a/utils/matrix_logic/validation.py b/utils/matrix_logic/validation.py index 6aa2bd5a7a..eceed9a456 100644 --- a/utils/matrix_logic/validation.py +++ b/utils/matrix_logic/validation.py @@ -78,6 +78,7 @@ class Fields(Enum): CONC = 'conc' MAX_MODEL_LEN = 'max-model-len' EXP_NAME = 'exp-name' + RECIPE_FINGERPRINT = 'recipe-fingerprint' DISAGG = 'disagg' SCENARIO_TYPE = 'scenario-type' @@ -175,6 +176,11 @@ class SingleNodeMatrixEntry(BaseModel): run_eval: bool = Field(alias=Fields.RUN_EVAL.value) eval_only: bool = Field(alias=Fields.EVAL_ONLY.value, default=False) router: Optional[ComponentMetadata] = None + recipe_fingerprint: Optional[str] = Field( + default=None, + alias=Fields.RECIPE_FINGERPRINT.value, + pattern=r"^[0-9a-f]{64}$", + ) @model_validator(mode='after') def validate_single_node_topology(self): @@ -245,6 +251,11 @@ class MultiNodeMatrixEntry(BaseModel): kv_p2p_transfer: Optional[str] = Field( default=None, alias=Fields.KV_P2P_TRANSFER.value, min_length=1 ) + recipe_fingerprint: Optional[str] = Field( + default=None, + alias=Fields.RECIPE_FINGERPRINT.value, + pattern=r"^[0-9a-f]{64}$", + ) @model_validator(mode='after') def validate_worker_hardware_pair(self): @@ -295,6 +306,11 @@ class SingleNodeAgenticMatrixEntry(BaseModel): # omit them, and exclude_none keeps them out of dumped benchmark output. run_eval: Optional[bool] = Field(default=None, alias=Fields.RUN_EVAL.value) eval_only: Optional[bool] = Field(default=None, alias=Fields.EVAL_ONLY.value) + recipe_fingerprint: Optional[str] = Field( + default=None, + alias=Fields.RECIPE_FINGERPRINT.value, + pattern=r"^[0-9a-f]{64}$", + ) @model_validator(mode='after') def validate_kv_offload_fields(self): @@ -341,6 +357,11 @@ class MultiNodeAgenticMatrixEntry(BaseModel): run_eval: Optional[bool] = Field(default=None, alias=Fields.RUN_EVAL.value) eval_only: Optional[bool] = Field(default=None, alias=Fields.EVAL_ONLY.value) eval_conc: Optional[int] = Field(default=None, alias=Fields.EVAL_CONC.value) + recipe_fingerprint: Optional[str] = Field( + default=None, + alias=Fields.RECIPE_FINGERPRINT.value, + pattern=r"^[0-9a-f]{64}$", + ) @model_validator(mode='after') def validate_worker_hardware_pair(self): @@ -879,8 +900,8 @@ class ChangelogEntry(BaseModel): alias="append-only", default=False, description=( - "Run only concurrency points added to an otherwise unchanged existing " - "curve, then append them to that curve during dashboard ingestion" + "Run only generated points or recipe variants added while preserving " + "every existing generated point, then append them to the latest curve" ), ) eval_min_prefill_ep: Optional[int] = Field( diff --git a/utils/process_changelog.py b/utils/process_changelog.py index 04f1edc1f7..6149d8fba7 100644 --- a/utils/process_changelog.py +++ b/utils/process_changelog.py @@ -1,11 +1,13 @@ import argparse import copy +import hashlib import json import re import subprocess import tempfile from collections import defaultdict from contextlib import contextmanager +from dataclasses import dataclass from pathlib import Path import yaml @@ -18,7 +20,13 @@ ) SCENARIO_TYPES = ("fixed-seq-len", "agentic-coding") -CONCURRENCY_CONFIG_FIELDS = {"conc-list", "conc-start", "conc-end"} + + +@dataclass(frozen=True) +class GenerationInputs: + config_files: list[str] + generator_script: str + runner_config: str def _freeze_config_value(value): @@ -149,21 +157,53 @@ def get_config_keys_from_master( @contextmanager -def config_files_at_ref(ref: str): - """Materialize the master configs from ``ref`` for matrix generation.""" +def generation_inputs_at_ref(ref: str): + """Materialize config and generator inputs from one repository revision.""" with tempfile.TemporaryDirectory(prefix="inferencex-append-only-") as temp_dir: - paths = [] - for config_file in MASTER_CONFIGS: + files_result = subprocess.run( + [ + "git", + "ls-tree", + "-r", + "--name-only", + ref, + "--", + "utils/matrix_logic", + *MASTER_CONFIGS, + "configs/runners.yaml", + ], + capture_output=True, + check=True, + text=True, + ) + repo_paths = files_result.stdout.splitlines() + required_paths = { + *MASTER_CONFIGS, + "configs/runners.yaml", + GENERATE_SWEEPS_PY_SCRIPT, + } + missing_paths = required_paths - set(repo_paths) + if missing_paths: + raise ValueError( + f"append-only base revision is missing generation inputs: " + f"{sorted(missing_paths)}" + ) + + for repo_path in repo_paths: result = subprocess.run( - ["git", "show", f"{ref}:{config_file}"], + ["git", "show", f"{ref}:{repo_path}"], capture_output=True, check=True, ) - destination = Path(temp_dir) / config_file + destination = Path(temp_dir) / repo_path destination.parent.mkdir(parents=True, exist_ok=True) destination.write_bytes(result.stdout) - paths.append(str(destination)) - yield paths + + yield GenerationInputs( + config_files=[str(Path(temp_dir) / path) for path in MASTER_CONFIGS], + generator_script=str(Path(temp_dir) / GENERATE_SWEEPS_PY_SCRIPT), + runner_config=str(Path(temp_dir) / "configs/runners.yaml"), + ) def _matrix_curve_key(entry: dict) -> tuple: @@ -172,11 +212,51 @@ def _matrix_curve_key(entry: dict) -> tuple: sorted( (key, _freeze_config_value(value)) for key, value in entry.items() - if key not in {"conc", "exp-name"} + if key not in {"conc", "exp-name", "recipe-fingerprint"} ) ) +def recipe_fingerprint(entry: dict) -> str: + """Hash the generated recipe independently of point-level concurrency/name.""" + recipe = { + key: value + for key, value in entry.items() + if key not in {"conc", "exp-name", "recipe-fingerprint"} + } + canonical = json.dumps( + recipe, + sort_keys=True, + separators=(",", ":"), + ensure_ascii=True, + ) + return hashlib.sha256(canonical.encode("utf-8")).hexdigest() + + +def _matrix_visual_series_key(entry: dict) -> tuple: + """Identify the App curve that an appended recipe must already belong to.""" + is_agentic = entry.get("scenario-type") == "agentic-coding" + kv_offloading = entry.get("kv-offloading", "none") + offload_mode = "off" if kv_offloading in (None, "", "none") else "on" + prefill = entry.get("prefill") or {} + decode = entry.get("decode") or {} + return ( + entry.get("model"), + entry.get("model-prefix"), + entry.get("precision"), + entry.get("framework"), + entry.get("runner"), + bool(entry.get("disagg", False)), + "agentic_traces" if is_agentic else "single_turn", + None if is_agentic else entry.get("isl"), + None if is_agentic else entry.get("osl"), + offload_mode, + "" if is_agentic else entry.get("spec-decoding", "none"), + prefill.get("hardware"), + decode.get("hardware"), + ) + + def _matrix_concurrencies(entry: dict) -> tuple[int, ...]: conc = entry.get("conc") if isinstance(conc, int): @@ -187,11 +267,12 @@ def _matrix_concurrencies(entry: dict) -> tuple[int, ...]: def append_only_delta(base_entries: list[dict], head_entries: list[dict]) -> list[dict]: - """Return only newly added points, rejecting any existing-curve mutation. + """Return only newly added points, rejecting any existing-point mutation. Generated matrix rows are the runtime contract. Grouping them without ``conc`` - and ``exp-name`` catches image, launcher, topology, arguments, duration, and - scenario changes while allowing only concurrency-set expansion. + and ``exp-name`` lets an existing recipe gain concurrency while also permitting + entirely new recipe variants. Every base recipe and concurrency must remain in + the head unchanged; the returned delta is therefore strictly additive. """ base_groups: dict[tuple, set[int]] = defaultdict(set) head_groups: dict[tuple, set[int]] = defaultdict(set) @@ -204,11 +285,9 @@ def append_only_delta(base_entries: list[dict], head_entries: list[dict]) -> lis raise ValueError("append-only requires an existing curve in the base revision") removed_curves = base_groups.keys() - head_groups.keys() - new_curves = head_groups.keys() - base_groups.keys() - if removed_curves or new_curves: + if removed_curves: raise ValueError( - "append-only may not add, remove, or modify curve logic; only concurrency " - "points may be added" + "append-only may not remove or modify existing generated recipes" ) for key, base_concurrencies in base_groups.items(): @@ -223,7 +302,7 @@ def append_only_delta(base_entries: list[dict], head_entries: list[dict]) -> lis emitted_concurrencies: dict[tuple, set[int]] = defaultdict(set) for entry in head_entries: key = _matrix_curve_key(entry) - added = head_groups[key] - base_groups[key] + added = head_groups[key] - base_groups.get(key, set()) conc = entry.get("conc") if isinstance(conc, int): if conc in added and conc not in emitted_concurrencies[key]: @@ -241,7 +320,22 @@ def append_only_delta(base_entries: list[dict], head_entries: list[dict]) -> lis delta.append(delta_entry) if not delta: - raise ValueError("append-only did not add any concurrency points") + raise ValueError("append-only did not add any generated points") + + base_images_by_series: dict[tuple, set[str | None]] = defaultdict(set) + for entry in base_entries: + base_images_by_series[_matrix_visual_series_key(entry)].add( + entry.get("image") + ) + for entry in delta: + series_key = _matrix_visual_series_key(entry) + base_images = base_images_by_series.get(series_key, set()) + image = entry.get("image") + if image is None or base_images != {image}: + raise ValueError( + "append-only additions must belong to an existing visual curve " + "with one unchanged non-null image" + ) return delta @@ -250,7 +344,13 @@ def validate_append_only_scope( head_master: dict, selected_config_scenarios: dict[str, set[str]], ) -> None: - """Reject config edits that would mutate an existing generated curve.""" + """Reject edits outside selected existing configs and scenarios. + + Changes inside an explicitly selected scenario are checked semantically by + ``append_only_delta`` after generating the complete base and head matrices. + This permits arbitrary additive recipe variants while ensuring every existing + generated point remains unchanged and present. + """ selected_configs = selected_config_scenarios.keys() all_keys = base_master.keys() | head_master.keys() unrelated_changes = [ @@ -267,22 +367,26 @@ def validate_append_only_scope( for config, allowed_scenarios in selected_config_scenarios.items(): base_config = base_master[config] head_config = head_master[config] - if base_config.keys() != head_config.keys(): - raise ValueError( - f"append-only changed top-level fields for config {config!r}" - ) - for field in base_config.keys() - {"scenarios"}: - if base_config[field] != head_config[field]: - raise ValueError( - f"append-only changed non-scenario field {field!r} in {config!r}" - ) - base_scenarios = base_config.get("scenarios", {}) head_scenarios = head_config.get("scenarios", {}) if base_scenarios.keys() != head_scenarios.keys(): raise ValueError( f"append-only added or removed a scenario in config {config!r}" ) + + unselected_scenarios = base_scenarios.keys() - allowed_scenarios + base_top_level = { + key: value for key, value in base_config.items() if key != "scenarios" + } + head_top_level = { + key: value for key, value in head_config.items() if key != "scenarios" + } + if unselected_scenarios and base_top_level != head_top_level: + raise ValueError( + "append-only changed config-wide fields that can affect scenarios " + f"outside its changelog scope: {config!r}" + ) + for scenario in base_scenarios: if scenario not in allowed_scenarios: if base_scenarios[scenario] != head_scenarios[scenario]: @@ -291,33 +395,6 @@ def validate_append_only_scope( f"{config!r} / {scenario!r}" ) continue - _validate_concurrency_only_structure( - base_scenarios[scenario], - head_scenarios[scenario], - f"{config}.{scenario}", - ) - - -def _validate_concurrency_only_structure(base, head, path: str) -> None: - """Require identical config structure except at explicit concurrency fields.""" - if isinstance(base, dict) and isinstance(head, dict): - base_keys = base.keys() - CONCURRENCY_CONFIG_FIELDS - head_keys = head.keys() - CONCURRENCY_CONFIG_FIELDS - if base_keys != head_keys: - raise ValueError(f"append-only changed config structure at {path}") - for key in base_keys: - _validate_concurrency_only_structure(base[key], head[key], f"{path}.{key}") - return - if isinstance(base, list) and isinstance(head, list): - if len(base) != len(head): - raise ValueError(f"append-only changed config structure at {path}") - for index, (base_item, head_item) in enumerate(zip(base, head)): - _validate_concurrency_only_structure( - base_item, head_item, f"{path}[{index}]" - ) - return - if base != head: - raise ValueError(f"append-only changed non-concurrency value at {path}") def main(): @@ -388,12 +465,12 @@ def main(): ) resolved_entries.append((entry, all_configs)) - base_config_context = None - base_config_files = None + base_inputs_context = None + base_inputs = None if has_append_only: - base_config_context = config_files_at_ref(args.base_ref) - base_config_files = base_config_context.__enter__() - base_master = load_config_files(base_config_files) + base_inputs_context = generation_inputs_at_ref(args.base_ref) + base_inputs = base_inputs_context.__enter__() + base_master = load_config_files(base_inputs.config_files) selected_config_scenarios: dict[str, set[str]] = defaultdict(set) for entry, configs in resolved_entries: for config in configs: @@ -450,6 +527,8 @@ def main(): *benchmark_configs, "--config-files", *MASTER_CONFIGS, + "--runner-config", + "configs/runners.yaml", "--no-evals", ] if scenarios != SCENARIO_TYPES: @@ -464,10 +543,13 @@ def main(): head_results = json.loads(result.stdout) if entry.append_only: base_cmd = head_cmd.copy() + base_cmd[1] = base_inputs.generator_script config_files_index = base_cmd.index("--config-files") + 1 base_cmd[ config_files_index:config_files_index + len(MASTER_CONFIGS) - ] = base_config_files + ] = base_inputs.config_files + runner_config_index = base_cmd.index("--runner-config") + 1 + base_cmd[runner_config_index] = base_inputs.runner_config base_result = subprocess.run( base_cmd, capture_output=True, @@ -530,13 +612,14 @@ def main(): ) all_eval_results.extend(entry_eval_results) - if base_config_context is not None: - base_config_context.__exit__(None, None, None) + if base_inputs_context is not None: + base_inputs_context.__exit__(None, None, None) if args.trim_conc: all_benchmark_results = trim_conc(all_benchmark_results) for result in all_benchmark_results: + result["recipe-fingerprint"] = recipe_fingerprint(result) if result.get("scenario-type") == "agentic-coding": if result.get("prefill") is not None: final_results["multi_node"]["agentic"].append(result) diff --git a/utils/process_result.py b/utils/process_result.py index a8bdc8cca6..ec7fa2693c 100644 --- a/utils/process_result.py +++ b/utils/process_result.py @@ -123,6 +123,7 @@ def record_power_internal_error( isl = base_env['ISL'] osl = base_env['OSL'] image = base_env['IMAGE'] +recipe_fingerprint = os.environ.get('RECIPE_FINGERPRINT', '') with open(f'{result_filename}.json') as f: bmk_result = json.load(f) @@ -137,6 +138,7 @@ def record_power_internal_error( 'precision': precision, 'spec_decoding': spec_decoding, 'disagg': disagg, + 'recipe_fingerprint': recipe_fingerprint, 'isl': int(isl), 'osl': int(osl), } diff --git a/utils/test_process_changelog.py b/utils/test_process_changelog.py index bc9becf9d6..252457590f 100644 --- a/utils/test_process_changelog.py +++ b/utils/test_process_changelog.py @@ -4,12 +4,21 @@ import subprocess import sys from contextlib import nullcontext +from pathlib import Path from types import SimpleNamespace import process_changelog +from matrix_logic.generate_sweep_configs import generate_test_config_sweep +from matrix_logic.validation import validate_master_config -def _fixed_matrix_row(conc, *, image="vllm/vllm-openai:v0.16.0"): +def _fixed_matrix_row( + conc, + *, + image="vllm/vllm-openai:v0.16.0", + tp=8, + duration=None, +): return { "image": image, "model": "deepseek-ai/DeepSeek-V4-Pro", @@ -20,7 +29,7 @@ def _fixed_matrix_row(conc, *, image="vllm/vllm-openai:v0.16.0"): "runner": "cluster:b300-nv", "isl": 8192, "osl": 1024, - "tp": 8, + "tp": tp, "pp": 1, "dcp-size": 1, "pcp-size": 1, @@ -28,11 +37,11 @@ def _fixed_matrix_row(conc, *, image="vllm/vllm-openai:v0.16.0"): "dp-attn": True, "conc": conc, "max-model-len": 10240, - "exp-name": f"dsv4_conc{conc}", + "exp-name": f"dsv4_tp{tp}_conc{conc}", "disagg": False, "run-eval": False, "eval-only": False, - } + } | ({"duration": duration} if duration is not None else {}) def _scenario_values(command): @@ -42,6 +51,21 @@ def _scenario_values(command): return command[index:] +def test_recipe_fingerprint_reaches_all_e2e_benchmark_jobs(): + workflow = (Path(__file__).parents[1] / ".github/workflows/e2e-tests.yml").read_text() + + assert workflow.count("uses: ./.github/workflows/benchmark") == 8 + assert workflow.count("recipe-fingerprint: ${{ matrix.config") == 8 + + +def test_recipe_fingerprint_disambiguates_result_and_artifact_names(): + repo_root = Path(__file__).parents[1] + for template_name in ("benchmark-tmpl.yml", "benchmark-multinode-tmpl.yml"): + template = (repo_root / ".github/workflows" / template_name).read_text() + assert 'RECIPE_FINGERPRINT: ${{ inputs.recipe-fingerprint }}' in template + assert 'recipe-${RECIPE_FINGERPRINT:0:16}' in template + + def test_trim_conc_supports_nested_backend_metadata(): common = { "model": "moonshotai/Kimi-K3", @@ -150,12 +174,114 @@ def test_append_only_delta_rejects_image_changes(): try: process_changelog.append_only_delta(base, head) except ValueError as error: - assert "curve logic" in str(error) + assert "remove or modify" in str(error) else: raise AssertionError("image mutation should reject append-only mode") -def test_append_only_scope_rejects_non_concurrency_recipe_changes(): +def test_append_only_delta_allows_new_parallelism_with_its_points(): + base = [ + _fixed_matrix_row(1, tp=4), + _fixed_matrix_row(4, tp=4), + _fixed_matrix_row(8, tp=4), + ] + head = [ + *base, + _fixed_matrix_row(12, tp=8), + _fixed_matrix_row(16, tp=8), + ] + + delta = process_changelog.append_only_delta(base, head) + + assert [(entry["tp"], entry["conc"]) for entry in delta] == [ + (8, 12), + (8, 16), + ] + + +def test_append_only_delta_allows_any_new_recipe_while_preserving_old_recipe(): + base = [_fixed_matrix_row(4, duration=3600)] + head = [*base, _fixed_matrix_row(6, duration=300)] + + delta = process_changelog.append_only_delta(base, head) + + assert [(entry["duration"], entry["conc"]) for entry in delta] == [(300, 6)] + + +def test_append_only_delta_rejects_head_only_image_variant(): + base = [_fixed_matrix_row(4)] + head = [ + *base, + _fixed_matrix_row(8, image="vllm/vllm-openai:v0.16.1", tp=16), + ] + + try: + process_changelog.append_only_delta(base, head) + except ValueError as error: + assert "unchanged non-null image" in str(error) + else: + raise AssertionError("an append cannot fork the target curve's image") + + +def test_recipe_fingerprint_ignores_concurrency_and_experiment_name(): + first = _fixed_matrix_row(4) + second = _fixed_matrix_row(16) + + assert process_changelog.recipe_fingerprint(first) == ( + process_changelog.recipe_fingerprint(second) + ) + + +def test_recipe_fingerprint_changes_for_any_recipe_variant(): + base = _fixed_matrix_row(4, tp=4, duration=3600) + changed_parallelism = _fixed_matrix_row(4, tp=8, duration=3600) + changed_duration = _fixed_matrix_row(4, tp=4, duration=300) + + fingerprints = { + process_changelog.recipe_fingerprint(entry) + for entry in (base, changed_parallelism, changed_duration) + } + + assert len(fingerprints) == 3 + + +def test_append_only_delta_rejects_removed_parallelism_recipe(): + tp4 = _fixed_matrix_row(4, tp=4) + tp8 = _fixed_matrix_row(8, tp=8) + + try: + process_changelog.append_only_delta([tp4, tp8], [tp4]) + except ValueError as error: + assert "remove or modify" in str(error) + else: + raise AssertionError("removing a parallelism recipe should reject append-only mode") + + +def test_append_only_delta_rejects_modified_existing_recipe(): + base = [_fixed_matrix_row(4, duration=3600)] + head = [_fixed_matrix_row(4, duration=300)] + + try: + process_changelog.append_only_delta(base, head) + except ValueError as error: + assert "remove or modify" in str(error) + else: + raise AssertionError("modifying an existing recipe should reject append-only mode") + + +def test_append_only_delta_rejects_removed_existing_point(): + base = [_fixed_matrix_row(4), _fixed_matrix_row(8)] + head = [_fixed_matrix_row(8), _fixed_matrix_row(12)] + + try: + process_changelog.append_only_delta(base, head) + except ValueError as error: + assert "remove existing concurrency" in str(error) + else: + raise AssertionError("removing an existing point should reject append-only mode") + + +def test_append_only_scope_defers_selected_scenario_changes_to_matrix_comparison(): base = { "test-config": { "image": "vllm/vllm-openai:v0.16.0", @@ -178,14 +304,123 @@ def test_append_only_scope_rejects_non_concurrency_recipe_changes(): }, } } + process_changelog.validate_append_only_scope( + base, head, {"test-config": {"agentic-coding"}} + ) + + +def test_append_only_scope_allows_additive_top_level_restructuring(): + router_a = {"name": "router-a", "version": "1"} + router_b = {"name": "router-b", "version": "2"} + base = { + "test-config": { + "image": "img", + "model": "m", + "model-prefix": "m", + "precision": "fp4", + "framework": "vllm", + "runner": "b200", + "multinode": False, + "router": router_a, + "scenarios": { + "fixed-seq-len": [ + { + "isl": 8192, + "osl": 1024, + "search-space": [{"tp": 4, "conc-list": [1, 4, 8]}], + } + ] + }, + } + } + head = json.loads(json.dumps(base)) + head["test-config"].pop("router") + search_space = head["test-config"]["scenarios"]["fixed-seq-len"][0][ + "search-space" + ] + search_space[0]["router"] = router_a + search_space.append( + {"tp": 8, "conc-list": [12, 16], "router": router_b} + ) + + validate_master_config(base) + validate_master_config(head) + args = SimpleNamespace( + config_keys=["test-config"], + seq_lens=None, + conc=None, + scenario_type=["fixed-seq-len"], + runner_node_filter=None, + ) + base_rows = generate_test_config_sweep(args, base) + head_rows = generate_test_config_sweep(args, head) + + process_changelog.validate_append_only_scope( + base, head, {"test-config": {"fixed-seq-len"}} + ) + delta = process_changelog.append_only_delta(base_rows, head_rows) + + assert [(row["tp"], row["conc"], row["router"]) for row in delta] == [ + (8, 12, router_b), + (8, 16, router_b), + ] + + +def test_append_only_scope_rejects_global_change_with_unselected_scenario(): + base = { + "test-config": { + "router": {"name": "dynamo-router", "version": "0.8.1"}, + "scenarios": { + "fixed-seq-len": {"search-space": [{"tp": 4, "conc-list": [1]}]}, + "agentic-coding": {"search-space": [{"tp": 4, "conc-list": [1]}]}, + }, + } + } + head = { + "test-config": { + "router": {"name": "dynamo-router", "version": "0.8.2"}, + "scenarios": base["test-config"]["scenarios"], + } + } + + try: + process_changelog.validate_append_only_scope( + base, head, {"test-config": {"fixed-seq-len"}} + ) + except ValueError as error: + assert "config-wide fields" in str(error) + else: + raise AssertionError("global changes may not affect an unselected scenario") + + +def test_append_only_scope_rejects_changes_to_unselected_scenario(): + base = { + "test-config": { + "scenarios": { + "fixed-seq-len": {"search-space": [{"tp": 4, "conc-list": [1]}]}, + "agentic-coding": {"search-space": [{"tp": 4, "conc-list": [1]}]}, + } + } + } + head = { + "test-config": { + "scenarios": { + "fixed-seq-len": {"search-space": [{"tp": 4, "conc-list": [1]}]}, + "agentic-coding": { + "search-space": [{"tp": 4, "conc-list": [1, 4]}] + }, + } + } + } + try: process_changelog.validate_append_only_scope( - base, head, {"test-config": {"agentic-coding"}} + base, head, {"test-config": {"fixed-seq-len"}} ) except ValueError as error: - assert "duration" in str(error) + assert "outside its changelog scope" in str(error) else: - raise AssertionError("recipe mutation should reject append-only mode") + raise AssertionError("unselected scenario changes should reject append-only mode") def test_append_only_scope_allows_range_to_list_expansion(): @@ -233,8 +468,14 @@ def test_append_only_main_runs_only_added_points_and_skips_evals( monkeypatch.setattr(process_changelog, "get_added_lines", lambda *_: added_yaml) monkeypatch.setattr( process_changelog, - "config_files_at_ref", - lambda *_: nullcontext(["base-nvidia.yaml", "base-amd.yaml"]), + "generation_inputs_at_ref", + lambda *_: nullcontext( + process_changelog.GenerationInputs( + config_files=["base-nvidia.yaml", "base-amd.yaml"], + generator_script="base-generate-sweep-configs.py", + runner_config="base-runners.yaml", + ) + ), ) monkeypatch.setattr( process_changelog, @@ -259,9 +500,14 @@ def fake_run(command, **kwargs): output = json.loads(capsys.readouterr().out) assert [row["conc"] for row in output["single_node"]["8k1k"]] == [8] + assert len(output["single_node"]["8k1k"][0]["recipe-fingerprint"]) == 64 assert output["evals"] == [] assert output["changelog_metadata"]["entries"][0]["append-only"] is True assert len(commands) == 2 + assert commands[0][1] == process_changelog.GENERATE_SWEEPS_PY_SCRIPT + assert commands[1][1] == "base-generate-sweep-configs.py" + assert commands[0][commands[0].index("--runner-config") + 1] == "configs/runners.yaml" + assert commands[1][commands[1].index("--runner-config") + 1] == "base-runners.yaml" def test_all_evals_skips_benchmarks_and_uses_all_evals_generator_flag( diff --git a/utils/test_process_result.py b/utils/test_process_result.py index 4d5219010f..209f8b91fe 100644 --- a/utils/test_process_result.py +++ b/utils/test_process_result.py @@ -52,6 +52,7 @@ def base_env_vars(): "DISAGG": "false", "MODEL_PREFIX": "dsr1", "IMAGE": "test-image", + "RECIPE_FINGERPRINT": "a" * 64, } @@ -215,6 +216,7 @@ def test_single_node_processing(self, tmp_path, sample_benchmark_result, single_ assert output_data["isl"] == 1024 assert output_data["osl"] == 1024 assert output_data["disagg"] is False + assert output_data["recipe_fingerprint"] == "a" * 64 # Verify single-node specific fields assert output_data["is_multinode"] is False