Skip to content

feat(ci): support additive append-only curve updates - #2613

Merged
cquil11 merged 4 commits into
mainfrom
agent/append-only-sweeps
Aug 14, 2026
Merged

feat(ci): support additive append-only curve updates#2613
cquil11 merged 4 commits into
mainfrom
agent/append-only-sweeps

Conversation

@cquil11

@cquil11 cquil11 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add an explicit append-only: true perf-changelog mode
  • generate base and head matrices with each revision's own generator, validation code, master configs, and runner metadata
  • require every existing generated point to remain byte-for-byte equivalent, then schedule only newly added recipes or concurrency points
  • allow additive topology and recipe variants within an existing visual curve while requiring one unchanged non-null image
  • assign a deterministic recipe fingerprint so same-coordinate recipe variants remain distinct downstream
  • propagate the fingerprint through internal and external workflows, result payloads, and artifact filenames
  • add the behavioral-isolation policy to both active AI reviewers and the CODEOWNER signoff check

Safety model

The base generated matrix must be an immutable subset of the head matrix. Existing points, configs, scenarios, images, and recipe properties cannot be removed or modified. New generated recipes are allowed only inside a selected existing config/scenario and the same App visual series and image.

There is intentionally no file allowlist. Benchmark and launcher code may change, but human and AI reviewers must prove that each runtime behavior change is gated to the appended recipe fingerprints and cannot affect an existing point. Shared, unguarded, or unprovable behavior is blocking.

Verification

  • 435 focused changelog, schema, generator, result-processing, and workflow-gating tests passed
  • all changed workflow YAML files parse successfully
  • the full generated catalog stays within artifact filename limits after fingerprint suffixes
  • three independent review tracks tested producer, App, and end-to-end behavior; all actionable findings were implemented and the final verdicts are clean
  • git diff --check, formatting, lint, and type checking passed where applicable
  • the disposable canary scheduled only B200 SGLang AgentX Qwen 3.5 TP4 concurrency 6 and measured for about five minutes

End-to-end links

Vercel team sign-in may be required because preview protection remains enabled.

Companion PR

@cquil11
cquil11 marked this pull request as ready for review August 14, 2026 22:01
@cquil11
cquil11 requested a review from a team August 14, 2026 22:01

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, two other concerns were checked and ruled out: (1) the base-config temp directory in config_files_at_ref is not leaked on an early exception in any way that matters — the script is a short-lived CLI process that exits immediately on error, so the OS reclaims the tempdir; (2) the append-only scope check's key-set comparison in _validate_concurrency_only_structure can reject a base/head pair that differ only in whether a default-valued field (e.g. pp: 1) is explicit vs. omitted, but that's consistent with the PR's stated fail-closed design (reject when exclusivity can't be proven), not a bug.

Extended reasoning...

Reviewed the temp-directory lifecycle in config_files_at_ref (utils/process_changelog.py) and the key-set diffing in _validate_concurrency_only_structure. Neither represents a real defect given the script's short-lived CLI execution model and the intentionally conservative (fail-closed) design of the append-only scope validator.

Comment thread docs/PR_REVIEW_CHECKLIST.md Outdated
- [ ] 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)`<PR_NUMBER>.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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 AGENTS.md and docs/documentation-procedures.md require every English contributor-facing doc change to be synced into its _zh.md counterpart in the same PR, and explicitly name docs/PR_REVIEW_CHECKLIST.md as needing English-verbatim sync in both language files. This PR adds a new append-only checklist bullet to docs/PR_REVIEW_CHECKLIST.md (line 31) and a new 'Adding points to the latest curve with append-only' section to CONTRIBUTING.md, but touches neither docs/PR_REVIEW_CHECKLIST_zh.md nor CONTRIBUTING_zh.md — both now diverge from their English source.

Extended reasoning...

The bug: docs/documentation-procedures.md establishes a repo-mandated, same-PR bilingual sync requirement for contributor-facing docs: line 11 states _Required:_ Write or update the English page first, then translate it into the matching Simplified Chinese _zh.mdpage in the same change_", and line 129 repeats "Translate the finalized English changes into the matching_zh.mdpage in the same PR." The 'Special case: the PR review checklist' section (around line 136) goes further and explicitly names this exact file:docs/PR_REVIEW_CHECKLIST.md's template 'must remain English-verbatim in **both** checklist language files because the verifier detects exact English wording.' AGENTS.mdline 16 reiterates that contributor-facing docs ship with a synchronized_zh.md` page.

