[AMD][MI355X] DSv4 SGLang agentic: v0.5.15 image, router, HiCache + MTP#2200
[AMD][MI355X] DSv4 SGLang agentic: v0.5.15 image, router, HiCache + MTP#2200seungrokj wants to merge 4 commits into
Conversation
- Add dsv4_fp4_mi355x_sglang_mtp.sh agentic launcher with EAGLE spec decoding - Add dsv4-fp4-mi355x-sglang-agentic-hicache-mtp config in amd-master.yaml - Sweep TP8 +/- DPA +/- HiCache with MTP (conc 1-96) - Exclude mia1-p01-g12 from agentic salloc - Update perf-changelog Co-Authored-By: Claude Opus 4.6 <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 关于重新运行失败任务的文档 |
…agentic config Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
configs/amd-master.yaml:2351-2354— This diff removesprecision: fp4andframework: sglangfrom the pre-existingdsv4-fp4-mi355x-sglang-agenticentry — unrelated to the new hicache-mtp recipe.SingleNodeMasterConfigEntryrequires both fields (no default,extra='forbid'), so config validation now fails for this entry, breaking sweep-config generation for the entireamd-master.yamlfile. This looks like an accidental deletion; please restore both fields.Extended reasoning...
The bug: the diff to
configs/amd-master.yamldeletesprecision: fp4andframework: sglangfrom the pre-existingdsv4-fp4-mi355x-sglang-agenticentry (lines 2351-2354), while adding the new, unrelateddsv4-fp4-mi355x-sglang-agentic-hicache-mtprecipe elsewhere in the same file. The two fields are not moved or renamed anywhere — they simply vanish from the existing entry.Why it breaks:
utils/matrix_logic/validation.pydefinesSingleNodeMasterConfigEntry(around lines 731-744) with:model_config = ConfigDict(extra='forbid', populate_by_name=True) ... precision: str framework: str
Both fields are required (no default) and the model forbids extras.
validate_master_config()instantiates this model for every non-multinode entry in the merged config, so as soon as it reachesdsv4-fp4-mi355x-sglang-agenticit raises a pydanticValidationError(surfaced asValueError) for the missing fields. Sinceload_config_files(..., validate=True)is called unconditionally inmain()before any sweep/test-config routing happens, this one bad entry aborts config loading for the entireamd-master.yamlfile — not just this recipe, and not just full-sweep generation, but the PR's own new hicache-mtp recipe as well.Confirmed by execution: running
load_config_files(['configs/amd-master.yaml'])against the post-PR repo state reproduces:ValueError: Master config entry 'dsv4-fp4-mi355x-sglang-agentic' failed validation: 2 validation errors for SingleNodeMasterConfigEntry precision: Field required [type=missing] framework: Field required [type=missing]Secondary failure mode (if validation were ever bypassed):
generate_sweep_configs.pyalso does direct dict indexing —precision = val[Fields.PRECISION.value]andframework = val[Fields.FRAMEWORK.value](~lines 393-394, and again ~794-795) — with no.get()/default, so this entry would also raise a plainKeyErrorat the point it's iterated inall_config_data.items().Step-by-step proof:
- Diff removes
precision: fp4/framework: sglangfromdsv4-fp4-mi355x-sglang-agenticwhile leaving every sibling agentic entry (qwen3.5-fp8-mi355x-sglang-agentic,kimik2.5-fp4-mi355x-vllm-agentic,dsv4-fp4-mi355x-vllm-agentic, and the PR's own newdsv4-fp4-mi355x-sglang-agentic-hicache-mtp) with both fields intact — confirming these fields are mandatory and this removal isn't an intentional optionalization. - Any invocation of the config loader/generator against
configs/amd-master.yaml(full-sweep generation, single-config test, or even the PR's own new recipe) callsload_config_files(..., validate=True). validate_master_config()iterates every entry and builds aSingleNodeMasterConfigEntrypydantic model from it.- For
dsv4-fp4-mi355x-sglang-agentic,precisionandframeworkare absent from the raw dict, so pydantic raisesValidationError: precision Field required, framework Field required. - This propagates as a
ValueErrorout ofvalidate_master_config(), aborting the whole config-loading step before any downstream code (including the sweep matrix generation for the PR's new recipe) can run.
Fix: restore
precision: fp4andframework: sglangto thedsv4-fp4-mi355x-sglang-agenticentry, matching every sibling recipe. - Diff removes
| # mia1-p01-g09: pyxis broken (persistently fails to create container filesystem) | ||
| # mia1-p01-g11: docker.sock permissions denied (cluster-cleanup step fails) | ||
| # Both have been root-caused via #1431/#1432/#1440/#1441/#1443 sweep failures. | ||
| salloc --partition=$PARTITION --exclude=mia1-p01-g09,mia1-p01-g11 --gres=gpu:$GPU_COUNT --exclusive --cpus-per-task=128 --time=500 --no-shell --job-name="$RUNNER_NAME" | ||
| salloc --partition=$PARTITION --exclude=mia1-p01-g09,mia1-p01-g11,mia1-p01-g12 --gres=gpu:$GPU_COUNT --exclusive --cpus-per-task=128 --time=500 --no-shell --job-name="$RUNNER_NAME" | ||
| JOB_ID=$(squeue --name="$RUNNER_NAME" -h -o %A | head -n1) | ||
|
|
||
| srun --jobid=$JOB_ID bash -c "docker stop \$(docker ps -a -q)" |
There was a problem hiding this comment.
🟡 The comment block above salloc documents only mia1-p01-g09 and mia1-p01-g11 ("Both have been root-caused via #1431/..."), but the --exclude list now also drops mia1-p01-g12 with no rationale or ticket reference. Purely a documentation staleness issue with no functional impact — worth a one-line update for g12 to match the existing convention.
Extended reasoning...
What's stale: The three-line comment directly above the salloc invocation in runners/launch_mi355x-amds.sh (lines 211-214) explains why two specific nodes are excluded:
# Exclude known-bad mi355x compute nodes (KLAUD_DEBUG §5.1 / §5.2):
# mia1-p01-g09: pyxis broken (persistently fails to create container filesystem)
# mia1-p01-g11: docker.sock permissions denied (cluster-cleanup step fails)
# Both have been root-caused via #1431/#1432/#1440/#1441/#1443 sweep failures.
This PR changes the very next line from --exclude=mia1-p01-g09,mia1-p01-g11 to --exclude=mia1-p01-g09,mia1-p01-g11,mia1-p01-g12, but does not touch the comment block above it.
Concrete walkthrough: A future reader hits this file, sees the exclude list contains three node names (g09, g11, g12), then reads the comment immediately above expecting an explanation for all three. Instead they find only two bullet points (g09, g11) and the closing line "Both have been root-caused via #1431/#1432/#1440/#1441/#1443 sweep failures" — the word "Both" now literally undercounts the exclusions by one, and there is no ticket or root-cause note for g12 at all. The reader is left wondering whether g12's exclusion was intentional, temporary, or accidental, and has no ticket to check for status/removal.
Why nothing catches this automatically: This is a bash comment, not a lint-checked artifact — there's no mechanism that would flag a mismatch between a #-comment and the executable line below it. The salloc command itself is syntactically and functionally correct regardless of the comment's accuracy, so no test or CI check would surface this.
Impact: None functionally — the node is still correctly excluded from the Slurm allocation either way. This is purely a maintainability/documentation issue: the next engineer debugging cluster-runner flakiness loses a bit of context and has to go spelunking (e.g., into perf-changelog.yaml, which does record "Exclude mia1-p01-g12 from agentic salloc" for this PR, but with no root-cause ticket either) to figure out why g12 is on the list.
Suggested fix: Add a third bullet for mia1-p01-g12 (with whatever root-cause/ticket reference is available) and update "Both" to reflect three excluded nodes, e.g.:
# mia1-p01-g09: pyxis broken (persistently fails to create container filesystem)
# mia1-p01-g11: docker.sock permissions denied (cluster-cleanup step fails)
# mia1-p01-g12: <reason/ticket>
# All three have been root-caused via #1431/#1432/#1440/#1441/#1443 sweep failures (or update as appropriate).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29321504588 |
Summary
dsv4_fp4_mi355x_sglang_mtp.shagentic launcher with EAGLE speculative decoding (3 steps, topk 1, 4 draft tokens)dsv4-fp4-mi355x-sglang-agentic-hicache-mtpconfig inamd-master.yamlmia1-p01-g12from agentic sallocTest plan
🤖 Generated with Claude Code