[AMD] [AGENTX] KIMI Perf Tuning - #3059
Conversation
Signed-off-by: Sirra <asirra@amd.com>
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the 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 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
3 similar comments
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the 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 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the 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 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the 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 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
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>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the KV-offload backend selection path: the old require_agentic_kv_offload_backend validation and the case statement erroring on unsupported KV_OFFLOAD_BACKEND values are gone, but since the new script only ever wires up SimpleCPUOffloadConnector and no longer branches on that variable's value, an unrecognized value is silently ignored rather than causing a functional problem — low-value on its own, not something to call out further.
Extended reasoning...
The inline findings already flag the four confirmed issues (perf-changelog pr-link placeholder mismatch with CI validation, dropped DCP_SIZE divisibility check, dropped TP==8 fail-fast guard, and the CCD-pinning rank/physical-GPU-index assumption). I independently verified one additional candidate from the ruled-out list — the removal of KV_OFFLOAD_BACKEND validation (previously enforced via require_agentic_kv_offload_backend and a case statement erroring on unsupported values) — by diffing the relevant lines. Since the rewritten script hardcodes the SimpleCPUOffloadConnector path and no longer branches on KV_OFFLOAD_BACKEND at all, an invalid value there is inert rather than causing misbehavior, so this is a minor/cosmetic regression (loss of a helpful early error message) rather than a functional bug worth a separate inline comment. Given the four confirmed findings already block approval, and this additional item does not change that calculus, a brief defer note recording what else was examined and ruled out is appropriate.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| - agentic-coding | ||
| description: | ||
| - "Image : vllm/vllm-openai-rocm:nightly-e7edf17cea217e52701f913cd8491fcacf2d9490" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/TBD |
There was a problem hiding this comment.
🔴 New entry's pr-link is "https://.../pull/TBD", but infx/workflows/validate_perf_changelog.py only accepts the literal "XXX" placeholder (or the exact numeric pull/{pr_number} link) — "TBD" matches neither, so the check-changelog CI gate will reject this entry and block the PR from merging. Fix: use "XXX" as the placeholder (per .github/workflows/claude.yml's documented convention and PR_LINK_PLACEHOLDERS in validate_perf_changelog.py), letting the merge tooling canonicalize it to the real PR URL.
Extended reasoning...
validate_perf_changelog.py's validate_added_pr_link() checks the appended entry's pr-link against CANONICAL_PR_LINK (regex pull/\d+) or PR_LINK_PLACEHOLDERS = {"XXX", ".../pull/XXX"}. "TBD" fails the regex (not digits) and is not in the placeholder set, so on this PR's check-changelog run it raises ChangelogValidationError ("new PR entry must use ... or an XXX placeholder; found '.../pull/TBD'"), failing CI until someone edits the entry to XXX or the real PR number.
Verification: normal (mechanism corrected). The core defect is real: perf-changelog.yaml:7418 sets pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/TBD, which is neither a recognized placeholder nor the canonical numeric link. validate_perf_changelog.py:24-27 defines PR_LINK_PLACEHOLDERS = {"XXX", ".../pull/XXX"} only, and validate_added_pr_link (:156-160) raises ChangelogValidationError when…
| case "$CONC" in | ||
| # No KV offload; the working set fits in HBM. | ||
| 1) | ||
| SYNTHETIC_ACCEPT_LEN=3.75 | ||
| SPEC_NUM_TOKENS=6 | ||
| GPU_MEM_UTIL=0.9 | ||
| MAX_NUM_BATCHED_TOKENS=16384 | ||
| ;; | ||
| 4|8|10|12|14) | ||
| SYNTHETIC_ACCEPT_LEN=3.00 | ||
| SPEC_NUM_TOKENS=3 | ||
| GPU_MEM_UTIL=0.9 | ||
| MAX_NUM_BATCHED_TOKENS=8192 | ||
| ;; | ||
| 44|48|52) | ||
| SPEC_NUM_TOKENS=0 | ||
| GPU_MEM_UTIL=0.9 | ||
| MAX_NUM_BATCHED_TOKENS=8192 | ||
| 1|2|4|8) | ||
| DCP_SIZE="${DCP_SIZE:-1}" | ||
| SPEC_NUM_TOKENS="${SPEC_NUM_TOKENS:-4}" | ||
| case "$SPEC_NUM_TOKENS" in | ||
| 1) SYNTHETIC_ACCEPT_LEN=1.85 ;; | ||
| 2) SYNTHETIC_ACCEPT_LEN=2.51 ;; | ||
| 3) SYNTHETIC_ACCEPT_LEN=3.00 ;; | ||
| 4) SYNTHETIC_ACCEPT_LEN=3.36 ;; | ||
| 5) SYNTHETIC_ACCEPT_LEN=3.62 ;; | ||
| 6) SYNTHETIC_ACCEPT_LEN=3.75 ;; | ||
| 7) SYNTHETIC_ACCEPT_LEN=3.84 ;; | ||
| 8) SYNTHETIC_ACCEPT_LEN=4.00 ;; | ||
| *) echo "[spec] no golden AL for k=$SPEC_NUM_TOKENS" >&2; exit 1 ;; | ||
| esac | ||
| DRAFT_KV_DTYPE="${DRAFT_KV_DTYPE:-fp8}" | ||
| SPEC_BASE="\"model\":\"Inferact/Kimi-K3-DSpark\",\"num_speculative_tokens\":$SPEC_NUM_TOKENS,\"method\":\"dspark\",\"attention_backend\":\"TRITON_MLA\",\"kv_cache_dtype\":\"$DRAFT_KV_DTYPE\",\"draft_sample_method\":\"probabilistic\"" | ||
| if [ "${EVAL_ONLY:-false}" = "true" ]; then | ||
| SPEC_ARGS=(--speculative-config "{$SPEC_BASE,\"rejection_sample_method\": \"block\"}") | ||
| echo "MTP: k=$SPEC_NUM_TOKENS LIVE block rejection (accuracy gate) draft_kv=$DRAFT_KV_DTYPE" | ||
| else | ||
| SPEC_ARGS=(--speculative-config "{$SPEC_BASE,\"rejection_sample_method\": \"synthetic\", \"synthetic_acceptance_length\": $SYNTHETIC_ACCEPT_LEN}") | ||
| echo "MTP: k=$SPEC_NUM_TOKENS synthetic_accept=$SYNTHETIC_ACCEPT_LEN draft_kv=$DRAFT_KV_DTYPE" | ||
| fi | ||
| SPEC_ROWS=$(( SPEC_NUM_TOKENS + 1 )) | ||
| KDA_ARGS=(--additional-config '{"kda_prefill_backend":"triton"}') | ||
| MAX_NUM_SEQS="${MAX_NUM_SEQS:-$(( CONC > 2 ? CONC : 2 ))}" | ||
| MAX_BATCHED_TOKENS="${MAX_BATCHED_TOKENS:-16384}" | ||
| ;; | ||
| *) | ||
| SPEC_NUM_TOKENS=0 | ||
| GPU_MEM_UTIL=0.9 | ||
| MAX_NUM_BATCHED_TOKENS=8192 | ||
| DCP_SIZE="${DCP_SIZE:-8}" | ||
| MAX_BATCHED_TOKENS="${MAX_BATCHED_TOKENS:-24576}" | ||
| if [ "$CONC" -lt 72 ]; then MAX_NUM_SEQS="${MAX_NUM_SEQS:-$(( CONC * 14 / 10 ))}" | ||
| elif [ "$CONC" -eq 72 ]; then MAX_NUM_SEQS="${MAX_NUM_SEQS:-96}" | ||
| else MAX_NUM_SEQS="${MAX_NUM_SEQS:-112}"; fi | ||
| ;; | ||
| esac | ||
| export DCP_SIZE |
There was a problem hiding this comment.
🟡 (optional) The pre-diff if [ $((TP % DCP_SIZE)) -ne 0 ]; then exit 1 guard validating that DCP_SIZE divides TP is gone; DCP_SIZE is now set/exported (lines 74, 102, 109) with no divisibility check before being handed to --decode-context-parallel-size. Fix: restore a TP % DCP_SIZE == 0 check (covering both the CONC 1|2|4|8 and default branches) before exporting DCP_SIZE, so a mismatched override fails fast with a clear message instead of reaching vLLM.
Extended reasoning...
Base script had this exact check right after resolving DCP_SIZE. Now if a caller overrides DCP_SIZE (env var, still user-settable) with a value that doesn't evenly divide TP, the script proceeds straight into CP_ARGS+=(--decode-context-parallel-size "$DCP_SIZE" ...) and vllm serve, producing an obscure vLLM-internal startup failure or mis-sharded KV cache instead of the previous clear bash-level error.
Verification: nit. The candidate's factual claim holds: the base script's fail-fast guard if [ $((TP % DCP_SIZE)) -ne 0 ]; then ... exit 1; fi (present right after DCP_SIZE="${DCP_SIZE:-8}" in the pre-diff file) is deleted. In the merged file DCP_SIZE is resolved in the case "$CONC" (${DCP_SIZE:-1} for 1|2|4|8, ${DCP_SIZE:-8} default), then export DCP_SIZE and… | nit. The TP % DCP_SIZE…
|
|
||
| source "$(dirname "$0")/../../benchmark_lib.sh" | ||
|
|
||
| wait_for_amd_gpu_clean |
There was a problem hiding this comment.
🟡 (optional) The hard TP==8 guard ('Kimi-K3 MXFP4 is a 1.56 TB checkpoint and only fits at TP=8') is removed with no replacement, so a TP<8 run no longer fails fast before staging/loading the ~1.56TB checkpoint. Fix: keep a fail-fast check that TP produces enough aggregate HBM to hold the checkpoint (or restore the TP==8 check) so misconfigured TP still errors immediately instead of after a long download/load followed by an OOM. Currently only reachable if a caller sets TP!=8 for this script; configs/amd-master.yaml still only uses tp:8.
Extended reasoning...
Base script exits immediately with a clear message when TP!=8, before hf download/model load. The new script (lines ~1-30) has no TP check at all; TOTAL_RANKS=TP*DP_SIZE is computed and used for offload sizing but never validated against checkpoint size. If any job (env-var override, a future search-space row, or manual run) sets TP=4, the script proceeds through the multi-hundred-GB-per-GPU checkpoint download/load and only fails deep inside vLLM startup with an OOM, wasting a long-running job slot instead of failing in seconds.
Verification: nit. The base branch (diff-removed lines) fast-failed on TP!=8: if [ "$TP" -ne 8 ]; then echo "Error: Kimi-K3 MXFP4 is a 1.56 TB checkpoint and only fits at TP=8 ..." >&2; exit 1; fi. The merged script has no TP validation: TP appears only at line 8 (check_env_vars), line 12 (TOTAL_RANKS=$(( TP * DP_SIZE ))), and line 155 (--tensor-parallel-size "$TP"); TOTAL_RANKS is used solely for…
| try: topo=subprocess.run(["rocm-smi","--showtoponuma"],capture_output=True,text=True).stdout | ||
| except Exception: pass | ||
| gpu_node={int(m.group(1)):int(m.group(2)) for m in re.finditer(r"GPU\[(\d+)\].*?Numa Node:\s*(\d+)",topo)} | ||
| if not gpu_node: raise SystemExit | ||
| by={} | ||
| for d in l3_domains(): by.setdefault(node_of(expand(d)),[]).append(d) | ||
| for n in by: by[n].sort(key=lambda d: expand(d)[0]) | ||
| for n in sorted(by): | ||
| for i,g in enumerate(sorted(k for k,v in gpu_node.items() if v==n)): | ||
| if i < len(by[n]): print(f"{g} {by[n][i]}") | ||
| CCDPY | ||
|
|
||
| PIN_CCD="${PIN_CCD:-1}" | ||
| pin_workers_to_ccd() { | ||
| [ "$PIN_CCD" = "1" ] || return 0 | ||
| [ -s /tmp/ccdmap.txt ] || return 0 | ||
| local pinned=0 | ||
| while read -r _g _cpus; do | ||
| for _p in $(pgrep -f "VLLM::Worker_TP${_g}([^0-9]|$)" 2>/dev/null); do | ||
| for _t in /proc/$_p/task/*; do | ||
| taskset -pc "$_cpus" "${_t##*/}" >/dev/null 2>&1 && pinned=$((pinned+1)) || true | ||
| done | ||
| done | ||
| done < /tmp/ccdmap.txt | ||
| echo "[pin-ccd] pinned $pinned threads" | ||
| } | ||
|
|
||
|
|
||
| wait_for_server_ready --port "$PORT" --server-log "$SERVER_LOG" --server-pid "$SERVER_PID" | ||
|
|
||
| if [ "${EVAL_ONLY}" = "true" ]; then | ||
| pin_workers_to_ccd || true |
There was a problem hiding this comment.
🟡 (optional) pin_workers_to_ccd (default-on via PIN_CCD=1) matches vLLM's logical TP-rank from the process name (VLLM::Worker_TP) against physical GPU indices from rocm-smi --showtoponuma, assuming rank N always equals physical GPU N. rocm-smi enumerates absolute device IDs and ignores HIP/ROCR_VISIBLE_DEVICES, so when that var reorders or subsets GPUs, threads are taskset to the wrong CCD/NUMA node silently, hurting perf instead of helping. Fix: derive the rank->physical-GPU map from the same visibility ordering vLLM actually uses, not rocm-smi's raw index.
Extended reasoning...
Lines 186-219 build /tmp/ccdmap.txt mapping physical GPU id g (from rocm-smi's absolute enumeration, unaffected by HIP_VISIBLE_DEVICES) to a CCD's shared_cpu_list. pin_workers_to_ccd (221-239) then greps for process name VLLM::Worker_TP${_g} and tasksets its threads to that cpu_list, treating _g as both the physical GPU index and the vLLM TP rank. Lines 14-15 show the script explicitly forwards ROCR_VISIBLE_DEVICES into HIP_VISIBLE_DEVICES, which is the mechanism vLLM/ROCm uses to remap logical rank -> physical device (e.g. ROCR_VISIBLE_DEVICES=4,5,6,7,0,1,2,3 or a subset); this pattern recurs in ~15 other benchmark scripts in this tree, confirming it's exercised on this shared cluster. When it reorders devices, rank 0's actual physical GPU is no longer GPU 0, so the taskset call for rank 0 pins its worker threads to GPU 0's NUMA/CCD cpus while the process actually runs on a different physical GPU possibly on another NUMA node -- silently pinning to the wrong (even cross-socket) CPUs with no error surfaced, since pgrep still finds a process named Worker_TP0 to pin. This runs by…
Verification: nit (perf-only, conditional, best-effort/silent). The candidate's read of the new code is accurate. /tmp/ccdmap.txt is built keyed on the absolute GPU index parsed from rocm-smi --showtoponuma output: line 211 gpu_node={int(m.group(1)):int(m.group(2)) for m in re.finditer(r"GPU\[(\d+)\].*?Numa Node:\s*(\d+)",topo)}, and line 217-218 emit print(f"{g} {by[n][i]}") with g = that…
Signed-off-by: Sirra <asirra@amd.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34705085991 |
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
Signed-off-by: Sirra <asirra@amd.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34705085991 |
Signed-off-by: Sirra <asirra@amd.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34733151910 |
2 participants
Kimi Perf Tuning
Image :
vllm/vllm-openai-rocm:nightly-e7edf17cea217e52701f913cd8491fcacf2d9490Note
Medium Risk
Benchmark-only changes, but they alter serving flags, KV offload backend, DCP/MTP behavior, and the published concurrency sweep—results won’t be comparable to prior LMCache arms without re-baselining.
Overview
Retunes Kimi-K3 MXFP4 AgentX on MI355X (vLLM + DSpark MTP) by pinning a newer ROCm vLLM nightly and reshaping both the launch recipe and benchmark matrix.
The
kimik3_fp4_mi355x_mtp.shlauncher is streamlined around measured perf settings: updated AITER/K3 env flags,FULL_DECODE_ONLYCUDA graphs sized from MTP row count, concurrency-aware DCP (1 for low conc with DSpark MTP + golden synthetic acceptance lengths; DCP=8 for higher conc),ROCM_AITER_MLAattention, and extra serve flags (fastsafetensors, fp8 KV,--no-async-scheduling). LMCache provisioning and thelmcacheKV-offload path are removed; DRAM offload now uses onlySimpleCPUOffloadConnectorwith per-rank CPU budget fromTOTAL_RANKS. After the server is up, optional CCD thread pinning maps vLLM TP workers to L3 domains.configs/amd-master.yamlbumpsdram-utilizationto 0.65, replaces LMCache-heavy search arms with no-offload points at conc 1/4/8 and DCP8 +vllm-simpleDRAM offload at 16–80.perf-changelog.yamlrecords the image pin.Reviewed by Cursor Bugbot for commit d349d82. Bugbot is set up for automated code reviews on this repo. Configure here.