Skip to content

CONTRIBUTING.md

Latest commit

 

History

History
116 lines (82 loc) · 12.3 KB

CONTRIBUTING.md

File metadata and controls

116 lines (82 loc) · 12.3 KB
Raw

Contributing to InferenceX

English | 中文

Thanks for contributing! PRs are welcome. This page covers the review process every PR goes through before it can be merged.

PR review flow

  1. Open your PR and get it through PR validation. Add the full-sweep-fail-fast label (strongly recommended because a broken change wastes one job per matrix rather than the whole fan-out). Use full-sweep-enabled only if you need jobs to keep running past a failure. Let the benchmark sweep run and get a green full sweep, including evals, on a commit in your PR.
  2. For changes owned by a non-admin CODEOWNER other than @SemiAnalysisAI/core, ask one eligible CODEOWNER to review and post the PR Review Checklist sign-off (see below) in their approval comment.
  3. Ping a core maintainer on Slack for final approval, after obtaining the checklist sign-off when required.
  4. An authorized maintainer posts /reuse-sweep-run (see below) and the PR is merged via the reuse path.

Performance changelog requirement: Every change that can affect benchmark performance and every recipe addition or modification MUST append a new entry to the physical end of perf-changelog.yaml. Historical entries MUST NOT be edited.

The PR Review Checklist (CODEOWNER sign-off)

Sign-off is required only when a changed file has a CODEOWNER other than a repository admin or @SemiAnalysisAI/core. Ownership comes from the current tip of the PR target branch, resolved once and pinned to the same SHA for CODEOWNERS validation and content reads, using the last matching rule; renames check both old and new paths. The PR head and its potentially stale recorded base SHA do not supply ownership rules. A matching core owner does not exempt another owner on the same file. Individual admins must have both repository permission: admin and role_name: admin; other teams and email owners require sign-off. Missing ownership data or failed permission lookups cannot grant an exemption. Changes without a qualifying owner receive a successful “not applicable” status without starting the verifier.

One eligible CODEOWNER reviewer fills in the latest PR_REVIEW_CHECKLIST.md template in their approval comment.

Only one eligible CODEOWNER reviewer needs to post the checklist for each PR. Check for an existing checklist before posting; additional reviewers do not need to post their own copies. For corrections, missing evidence, or verification retries, the original reviewer must edit their existing checklist comment instead of adding a new one. Create a replacement only if the original comment was deleted.

A friendly reminder. Please follow the latest checklist template correctly:

  • Always copy the template from the current docs/PR_REVIEW_CHECKLIST.md on main. The checklist evolves, and a sign-off made from a stale copy will be flagged as missing items.

  • Keep the template's opening phrase intact:

    As a PR reviewer and CODEOWNER, I have reviewed this and have:

    Our CI verification workflow, codeowner-signoff-verify.yml, triggers on exactly this phrase. If your approval comment does not follow the checklist template, including that phrase, the sign-off verification CI will not trigger at all, and your sign-off won't count toward merge.

  • The sign-off can be posted as a regular conversation comment, a review summary, or an inline review comment. All three trigger verification.

  • Before the first PASS, head updates, reopening a PR, and marking it ready recover the latest eligible existing sign-off on the current head. This catches reviews missed during merge conflicts without requiring a duplicate checklist.

  • Starting Claude still requires an eligible actor with repository write access. After an update by a non-writer or disallowed bot, a collaborator with write access can request the initial verification. Carrying an existing PASS forward does not require a new verification.

  • Fill in the "Additional detail section" with the links the checklist asks for (validation/eval workflow runs, the corresponding vLLM recipe / SGLang cookbook PR, and any exception reasoning).

Once the sign-off is posted, CI independently re-verifies the claims that gate a merge, including CODEOWNER status, a green sweep and evals on a commit in the PR, the linked recipe, the /reuse-sweep-run command, use of the latest checklist template, upstream vLLM/SGLang images, no architecture-changing benchmark hacks, and chat-template usage for speculative decoding. It then creates or updates one verdict comment for the PR, including the SHA actually assessed. Failing criteria stay visible; passing and N/A criteria appear together in a collapsed section. An existing verdict comment from the older per-commit format is reused; if the comment was deleted, the next verification creates a replacement. Checkmarks are not taken on trust, so please only check items you have actually verified.

Admin updates do not invalidate sign-off; non-admin updates do. The trusted workflow uses the authenticated user who pushed the update and requires repository permission: admin and role_name: admin. Commit author names and emails do not grant this exemption. An admin update that starts from a covered head advances coverage without calling Claude. A non-admin update, including a merge from main, needs fresh sign-off verification; a later admin push cannot clear that requirement.

The verdict comment records the assessed commit and the head covered by subsequent admin updates. The old codeowner-signoff-verified lifetime label is removed and cannot grant acceptance. Trusted legacy verdicts need an assessed SHA; contributor-authored comments, missing provenance, and unavailable permission information cannot extend coverage. If an update cannot be connected to the recorded covered head, verification is required. Deleting the verdict comment also removes that proof. To verify new non-admin changes, the original reviewer edits their existing checklist, or an authorized collaborator dispatches codeowner-signoff-verify.yml with pr-number and its comment_url (both must identify the same PR). This updates the same verdict comment, and a rejected reassessment revokes acceptance.

Reusing your PR's green sweep at merge with /reuse-sweep-run

A full benchmark sweep is expensive GPU time, and the runners are shared by every open PR. Without reuse, an approved PR's sweep would run twice, once for PR validation and again on main after merge. The reuse path avoids that:

  • After your PR has an eligible green full sweep, an authorized maintainer (OWNER/MEMBER/COLLABORATOR) comments /reuse-sweep-run on the PR (optionally pinning a specific run: /reuse-sweep-run <run_id>).
  • The merge-to-main run then validates and ingests the PR sweep's artifacts instead of re-running the whole sweep on main.
  • 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.
  • Reuse does not require retaining a sweep label. The bot reacts to the command with 👍 when accepted or 👎 when rejected, with details in the Actions run summary; source artifacts are revalidated at merge.
  • 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 for eligibility details.

Adding points to the latest curve with append-only

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 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.

- config-keys:
    - dsv4-fp4-b300-vllm-mtp
  description:
    - "Add TP8 at concurrency 12 and 16 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:

Error: File was unable to be removed
Error: EACCES: permission denied, rmdir '.../benchmark_logs/logs/slurm_job-<id>'

This bricks every subsequent job on that runner until someone with sudo on the shared /it-share storage manually removes the files. Because all AMD MI355X sweeps share the same runner pool, a single stranded root-owned directory blocks the entire queue for everyone.

Rules for benchmark scripts and Slurm containers:

  1. Never write as root into the runner workspace. If your container must run as root, write outputs to a separate scratch directory outside _work/ (e.g. /tmp or a dedicated staging path).
  2. If root writes are unavoidable, add a cleanup trap or teardown step that chowns or rms all root-owned files under the workspace before the job exits, including on cancellation (trap cleanup EXIT).
  3. Test your teardown path. Cancel a running benchmark mid-flight and verify no root-owned files remain in the workspace.

If you find a stranded root-owned file blocking runners, the recovery procedure is documented in .claude/commands/clean-amd-mi355-runner-root-files.md: SSH into the hop host with sudo, scan the _work directories, and delete the offending files.

After merging

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs.