Skip to content

[DNM][AMD][MI355X] DSv4 agentic-hicache: switch to mlpnopad image, narrow DPA sweep #2297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
seungrokj wants to merge 3 commits into from
+13 −6
Closed

[DNM][AMD][MI355X] DSv4 agentic-hicache: switch to mlpnopad image, narrow DPA sweep #2297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96cb022
[AMD][MI355X] DSv4 agentic-hicache: switch to mlpnopad image, narrow …
seungrokj Jul 21, 2026
a25dd30
[AMD][MI355X] Exclude unhealthy nodes mia1-p01-g09,g11 from salloc
seungrokj Jul 21, 2026
d372018
[AMD][MI355X] Add pr-link to DSv4 mlpnopad perf-changelog entry
seungrokj Jul 21, 2026
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
9 changes: 5 additions & 4 deletions configs/amd-master.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,7 +1536,7 @@ dsr1-fp4-mi355x-sglang-disagg-mtp:
- "DECODE_MTP_SIZE=1"

dsv4-fp4-mi355x-sglang-agentic-hicache:
image: lmsysorg/sglang-rocm:v0.5.14-rocm720-mi35x-20260710
image: rocm/pytorch-private:sglang-rocm720-mi35x_dsv4_mlpnopad

@claude claude Bot Jul 21, 2026

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.

🟡 This PR retargets dsv4-fp4-mi355x-sglang-agentic-hicache to rocm/pytorch-private:sglang-rocm720-mi35x_dsv4_mlpnopad, a mutable, non-digest tag intended for iterative kernel evaluation. Because runners/launch_mi355x-amds.sh caches the enroot squashfs keyed only on the image string and skips re-import whenever that file already exists, any runner that has already pulled this tag once will silently keep serving the stale build on every future re-push, with no error — the exact caching hazard already documented on the neighboring dsv4-fp4-mi355x-vllm entry in this file. Consider pinning to a digest-suffixed tag (e.g. @sha256:...) to avoid silently stale benchmark results during the mlpnopad iteration.

Extended reasoning...

The mechanism: runners/launch_mi355x-amds.sh (~lines 264-286) derives SQUASH_FILE deterministically from the IMAGE string alone (sed 's/[\/:@#]/_/g') and stores it at a persistent, node-local path under /var/lib/squash/. The import logic is:

if unsquashfs -l "$SQUASH_FILE" > /dev/null 2>&1; then
  echo "Squash file already exists and is valid, skipping import"
else
  rm -f "$SQUASH_FILE"
  enroot import ... docker://$IMAGE
fi

There is no comparison against the remote registry digest and no TTL/expiry on the cached file — once a squashfs exists for a given image-string key on a runner, it is reused unconditionally on every subsequent job that references the same tag.

Why this PR triggers it: configs/amd-master.yaml:1539 switches dsv4-fp4-mi355x-sglang-agentic-hicache from the previous tag lmsysorg/sglang-rocm:v0.5.14-rocm720-mi35x-20260710 (date-stamped, effectively immutable) to rocm/pytorch-private:sglang-rocm720-mi35x_dsv4_mlpnopad. Per the PR description, this new tag exists specifically for iterative MLP no-pad kernel evaluation — i.e. the vendor is expected to push multiple updated builds under this exact same tag string while tuning the kernel. That is precisely the scenario the launcher's caching logic cannot handle safely: the tag string never changes, so SQUASH_FILE's key never changes, so the cache is never invalidated.

Why nothing in the existing code prevents this: the import guard only checks local squashfs validity (unsquashfs -l succeeding), which tells you the cached file is a well-formed image — not that it matches what's currently at the registry. There's no digest check, no docker manifest inspect-style comparison, and no forced re-pull path exposed through this config.

Impact: on any mi355x-amds runner that has already executed this config once, a subsequent vendor push of an updated mlpnopad build to the same tag is silently ignored — the sweep runs against the old kernel and reports benchmark numbers that do not reflect the intended change, with no error, warning, or log signal distinguishing it from a correct run. This is especially costly in an iterative-evaluation workflow where the whole point is to compare kernel revisions run-over-run.