Where it manifests in this PR: the diff adds a new checklist bullet to docs/PR_REVIEW_CHECKLIST.md (the append-only sign-off verification item, at line 31) and an entirely new section, 'Adding points to the latest curve with append-only', to CONTRIBUTING.md. Neither docs/PR_REVIEW_CHECKLIST_zh.md nor CONTRIBUTING_zh.md appears anywhere in this PR's 8 changed files.

Proof: both zh files are genuine bilingual pairs today (each carries the reciprocal English | 中文 switcher, and PR_REVIEW_CHECKLIST_zh.md even instructs reviewers to copy the checklist template verbatim in English — see its own note: '模板请保持英文原文,原样复制粘贴,不要翻译'). Grepping both files for 'append-only' after this PR returns zero matches:
grep -c append-only CONTRIBUTING_zh.md docs/PR_REVIEW_CHECKLIST_zh.md0, 0.
So after merge, an English-only reader sees the new append-only workflow documented in both CONTRIBUTING.md and the checklist, while a Chinese-only reader sees neither — the checklist item a CODEOWNER is supposed to check off is simply absent from the zh template they'd copy from, and the zh contributing guide gives no explanation of what append-only: true means or when to use it.

Why nothing else catches this: there is no CI check enforcing bilingual parity; the requirement is purely a documented human-process convention in documentation-procedures.md, so it's easy for a PR that only touches English docs to slip through review unless a reviewer explicitly diffs the zh counterparts.

The fix: append the same checklist bullet (kept English-verbatim, matching the existing convention for that template) to docs/PR_REVIEW_CHECKLIST_zh.md, and add a translated 'Adding points to the latest curve with append-only' section to CONTRIBUTING_zh.md mirroring the new English section (code block and YAML example preserved verbatim, prose translated), per documentation-procedures.md's 'Update an existing bilingual pair' procedure.

Comment thread utils/process_changelog.py Outdated
Comment on lines +151 to +165
@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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 config_files_at_ref() in utils/process_changelog.py (lines 151-165) re-implements the git-show read that utils/validate_perf_changelog.py's read_git_file(ref, path) already does, but drops its stderr-decoding error handling. Because it uses subprocess.run(check=True) with captured stderr, a base ref missing one of MASTER_CONFIGS surfaces only a bare, unreadable CalledProcessError instead of read_git_file's readable 'could not read {path} at {ref}: {detail}' message. Reusing read_git_file (already imported by prepare_perf_changelog_merge.py and recover_failed_ingest.py) would fix this with no functional downside.

Extended reasoning...

What the bug is: config_files_at_ref() in utils/process_changelog.py:151-165 materializes the master configs at a given git ref for append-only matrix comparison. It does this by shelling out to git show <ref>:<path> via subprocess.run(..., capture_output=True, check=True) and writing result.stdout to a temp file. This is functionally the same operation as read_git_file(ref, path), already defined in utils/validate_perf_changelog.py:73, which is reused by both prepare_perf_changelog_merge.py and recover_failed_ingest.py.

The difference that matters: read_git_file doesn't just run the same git show command — on a non-zero return code it decodes result.stderr and raises ChangelogValidationError(f'could not read {path} at {ref}: {detail}'), giving the caller a readable, actionable error. The new config_files_at_ref instead relies on check=True, so on failure it raises a bare subprocess.CalledProcessError. Because capture_output=True swallows stderr into the exception object, CalledProcessError's default __str__ ('Command [...] returned non-zero exit status 128.') never surfaces that captured detail — the actual git error message (e.g. 'fatal: path does not exist in ') is silently discarded.

Where this triggers: this is exactly the failure mode append-only mode has to handle gracefully: a base ref that predates one of the files in MASTER_CONFIGS, or a base ref where a selected config file was renamed/moved. main() calls config_files_at_ref(args.base_ref) unconditionally whenever any changelog entry is append-only: true (line ~382 in main()), so this code path runs on every append-only sweep, not just an edge case.

Why nothing else catches it: the caller just does base_config_context.__enter__() and lets any exception propagate; there's no wrapping try/except that would add context back. The opaque error would surface directly in CI logs, forcing whoever's debugging the failed append-only sweep to go dig through git manually to find out which of the MASTER_CONFIGS paths was missing, rather than being told immediately.

