[AMD][AgentX] Kimi-K3 MI355X DSpark wrapper (kimik3_fp4_mi355x_mtp.sh) - #2509
[AMD][AgentX] Kimi-K3 MI355X DSpark wrapper (kimik3_fp4_mi355x_mtp.sh)#2509hyukjlee wants to merge 1 commit into
Conversation
Split out of #2403 as a standalone change. The MI355X launcher derives the benchmark script name from spec-decoding, not from the config key, so a spec-decoding=mtp row resolves to the _mtp suffix. This wrapper pins the upstream AMD Kimi-K3 DSpark reproducer values and execs the shared base recipe, which owns the model, AgentX replay and eval plumbing. DEPENDS ON #2403: this script execs benchmarks/single_node/agentic/ kimik3_fp4_mi355x.sh, which does not exist on main yet. Merge #2403 first, or this is a dangling reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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 关于重新运行失败任务的文档 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — trivial, self-contained wrapper script. The one inline nit (missing bilingual title/description per AGENTS.md) is a process-convention issue, not a code bug.
Extended reasoning...
Overview
The PR adds a single new 21-line bash script, benchmarks/single_node/agentic/kimik3_fp4_mi355x_mtp.sh. It exports a fixed set of env vars (SPEC_DECODE, KV_CACHE_DTYPE, GPU_MEM_UTIL, MAX_NUM_SEQS, EVAL_MAX_NUM_SEQS, MAX_NUM_BATCHED_TOKENS, LANGUAGE_MODEL_ONLY, PREFIX_CACHING, ENFORCE_EAGER), each with a ${VAR:-default} fallback matching the values documented in the PR's knob table, then execs the sibling base script kimik3_fp4_mi355x.sh, forwarding all args. bash -n is clean and I confirmed it locally. No other files are touched.
Security risks
None. The script only sets environment variables with static literal defaults and execs a sibling script by relative path derived from $(dirname "$0"); there is no user-controlled input, network access, or credential handling.
Level of scrutiny
Low. This is a config-only wrapper following the env-var-override-then-exec idiom used elsewhere in the repo's benchmark recipes (e.g. ${DRAFT_MODEL:-...} defaulting in sibling kimik3 scripts). The logic is straightforward enough that a human reviewer would only need to sanity-check the pinned values against the upstream AMD DSpark reproducer, which the PR description already documents in a table.
Other factors
The PR explicitly discloses that it depends on unmerged PR #2403 (which adds the base script kimik3_fp4_mi355x.sh this wrapper execs) and states it cannot merge first — I confirmed kimik3_fp4_mi355x.sh does not exist in the current tree, consistent with that disclosure. This is a merge-sequencing concern the author has already surfaced prominently, not a hidden defect, so it doesn't change my assessment of the code itself. The one bug found (missing bilingual PR title/description per AGENTS.md) is a minor process nit with no runtime effect.
| @@ -0,0 +1,21 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
🟡 The PR title and description are English-only, but AGENTS.md requires every PR title/description to be bilingual (title format <English title> / <中文标题>, plus a ## 中文说明 section in the body).
Extended reasoning...
AGENTS.md (top of the repo's AI-agent guidance, line 7) states explicitly:
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, matching the bilingual docs rule in Code Conventions.
This PR's title, [AMD][AgentX] Kimi-K3 MI355X DSpark wrapper (kimik3_fp4_mi355x_mtp.sh), has no trailing / <中文标题> segment, and the body (Summary table + "## 中文说明 section or any Chinese text at all.
Step-by-step proof:
- Open AGENTS.md and read the block starting
> **PR and GitHub-issue titles & descriptions must be bilingual...**— it unambiguously requires a<中文标题>suffix on every PR title and a Chinese-language section in every PR body. - Read this PR's title in the metadata:
[AMD][AgentX] Kimi-K3 MI355X DSpark wrapper (kimik3_fp4_mi355x_mtp.sh)— no/followed by Chinese characters anywhere. - Read the PR description: it contains only
## Summary, a markdown table, and## ⚠️ Merge order, all in English. There is no## 中文说明heading and no Chinese-script characters anywhere in the body. - Therefore the PR violates the explicit "applies to every PR and every issue" clause with no exception applicable here (the only stated exception is for the CODEOWNER sign-off template and bot-generated comments, neither of which applies to the PR author's own title/description).
This is a repo-mandated process convention, not a subjective style preference, so it's worth flagging even though it has zero effect on the shell script's runtime correctness (the bash -n-clean wrapper script itself is fine). It does not block the DSpark wrapper logic from working, so it shouldn't hold up merge — the author can simply add a <中文标题> to the title and append a ## 中文说明 section summarizing the change in Chinese.
1 participant
Summary
Standalone split of
benchmarks/single_node/agentic/kimik3_fp4_mi355x_mtp.shout of #2403.The MI355X launcher derives the benchmark script name from
spec-decoding, not from the config key, so aspec-decoding: mtprow resolves to the_mtpsuffix. This wrapper pins the upstream AMD Kimi-K3 DSpark reproducer values andexecs the shared base recipe, which owns the model, AgentX replay and eval plumbing.SPEC_DECODEtrueKV_CACHE_DTYPEauto→ bf16 (base recipe defaults tofp8; this overrides it)GPU_MEM_UTIL0.95MAX_NUM_SEQS/EVAL_MAX_NUM_SEQS16/128MAX_NUM_BATCHED_TOKENS4096PREFIX_CACHING/ENFORCE_EAGER/LANGUAGE_MODEL_ONLYauto/false/falseThis depends on #2403 and cannot merge first. The wrapper
execsbenchmarks/single_node/agentic/kimik3_fp4_mi355x.sh, which does not exist onmain— #2403 adds it. Landing this alone leaves a dangling reference.bash -nclean.