Proof (step-by-step):

  1. Vendor pushes build A to rocm/pytorch-private:sglang-rocm720-mi35x_dsv4_mlpnopad.
  2. CI runs this PR's sweep on runner X. SQUASH_FILE doesn't exist yet -> enroot import pulls build A -> squashfs cached at /var/lib/squash/rocm_pytorch-private_sglang-rocm720-mi35x_dsv4_mlpnopad.sqsh.
  3. Vendor iterates on the mlpnopad kernel and re-pushes build B to the same tag.
  4. A later sweep is scheduled on the same runner X (same tag string in the config). SQUASH_FILE computed from the image string is identical to step 2, so unsquashfs -l succeeds against the cached (build A) file, prints 'Squash file already exists and is valid, skipping import', and the job runs build A again — not build B.
  5. Benchmark results are recorded as if they reflect build B's kernel changes, but they were actually produced by the stale build A, with no indication to the author or reviewer that this happened.

Precedent: this exact caching hazard is already called out verbatim as an IMPORTANT comment on the neighboring dsv4-fp4-mi355x-vllm entry in this same file: 'pin to a digest-suffixed nightly tag rather than the floating :nightly... the floating tag silently keeps a stale build even after Docker Hub updates.' The new tag in this PR reintroduces the same pattern that comment warns against.

Fix: pin the image to a digest-suffixed reference (rocm/pytorch-private@sha256:<digest>) instead of the mutable tag, so each new build gets a distinct cache key and is guaranteed to be freshly pulled.

Severity note: this does not break the PR's own test plan — the first CI run on a fresh runner pulls the current image correctly, so the stated 'DPA no-offload conc 48/64' validation will pass. The risk only materializes on a later re-push to the same tag on a runner that already cached it, which is a real but not immediate/blocking concern for an internal evaluation config. Filing as a nit/suggestion to pin by digest, consistent with the precedent already documented elsewhere in this file.

@claude claude Bot Jul 21, 2026

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.

🟡 This PR's title and description are English-only, but AGENTS.md mandates bilingual PR titles/descriptions (format: <English title> / <中文标题>, plus a ## 中文说明 section mirroring the summary) for every PR. Please retitle and add the Chinese translation before merging — recent PRs like #2231 and #2232 follow this convention.

Extended reasoning...

AGENTS.md line 7 is explicit and unconditional: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section mirroring the summary; don't translate code blocks, logs, or stack traces — summarize around them)... This applies to every PR and every issue."

This PR's title, [AMD][MI355X] DSv4 agentic-hicache: switch to mlpnopad image, narrow DPA sweep, has no / <中文标题> suffix, and the body (Summary + Test plan) has no ## 中文说明 section translating the change. This isn't a subjective style nit invented by the reviewer — it's a literal, actively-enforced repo convention. The git log shows immediately preceding merged PRs following it correctly: 01e9e73 ... / 将 LMCache 分支并入官方 DSV4 FP4 B200 vLLM AgentX 扫描 (#2231) and 9ecc56d ... / 在调优后的 DSV4 FP4 B300 vLLM AgentX 配方上新增 LMCache 分支 (#2232).

Proof / concrete example: Compare this PR's title against the format the rule requires:

Nothing in the diff itself is wrong — the YAML changes to configs/amd-master.yaml (image swap, sweep-arm pruning) and the perf-changelog.yaml entry are functionally sound and unaffected by this. This is filed against configs/amd-master.yaml only because the bug tracker requires a diff-file location; the actual defect is in the PR title/description metadata, not in any line of code.

