[DIAGNOSTIC] MI355X Kimi-K3 AgentX with no KV transfer config - #2635
[DIAGNOSTIC] MI355X Kimi-K3 AgentX with no KV transfer config#2635sammshen wants to merge 47 commits into
Conversation
Add an lmcache kv-offload-backend point at TP8 conc 10 on top of the existing DSpark MTP serving stack, mirroring the vllm-simple offload arm for a direct backend comparison. The benchmark script gains an lmcache case arm that installs the LMCache 0.5.4rc1 ROCm wheel (torch/ROCm stack untouched), starts one MP server per the Kimi-K3 recipe (chunk size 768 = K3 unified block size at 8 GPUs, --separate-object-groups for the hybrid KDA/MLA two-group KV layout, --enable-extra-logging, --max-cpu-workers 8 --max-gpu-workers 1), and wires vLLM to it via LMCacheMPConnector.
A separate kimik3-fp4-mi355x-vllm-agentic-mtp-lmcache key lets the changelog select only the LMCache points instead of re-running the resident and vllm-simple arms of the base key. The base key returns to its upstream shape.
The LMCache MP server's L1 lives in /dev/shm and the script rejects budgets above 90% of free shm. mi355x-amds nodes mount ~1.5 TB of shm (cap ~1360 GB), so 0.50's 1499 GB budget failed the check in run 31644286169. 0.40 generates ~1199 GB, which fits with margin.
vLLM sizes the K3 unified attention block to 1536 tokens on the MI355X fp8-KV TRITON_MLA path (attention page >= mamba page), and the MP connector asserts chunk %% block == 0, so the recipe's CUDA-path 768 fails engine init (run 31644990546).
The connector requires the chunk to be a multiple of every engine KV group's tokens_per_block. On this stack the hybrid layout registers attention groups at 1536 and a KDA state group at 3072 (run 31645828378), so 1536 fails registration; 3072 is the minimum valid chunk.
Auto mode loads both transfer paths; pin server-driven STORE/RETRIEVE (as the MiniMax-M3 arm does) so the benchmark measures one deterministic path. The L1 stays shm-backed either way, so the /dev/shm capacity check still applies.
The default 300s read-lock TTL expires under a single GPU worker serializing huge K3 transfers: run 31648224111 logged 57k finish-read-on-non-read-locked-key warnings starting exactly at warmup+300s, followed by a GPU illegal-access crash mid-profile. Match the MiniMax-M3 arm's 7200s read TTL.
This reverts commit 20b4fda.
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: ApostaC <yihua98@uchicago.edu>
Signed-off-by: ApostaC <yihua98@uchicago.edu>
Signed-off-by: ApostaC <yihua98@uchicago.edu>
Signed-off-by: ApostaC <yihua98@uchicago.edu>
# Conflicts: # perf-changelog.yaml
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
…SemiAnalysisAI/InferenceX into amd/kimi-k3-agentic-perf-tuning
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: ApostaC <yihua98@uchicago.edu>
Not for merge. Adds kimik3-fp4-mi355x-vllm-agentic-mtp-nokv: identical to the -lmcache key (same image nightly-ac7509e2b1, same script, TP8/EP1, spec-mtp, conc 4/8/10/12/16) with kv-offloading: none, so the run starts no LMCache server and passes no --kv-transfer-config. Purpose: isolate the hipErrorIllegalAddress crashes. They have appeared on mia1-p01-g11 across the LMCache arm, #2602's vllm-simple arm and two LMCache versions -- 0-for-8 on that node -- which points at the node rather than the connector. This runs the same points with the connector absent to confirm that directly. Expected: if the crashes persist with no KV transfer config, they are not LMCache's. If they vanish, the connector is implicated after all.
sammshen
added
agentx
# Conflicts: # perf-changelog.yaml
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
perf-changelog.yaml:5981-5990— This hunk overwrites the existing #2609 changelog entry (minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp/-disagg-mtp) in place instead of appending after it, violating AGENTS.md's append-only invariant and permanently deleting that history. It also uses an invalid pr-link placeholder ('.../pull/PLACEHOLDER') that fails validate_perf_changelog.py's CI gate independently of the history-deletion issue — restore the deleted #2609 block, append the new entry after it, and use the 'XXX' placeholder token.Extended reasoning...
perf-changelog.yaml:5981-5990 breaks two independent rules at once.
Append-only violation / data loss. AGENTS.md states, under "Non-negotiable benchmark invariants," that perf-changelog.yaml "is append-only and byte-sensitive. Preserve all existing bytes and separator whitespace, and append only at the tail." The only hunk touching this file in the PR deletes the existing entry for
config-keys: minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp / -disagg-mtp(added by commit 5b9af26, pr-link.../pull/2609) — both its description lines and its pr-link — and writes the newkimik3-fp4-mi355x-vllm-agentic-mtp-nokventry directly on top of it, including removing a separator blank line. Since this is the only change to the file, that history is not re-added elsewhere; it is simply gone from the committed changelog.Invalid pr-link placeholder. Independent of the deletion, the new entrys pr-link is literally
https://github.com/SemiAnalysisAI/InferenceX/pull/PLACEHOLDER(confirmed by reading the file at HEAD, line 5990).utils/validate_perf_changelog.pydefinesPR_LINK_PLACEHOLDERS = {"XXX", ".../pull/XXX"}andCANONICAL_PR_LINKrequiring.../pull/\d+.validate_added_pr_link()accepts only a link equal to.../pull/<pr_number>on a PR run (here that would be.../pull/2635) or membership inPR_LINK_PLACEHOLDERS."PLACEHOLDER"matches neither, so it raisesChangelogValidationError: new PR entry must use .../pull/2635 or an XXX placeholder. On a main-branch validation run (pr_number=None) the canonical regex also rejects it sincePLACEHOLDERis non-numeric. Either way, the changelog CI gate inrun-sweep.ymlgoes red, andprepare_perf_changelog_merge.pywould independently reject it again at merge-prep time.Why existing code/process does not catch this. Nothing in the diff format itself prevents an in-place overwrite — YAML has no structural append-only enforcement, so this relies entirely on author discipline plus the CI validator. The validator does catch the placeholder issue (that gate is designed for exactly this), but it has no way to detect that history for an unrelated, previously-merged config key vanished, since from its perspective the file is just newly-valid or newly-invalid YAython entries.
Proof, step by step:
- Before this PR (post commit 5b9af26), lines ~5981-5990 held a valid entry:
config-keys: [minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp, minimaxm3-fp4-gb200-dynamo-vllm-agentic-disagg-mtp], two description lines,pr-link: .../pull/2609. - This PRs only changelog hunk replaces those exact lines with the new
kimik3-fp4-mi355x-vllm-agentic-mtp-nokventry — not inserted after, but substituted in place (confirmed viased -n 5975,5995p perf-changelog.yamlon the merged tree, which shows the #2609 config-keys/description/pr-link are simply absent). - Running
utils/validate_perf_changelog.pylogic against this entry:pr_number=2635(this PR),link="https://github.com/SemiAnalysisAI/InferenceX/pull/PLACEHOLDER". Expected canonical value is.../pull/2635;PLACEHOLDERis not inPR_LINK_PLACEHOLDERS = {"XXX", ".../pull/XXX"}and does not equal.../pull/2635, sovalidate_added_pr_linkraisesChangelogValidationError. - Both defects are fixable independently: re-insert the #2609 block before this new entry (restoring append-only-at-the-tail ordering) and change the pr-link to
.../pull/XXX(or the real number2635) to pass the CI gate.
Fix: restore the deleted #2609 config-keys/description/pr-link block, place the new
kimik3-fp4-mi355x-vllm-agentic-mtp-nokventry after it (append at the tail), and change the pr-link to theXXXplaceholder token (or the real PR number) instead ofPLACEHOLDER. - Before this PR (post commit 5b9af26), lines ~5981-5990 held a valid entry:
| agentic-coding: | ||
| - dram-utilization: 0.50 | ||
| search-space: | ||
| - { tp: 8, kv-offloading: none, conc-list: [1, 4, 8] , spec-decoding: mtp} | ||
| - { tp: 8, ep: 1, kv-offloading: dram, kv-offload-backend: { name: vllm-simple }, conc-list: [10], spec-decoding: mtp } | ||
| - { tp: 8, ep: 1, kv-offloading: dram, kv-offload-backend: { name: vllm-simple }, conc-list: [1, 4, 8, 10, 12, 14, 16, 20], spec-decoding: mtp } | ||
|
|
||
| # LMCache MP-server DRAM offload on top of the same DSpark MTP serving stack as | ||
| # kimik3-fp4-mi355x-vllm-agentic-mtp (same image, script, and topology). A | ||
| # dedicated key so LMCache points can be selected and swept without re-running | ||
| # the resident and vllm-simple arms of the base key. | ||
| kimik3-fp4-mi355x-vllm-agentic-mtp-lmcache: | ||
| image: vllm/vllm-openai-rocm:nightly-ac7509e2b1db40fec2f03dde1ed4e9dfdc2338c9 | ||
| model: moonshotai/Kimi-K3 | ||
| model-prefix: kimik3 | ||
| runner: cluster:mi355x-amds | ||
| precision: fp4 | ||
| framework: vllm | ||
| multinode: false | ||
| scenarios: | ||
| agentic-coding: | ||
| - dram-utilization: 0.60 | ||
| search-space: | ||
| - { tp: 8, ep: 1, kv-offloading: dram, kv-offload-backend: { name: lmcache, version: "0.5.4rc4" }, conc-list: [4, 8, 10, 12, 16], spec-decoding: mtp } | ||
|
|
||
| # DIAGNOSTIC ONLY -- not for merge. Identical to | ||
| # kimik3-fp4-mi355x-vllm-agentic-mtp-lmcache (same image, script, topology, | ||
| # concurrencies) with the KV transfer config removed entirely: no LMCache |
There was a problem hiding this comment.
🟡 The base key kimik3-fp4-mi355x-vllm-agentic-mtp no longer has a GPU-resident (kv-offloading: none) arm — this diff collapses it into the single vllm-simple arm — yet the new kimik3-fp4-mi355x-vllm-agentic-mtp-lmcache key's comment still says it exists 'so LMCache points can be selected and swept without re-running the resident and vllm-simple arms of the base key.' That 'resident arm' no longer exists on the base key, so either restore it or drop 'resident' from the comment.
Extended reasoning...
Before this PR, kimik3-fp4-mi355x-vllm-agentic-mtp (configs/amd-master.yaml:635-660 region) had two arms in its search space:
- { tp: 8, kv-offloading: none, conc-list: [1, 4, 8], spec-decoding: mtp}
- { tp: 8, ep: 1, kv-offloading: dram, kv-offload-backend: { name: vllm-simple }, conc-list: [10], spec-decoding: mtp }The first arm (kv-offloading: none) is the GPU-resident baseline. This diff deletes that arm entirely and replaces both with a single extended vllm-simple arm covering conc-list: [1, 4, 8, 10, 12, 14, 16, 20]. So the base key's GPU-resident coverage is gone.
Immediately below, the diff adds a new key, kimik3-fp4-mi355x-vllm-agentic-mtp-lmcache, whose comment reads: "A dedicated key so LMCache points can be selected and swept without re-running the resident and vllm-simple arms of the base key." That comment is describing a base key that no longer matches reality — there is no more "resident arm" on the base key to avoid re-running, since this same diff just removed it.
This is a self-inconsistency introduced entirely within this PR (not a pre-existing issue): the base key's arms and the sibling key's justifying comment are edited in the same diff, and they now contradict each other. Concretely:
- Diff hunk removes
- { tp: 8, kv-offloading: none, conc-list: [1, 4, 8], spec-decoding: mtp}from the base key. - Diff hunk adds the
-lmcachekey a few lines later with the comment referencing "the resident ... arms of the base key." - Reading the base key as merged, there is no resident arm — only the single
vllm-simplearm. - The only GPU-resident (
kv-offloading: none) coverage left anywhere in this file for Kimi-K3 is the diagnostic-only, explicitly-not-for-mergekimik3-fp4-mi355x-vllm-agentic-mtp-nokvkey, at different concurrencies ([4, 8, 10, 12, 16]vs. the old resident arm's[1, 4, 8]).
Two plausible fixes, either is reasonable: (a) if the low-conc GPU-resident baseline was meant to be preserved (e.g. folded into vllm-simple's now-wider conc list intentionally), just fix the stale wording in the -lmcache comment to stop referencing a "resident arm"; or (b) if the removal was accidental, restore the kv-offloading: none arm on the base key.
This has no runtime impact — it's a documentation/comment accuracy issue in a diagnostic-only PR, not something that breaks CI or produces wrong benchmark results — so it doesn't block merge, but a maintainer should clean it up before the comment misleads someone editing this file later.
| _patch() { | ||
| local target="$1" marker="$2" label="$3" | ||
| if [ -z "$target" ] || [ ! -f "$target" ]; then | ||
| echo "[$label] target not found; skipping." | ||
| return 0 | ||
| fi | ||
| if grep -q "$marker" "$target"; then | ||
| echo "[$label] already patched." | ||
| return 0 | ||
| fi | ||
| cp -n "$target" "$target.orig" 2>/dev/null || true | ||
| if $PY - "$target" "$label"; then | ||
| return 0 | ||
| else | ||
| echo "[$label] patch failed; left unchanged." >&2 | ||
| return 0 | ||
| fi | ||
| } |
There was a problem hiding this comment.
🟡 The generic _patch() helper (lines 41-58) is dead code: none of the three real patch functions (patch_aiter_pybind11, patch_triton_mla_cudagraph, patch_kv_blockpool) call it, so each reimplements its own marker-check/backup/heredoc-dispatch logic inline instead. It also doesn't match its own documented OLD/NEW heredoc contract (it just runs $PY - "$target" "$label" with no heredoc), so it couldn't even work as-is if called. Recommend deleting it, or routing the three patchers through a shared helper to remove the triplicated boilerplate.
Extended reasoning...
apply_k3_container_patches.sh defines a generic _patch() function at lines 41-58, documented via a comment at line 39-40 as taking a heredoc body that defines OLD and NEW python strings to apply a marker-gated source patch. Grepping the file for _patch shows it appears exactly twice: once in that doc comment, once in its own definition. There is no call site anywhere else in the script.
Instead, each of the three real patch functions — patch_aiter_pybind11 (line ~71), patch_triton_mla_cudagraph, and patch_kv_blockpool — reimplements the same sequence inline: check whether a marker string is already present via grep -q, back up the target to $target.orig via cp -n, then dispatch to a $PY - "$target" <<'EOF' ... EOF heredoc that does an exact string replace and writes the file back. This is roughly six lines of boilerplate duplicated three times, and a reader encountering _patch() first has to reconcile its contract against three implementations that don't actually use it and that diverge from each other in additional ways (e.g. patch_kv_blockpool explicitly calls out needing a unique marker, unlike the shared-marker assumption implicit in _patch()'s signature).
It's also worth noting _patch() is not just unused but effectively broken relative to its own contract: the doc comment above it says callers pass OLD/NEW via a heredoc, but the function body only ever invokes $PY - "$target" "$label" with no heredoc attached to that specific call — so even if something did call it today, it would not do what the comment describes. That's consistent with it having been orphaned when the three patchers were written directly instead.
There's no functional impact: this is a diagnostic-only script (per the PR description, not for merge as-is) and the dead function is never reached at runtime, so nothing breaks. The fix is purely a maintainability one — either delete _patch() (and its doc comment) since the three patchers have already diverged enough that unifying them isn't free, or actually refactor patch_aiter_pybind11/patch_triton_mla_cudagraph/patch_kv_blockpool to route through a corrected version of _patch() to eliminate the triplicated marker-check/backup/heredoc-dispatch boilerplate.
Proof: grep -n '_patch' benchmarks/single_node/agentic/apply_k3_container_patches.sh returns only line 39 (comment) and line 41 (definition) — zero call sites. Each of patch_aiter_pybind11, patch_triton_mla_cudagraph, and patch_kv_blockpool can be inspected to confirm they each independently perform grep -q "$marker" "$target", cp -n "$target" "$target.orig", and their own $PY - "$target" <<'EOF' ... EOF block rather than calling _patch"$target" "$marker" "$label" <<EOF ... EOF.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=32080861383 |
The branch was cut from kimik3-mi355x-nokv, so it carried that PR's changelog entry as well and the sweep selected both keys -- re-running the five nokv points that already have clean results, under AMD_SERIALIZE_KERNEL=3, on the same MI355X nodes the serialize jobs need. Those runs would be enormously slow and their numbers meaningless. Drop the nokv entry here so only kimik3-fp4-mi355x-vllm-agentic-mtp-serialize sweeps. The entry stays on #2635, which owns it; this removes nothing that exists on main.
|
Diagnostic complete. Result: 5/5 clean (c4/c8/c10/c12/c16) with the KV transfer config removed and DSpark fully active (2.60 mean acceptance), confirming speculative decoding alone does not cause the hipErrorIllegalAddress. Caveat recorded on the Confluence writeup: this arm removes DRAM offload, so it cannot exonerate a node that only faults under offload load. It does NOT rule out the bad-node hypothesis, which the node-placement data now supports (mia1-p01-g11: 8 failures / 8 jobs under offload). Superseded by #2598, which reproduces the crash on the current image. |
4 participants
DIAGNOSTIC ONLY — not for merge
Isolates whether the
hipErrorIllegalAddresscrashes on the MI355X Kimi-K3 AgentX runs come from the LMCache offload connector or from the node.What this changes
Adds
kimik3-fp4-mi355x-vllm-agentic-mtp-nokv, identical tokimik3-fp4-mi355x-vllm-agentic-mtp-lmcache— same image (nightly-ac7509e2b1), same script, TP8/EP1,spec-decoding: mtp, same concurrencies[4, 8, 10, 12, 16]— with one difference:- { tp: 8, ep: 1, kv-offloading: none, conc-list: [4, 8, 10, 12, 16], spec-decoding: mtp }kv-offloading: nonemeans the recipe starts no LMCache MP server and passes no--kv-transfer-config. GPU-resident KV only.Why
hipErrorIllegalAddresshas appeared across:vllm-simplearm, which has no LMCache at all…and every one of those landed on
mia1-p01-g11, which is 0-for-8 today. Other nodes produce clean results with the identical code. That points at the node rather than the connector, but the evidence is observational — this run tests it directly.How to read the result
Either way it also yields a clean GPU-resident baseline on the current image, which is useful for the LMCache comparison in #2598.
Note
Node exclusion cannot currently keep this off
g11:SALLOC_EXCLUDEis exported inbenchmark-tmpl.ymlbut no code consumes it — neithersallocinvocation passes--exclude. Until that's wired up, any point may still draw the bad node.