Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions .github/codeowner-signoff-verify-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -359,8 +359,34 @@ 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 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.
- 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.
- 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 — <reason>` 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:
Expand All @@ -386,7 +412,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
Expand Down
10 changes: 10 additions & 0 deletions .github/workflows/claude-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,16 @@ 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:
- 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.
- 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. 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.
- **MTP (Multi-Token Prediction)**: Predicts multiple tokens per forward pass using speculative decoding (e.g., EAGLE, NEXTN).
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/claude.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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`, 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.

Expand Down
28 changes: 28 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,34 @@ 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 <pr-number>` 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, 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:
- 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:
Expand Down
2 changes: 1 addition & 1 deletion docs/PR_REVIEW_CHECKLIST.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
- [ ] 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.

Check failure on line 31 in docs/PR_REVIEW_CHECKLIST.md

View check run for this annotation

Claude / Claude Code Review

Missing zh translation for new append-only doc content

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.

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.md → 0, 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.

- [ ] If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below.

### Additional detail section:
Expand All @@ -42,4 +43,3 @@


<img width="569" height="632" alt="image" src="https://github.com/user-attachments/assets/491d9763-ab09-4734-b0f1-39eefe1ab5c4" />

19 changes: 19 additions & 0 deletions utils/matrix_logic/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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=(
Expand All @@ -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."""
Expand Down
Loading
Loading