Why this doesn't block merge: there's no CI gate enforcing the bilingual-title/description rule (unlike the CODEOWNER sign-off phrase, which does have an automated checker per the recipe-reminder bot comment). Several other recently-merged PRs visible in the log (e.g. #2258 "vLLM DeepSeek-V4 B300 aggregate MTP", #2288 "CollectiveX: add UCCL-EP...") also ship English-only titles, so non-compliance clearly doesn't block merging in practice. The fix is trivial — retitle to append / <中文标题> and add a ## 中文说明 section — but it's a process/documentation gap, not a functional defect, so it's best treated as a nit for the author to clean up rather than a merge-blocking issue.

model: deepseek-ai/DeepSeek-V4-Pro
model-prefix: dsv4
runner: cluster:mi355x-amds
Expand All @@ -1547,9 +1547,10 @@ dsv4-fp4-mi355x-sglang-agentic-hicache:
agentic-coding:
- dram-utilization: 0.80
search-space:
- { tp: 8, kv-offloading: none, conc-list: [1, 2, 4, 8] }
- { tp: 8, dp-attn: true, kv-offloading: none, conc-list: [16, 32, 48, 64] }
- { tp: 8, dp-attn: true, kv-offloading: dram, kv-offload-backend: { name: hicache }, conc-list: [64] }
#- { tp: 8, kv-offloading: none, conc-list: [1, 2, 4, 8] }
#- { tp: 8, dp-attn: true, kv-offloading: none, conc-list: [16, 32, 48, 64] }
#- { tp: 8, dp-attn: true, kv-offloading: dram, kv-offload-backend: { name: hicache }, conc-list: [64] }
- { tp: 8, dp-attn: true, kv-offloading: none, conc-list: [48, 64] }

# MiniMax-M3 MXFP8 MI355X recipe:
# https://github.com/vllm-project/recipes/commit/2a3728ed9892debfd767a72a58ebc90b33f186e5
Expand Down
8 changes: 7 additions & 1 deletion perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5010,4 +5010,10 @@
- "Add MTP speculative-decoding for the B300 vLLM AgentX recipe as a separate config-key, so the existing dsv4-fp4-b300-vllm-agentic aggregate is not re-run."
- "dsv4-fp4-b300-vllm-agentic-mtp: MTP twins (--speculative-config {\"method\":\"mtp\",\"num_speculative_tokens\":3}) of the aggregate arms plus a new TP8 GPU-resident arm -- TP8 GPU-resident [1,2,4,6,8], TP4 GPU-resident [1,2,4,6,8,12,16,20], TP4 SimpleCPU [20,24,28,32,36,40], DEP4 [32,40,48,56], DEP8 [64,96,112,128,144,160,176,192,224]; routed via spec-decoding=mtp to dsv4_fp4_b300_vllm_mtp.sh (FULL_DECODE_ONLY cudagraph capture sizes in TOKENS = num_seqs*4)."
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2258


- config-keys:
- dsv4-fp4-mi355x-sglang-agentic-hicache
description:
- "Switch image to rocm/pytorch-private:sglang-rocm720-mi35x_dsv4_mlpnopad for MLP no-pad kernel evaluation on DSv4"
- "Disable non-DPA, full-DPA, and HiCache sweep arms; keep only DPA no-offload conc [48, 64]"
Comment on lines +5013 to +5018

@claude claude Bot Jul 21, 2026

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.

🔴 This new perf-changelog entry has two issues that will fail the changelog CI gate: it strips the trailing two spaces from the prior entry's blank separator line (violating AGENTS.md's explicit whitespace-preservation rule and breaking validate_raw_change's byte-for-byte prefix check), and the new entry itself omits the required pr-link field that ChangelogEntry (utils/matrix_logic/validation.py) mandates. Please restore the original separator line untouched and add pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2297 (or the XXX placeholder) to the new entry.

Extended reasoning...

This PR's perf-changelog.yaml addition has two concrete, verifiable defects that will fail the changelog validation gate.

1. Whitespace on the separator line was altered. AGENTS.md is explicit: "Never delete or modify whitespace in perf-changelog.yaml - CI depends on exact whitespace (including trailing spaces on blank separator lines). Altering it breaks CI." I diffed the actual git bytes: HEAD~1:perf-changelog.yaml ends with ...pull/2258\n \n — i.e. the blank line separating entries carries two trailing spaces. The PR head replaces that with a bare ...pull/2258\n\n- config-keys:..., stripping the two trailing spaces. This is enforced, not just documented: utils/validate_perf_changelog.py's validate_raw_change() requires head_raw.startswith(base_raw) for the append-only flow, and since the byte immediately after 2258\n changed from a space to the next line's -, that startswith check fails, raising "appended entries changed historical perf-changelog.yaml bytes; restore the base file byte-for-byte and append at the end." This function is invoked from utils/prepare_perf_changelog_merge.py, so it fails at merge-preparation time.

2. The new entry is missing the required pr-link field. ChangelogEntry in utils/matrix_logic/validation.py declares pr_link: str = Field(alias="pr-link") with no default, under model_config = ConfigDict(extra="forbid", populate_by_name=True) — making pr-link a required field with no fallback. The entry appended by this PR (config-keys: dsv4-fp4-mi355x-sglang-agentic-hicache) contains only config-keys and description, no pr-link. Both utils/process_changelog.py (line ~159, called to build the sweep matrix) and utils/validate_perf_changelog.py's parse_changelog call ChangelogEntry.model_validate(...) on every added/parsed entry, so the missing required field raises a Pydantic validation error and aborts before the sweep matrix or changelog validation can complete. Every other entry in the file — 626 of them — carries a pr-link, confirming this is a hard convention, not an optional field that happens to usually be filled in.

Why nothing else catches this before CI: the pr-link placeholder allowance (XXX or the PR's own URL, via PR_LINK_PLACEHOLDERS) only kicks in after model_validate succeeds — omitting the key entirely fails hard before that check ever runs. Likewise the whitespace rule is easy to trip accidentally: any editor/tool that trims trailing whitespace on save, or a manual copy-paste of the tail of the file, will silently drop the two trailing spaces on the separator line, which is exactly what happened here.

Concrete proof:

$ git show HEAD~1:perf-changelog.yaml | tail -c 20 | cat -A
github.com/SemiAnalysisAI/InferenceX/pull/2258$
  $                                    <- blank line, TWO trailing spaces + $
$ tail -c 300 perf-changelog.yaml | cat -A
2258$
$                                       <- blank line, NO trailing spaces (bug 1)
- config-keys:$
    - dsv4-fp4-mi355x-sglang-agentic-hicache$
  description:
    - "..."
    - "..."
                                        <- entry ends here; no pr-link key at all (bug 2)

Feeding this head content through validate_raw_change(base_raw, head_raw) returns head_raw.startswith(base_raw) == False (first divergent byte right after 2258\n), and feeding the new entry dict through ChangelogEntry.model_validate() raises pr_link\n Field required.

Fix: keep the original pull/2258\n \n line byte-for-byte, and append the new entry directly after it with a pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2297 line (or the XXX placeholder used by other in-flight PRs before their PR number is known) added to the new entry's fields.

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2297
2 changes: 1 addition & 1 deletion runners/launch_mi355x-amds.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ else
export GPU_COUNT="${GPU_COUNT:-${TP:?TP must be set}}"

set -x
salloc --partition=$PARTITION --gres=gpu:$GPU_COUNT --exclusive --cpus-per-task=128 --time=500 --no-shell --job-name="$RUNNER_NAME"
salloc --partition=$PARTITION --gres=gpu:$GPU_COUNT --exclusive --cpus-per-task=128 --time=500 --no-shell --job-name="$RUNNER_NAME" --exclude=mia1-p01-g09,mia1-p01-g11
JOB_ID=$(squeue --name="$RUNNER_NAME" -h -o %A | head -n1)

srun --jobid=$JOB_ID bash -c "docker stop \$(docker ps -a -q)"
Expand Down
Loading