test(ci): validate append-only Qwen 3.5 AgentX point - #2614
Conversation
…n35-canary # Conflicts: # perf-changelog.yaml
| 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"} | ||
| ) |
There was a problem hiding this comment.
🟡 The new _matrix_curve_key helper (utils/process_changelog.py:169-176) computes the exact same tuple-of-sorted-(key,_freeze_config_value(value))-excluding-conc/exp-name that trim_conc already builds inline (utils/process_changelog.py:91-97). This is a pre-existing duplication now made worse by the PR: trim_conc should call _matrix_curve_key(entry) instead of re-deriving the identical key, so a future change to the grouping rule can't silently diverge between the two.
Extended reasoning...
What the duplication is
This PR introduces _matrix_curve_key in utils/process_changelog.py (lines 169-176) as the canonical way to identify "one curve" for append-only delta computation:
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"}
)
)The pre-existing trim_conc function (lines ~91-97) builds the exact same key inline, in its single-node branch, to group matrix rows before keeping only the lowest-concurrency entry per group:
key = tuple(
sorted(
(k, _freeze_config_value(v))
for k, v in entry.items()
if k not in {"conc", "exp-name"}
)
)
groups.setdefault(key, []).append(len(out))Both blocks compute byte-for-byte the same tuple: same exclusion set ({"conc", "exp-name"}), same _freeze_config_value normalization for nested dict/list values, and the same tuple(sorted(...)) construction. They express the identical business rule — "two generated matrix rows belong to the same curve iff every field except concurrency and the generated experiment name matches."
Why this matters
Before this PR, that grouping logic lived in exactly one place (trim_conc). Now the PR adds a second, independent copy of it (_matrix_curve_key), used by the new append_only_delta function to decide which points are "new" versus "existing" for append-only sweep validation. Because _matrix_curve_key is used to gate what benchmark points can run in an append-only PR, and trim_conc's key governs which point survives concurrency trimming, a future change to the grouping rule — e.g. excluding one more point-level field, or changing how a field is normalized — would need to be applied in both places to stay consistent. If only one is updated, the two functions silently diverge: trim_conc might keep grouping by the old rule while _matrix_curve_key's append-only validation uses the new one (or vice versa), with no error or test failure to catch the mismatch since both are independently "correct" by their own logic.
Why nothing currently prevents this
Nothing enforces that the two key-derivations stay in sync — they're separate function bodies with duplicated logic, not a single shared helper. Python resolves names at call time, so trim_conc (defined earlier in the file) can call _matrix_curve_key (defined later) without any ordering issue; there's no structural reason for the duplication.
Suggested fix
trim_conc's single-node branch should replace its inline key construction with a call to the new helper:
key = _matrix_curve_key(entry)This is a clean drop-in — trim_conc only builds this key after its multi-node early-continue, so the refactor doesn't change any behavior, it just removes the duplicate derivation.
Concrete proof of duplication
Given entry = {"image": "vllm:v1", "conc": 4, "exp-name": "foo_conc4", "tp": 8}:
trim_conc's inline key:tuple(sorted((k, _freeze_config_value(v)) for k, v in entry.items() if k not in {"conc","exp-name"}))→(("image", "vllm:v1"), ("tp", 8))_matrix_curve_key(entry)→(("image", "vllm:v1"), ("tp", 8))
Identical output for identical input, computed by two independent code paths.
This is a maintainability/reuse (DRY) finding, not a functional bug — the two implementations currently produce identical results, so it does not block merge, but it's worth cleaning up in this PR since the duplicate was introduced by it.
| - [ ] 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. |
There was a problem hiding this comment.
🟡 The new append-only checklist bullet in docs/PR_REVIEW_CHECKLIST.md (lines 28-31) and the new "Adding points to the latest curve with append-only" section in CONTRIBUTING.md were not mirrored into docs/PR_REVIEW_CHECKLIST_zh.md / CONTRIBUTING_zh.md, leaving these bilingual doc pairs out of sync. This is a pre-existing documentation-process convention (docs/documentation-procedures.md), not a functional bug — flagging so the Chinese pages can be updated in a follow-up.
Extended reasoning...
docs/documentation-procedures.md states as a hard Required rule (line 11): "Write or update the English page first, then translate it into the matching Simplified Chinese _zh.md page in the same change." It further calls out (line 136) that the PR-review checklist template specifically must appear "English-verbatim in BOTH checklist language files," and lists mismatched translations (line 322) as a blocking documentation defect during review.
This PR edits two bilingual-paired docs that each carry an "English | 中文" language switcher pointing at a _zh.md counterpart: docs/PR_REVIEW_CHECKLIST.md gains a new checklist bullet about append-only: true verification (lines 28-31), and CONTRIBUTING.md gains an entirely new section, "Adding points to the latest curve with append-only" (lines 47-73). Neither docs/PR_REVIEW_CHECKLIST_zh.md nor CONTRIBUTING_zh.md appears in the PR's changed-files list, and grepping both files directly confirms there is no append-only content in either — PR_REVIEW_CHECKLIST_zh.md has zero matches for "append-only," and CONTRIBUTING_zh.md's only "追加" occurrence is an unrelated sentence about the perf-changelog append convention (line 19), not the new append-only curve-ingestion feature.
Concretely, a reader who switches to the 中文 version of either page today will see a checklist and contribution guide that don't mention append-only at all — the exact "mismatched translation" scenario documentation-procedures.md was written to prevent. Nothing in the PR's diff or CI enforces this sync (the doc-procedures rule is a human/reviewer convention, not something codeowner-signoff-verify-prompt.md's automated checks currently verify), so it's easy for a change like this to land without the translator step.
Proof: (1) grep -c append-only docs/PR_REVIEW_CHECKLIST_zh.md → 0. (2) grep -n 追加 CONTRIBUTING_zh.md → only line 19, an unrelated perf-changelog sentence, confirming the new "Adding points to the latest curve" section was never translated. (3) Both English files carry the "English | 中文" switcher (PR_REVIEW_CHECKLIST.md:5, CONTRIBUTING.md:5), and neither is in the documented English-only exception list (AGENTS.md, CLAUDE.md, KLAUD_DEBUG.md, .github/, utils/, configs/CONFIGS.md, experimental/README.md), so the translation-sync rule genuinely applies here.
Fix: translate the new checklist bullet into docs/PR_REVIEW_CHECKLIST_zh.md (verbatim per the English-verbatim-in-both-files rule for checklist content) and translate the new "Adding points to the latest curve with append-only" section into CONTRIBUTING_zh.md, in the same PR that introduces the English content. This is a documentation-process/quality gap rather than a functional defect — it doesn't affect the append-only ingestion logic, CI checks, or runtime behavior — so it should not block merging this PR, but it should be fixed before the change reaches main to keep the bilingual docs from drifting further apart.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31842466187 |
Summary
append-only: truePurpose
This is a disposable end-to-end validation PR and must not be merged. It temporarily targets
mainbecause the standard sweep workflow only runs for pull requests whose base ismain.The duration override is scoped to
qwen3.5_tp4_conc6_kvnone_spec-mtp, an experiment name absent from the base matrix, so it cannot affect any inherited point.Results
[1, 4, 6, 8, 12, 14]Links
Merge policy
Do not merge this canary PR. It remains in draft after validation.