Proof by example: suppose args.base_ref points to a commit before configs/amd-master.yaml was added to MASTER_CONFIGS. config_files_at_ref loops over MASTER_CONFIGS, and when it reaches configs/amd-master.yaml, runs git show <base_ref>:configs/amd-master.yaml, which exits 128 with stderr fatal: path 'configs/amd-master.yaml' does not exist in '<base_ref>'. With check=True, this raises CalledProcessError(128, [...]) whose string form omits that fatal message entirely — the process just dies with 'Command '['git', 'show', ...]' returned non-zero exit status 128.' If config_files_at_ref instead called read_git_file(ref, config_file) (e.g. destination.write_bytes(read_git_file(ref, config_file))), the same failure would raise ChangelogValidationError(\"could not read configs/amd-master.yaml at <base_ref>: fatal: path 'configs/amd-master.yaml' does not exist in '<base_ref>'\") — immediately actionable.

Fix: replace the inline subprocess.run call in config_files_at_ref with a call to read_git_file(ref, config_file) from utils/validate_perf_changelog.py (importing it the same way prepare_perf_changelog_merge.py and recover_failed_ingest.py already do), writing its returned bytes to destination. This removes the duplicated git-show logic and gives append-only sweep failures the same readable error message the rest of the changelog tooling already has.

Comment on lines +169 to +176
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"}
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 trim_conc() builds its curve-grouping key inline with the exact same expression that this PR's new _matrix_curve_key() helper (utils/process_changelog.py:169-177) implements. trim_conc should call _matrix_curve_key(entry) instead of duplicating the logic, so the two group-by-curve computations can't silently drift apart if one is edited later.

Extended reasoning...

This PR adds _matrix_curve_key() (utils/process_changelog.py:169-177) as a helper for append_only_delta(), defined as:

def _matrix_curve_key(entry: dict) -> tuple:
    return tuple(
        sorted(
            (key, _freeze_config_value(value))
            for key, value in entry.items()
            if key not in {"conc", "exp-name"}
        )
    )

The pre-existing trim_conc() function (same file, ~line 88-95) already computes an identical grouping key inline, just with k/v loop-variable names instead of key/value:

key = tuple(
    sorted(
        (k, _freeze_config_value(v))
        for k, v in entry.items()
        if k not in {"conc", "exp-name"}
    )
)

Both expressions exclude the same two fields (conc, exp-name), call the same _freeze_config_value normalizer, and produce the same sorted-tuple shape — they are semantically byte-for-byte identical. Both exist for the same purpose: identifying "one curve" (a unique parallelism/recipe configuration) while ignoring the point-level (conc) and generated (exp-name) fields, so that entries sharing every other field group together.

Nothing today enforces that these two computations stay in sync. If a future change needs to exclude an additional point-level field from the grouping key (e.g. a new per-point-only field is added to matrix entries), a developer would need to remember to update both trim_conc()'s inline key and _matrix_curve_key() — and it would be easy to update only the one they're looking at, silently causing trim_conc()'s deduplication and append_only_delta()'s curve-matching to disagree about what constitutes "the same curve."

The fix is a one-line change: replace the inline key = tuple(sorted(...)) block inside trim_conc()'s loop with key = _matrix_curve_key(entry). Since _matrix_curve_key is defined earlier in the same module and takes the same entry dict, this is a drop-in, behavior-preserving substitution — the existing test_trim_conc_supports_nested_backend_metadata test would still pass unchanged, since the computed key is identical either way.

All three independent verifiers confirmed this is a genuine, low-risk DRY consolidation opportunity introduced by this PR (the helper is new, so the duplication is new too), not a pre-existing issue. It's a maintainability/code-quality nit, not a correctness bug — nothing breaks if it's left as-is, but consolidating removes a latent risk of the two curve-grouping computations drifting apart.

@cquil11 cquil11 changed the title feat(ci): add append-only curve sweeps feat(ci): support additive append-only curve updates Aug 14, 2026
@cquil11
cquil11 merged commit 35d731a into main Aug 14, 2026
7 checks passed
@cquil11
cquil11 deleted the agent/append-only-sweeps branch August 14, 2026 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant