Skip to content

[AMD][MI355X] DSv4 SGLang agentic: v0.5.15 image, router, HiCache + MTP#2200

Open
seungrokj wants to merge 4 commits into
mainfrom
amd/agentx_dsv4_sgl_mtp
Open

[AMD][MI355X] DSv4 SGLang agentic: v0.5.15 image, router, HiCache + MTP#2200
seungrokj wants to merge 4 commits into
mainfrom
amd/agentx_dsv4_sgl_mtp

Conversation

@seungrokj

Copy link
Copy Markdown
Collaborator

Summary

  • Add dsv4_fp4_mi355x_sglang_mtp.sh agentic launcher with EAGLE speculative decoding (3 steps, topk 1, 4 draft tokens)
  • Add dsv4-fp4-mi355x-sglang-agentic-hicache-mtp config in amd-master.yaml
  • Sweep TP8 +/- DPA +/- HiCache with MTP across conc 1-96
  • Exclude mia1-p01-g12 from agentic salloc
  • Update perf-changelog

Test plan

  • Full sweep validation on MI355X cluster
  • Verify MTP + HiCache configs launch correctly
  • Verify DPA + router + MTP configs launch correctly

🤖 Generated with Claude Code

- 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>
@github-actions

Copy link
Copy Markdown
Contributor

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 As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

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 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

@seungrokj seungrokj changed the title [AMD][MI355X] DSv4 SGLang: agentic MTP+HiCache recipe [AMD][MI355X] DSv4 SGLang agentic: v0.5.15 image, router, HiCache + MTP Jul 14, 2026
…agentic config

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@claude claude Bot left a comment

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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 configs/amd-master.yaml:2351-2354 — This diff removes precision: fp4 and framework: sglang from the pre-existing dsv4-fp4-mi355x-sglang-agentic entry — unrelated to the new hicache-mtp recipe. SingleNodeMasterConfigEntry requires both fields (no default, extra='forbid'), so config validation now fails for this entry, breaking sweep-config generation for the entire amd-master.yaml file. This looks like an accidental deletion; please restore both fields.

    Extended reasoning...

    The bug: the diff to configs/amd-master.yaml deletes precision: fp4 and framework: sglang from the pre-existing dsv4-fp4-mi355x-sglang-agentic entry (lines 2351-2354), while adding the new, unrelated dsv4-fp4-mi355x-sglang-agentic-hicache-mtp recipe 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.py defines SingleNodeMasterConfigEntry (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 reaches dsv4-fp4-mi355x-sglang-agentic it raises a pydantic ValidationError (surfaced as ValueError) for the missing fields. Since load_config_files(..., validate=True) is called unconditionally in main() before any sweep/test-config routing happens, this one bad entry aborts config loading for the entire amd-master.yaml file — 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.py also does direct dict indexing — precision = val[Fields.PRECISION.value] and framework = val[Fields.FRAMEWORK.value] (~lines 393-394, and again ~794-795) — with no .get()/default, so this entry would also raise a plain KeyError at the point it's iterated in all_config_data.items().

    Step-by-step proof:

    1. Diff removes precision: fp4 / framework: sglang from dsv4-fp4-mi355x-sglang-agentic while 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 new dsv4-fp4-mi355x-sglang-agentic-hicache-mtp) with both fields intact — confirming these fields are mandatory and this removal isn't an intentional optionalization.
    2. 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) calls load_config_files(..., validate=True).
    3. validate_master_config() iterates every entry and builds a SingleNodeMasterConfigEntry pydantic model from it.
    4. For dsv4-fp4-mi355x-sglang-agentic, precision and framework are absent from the raw dict, so pydantic raises ValidationError: precision Field required, framework Field required.
    5. This propagates as a ValueError out of validate_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: fp4 and framework: sglang to the dsv4-fp4-mi355x-sglang-agentic entry, matching every sibling recipe.

Comment thread benchmarks/single_node/agentic/dsv4_fp4_mi355x_sglang_mtp.sh Outdated
Comment on lines 212 to 218
# 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)"

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.

🟡 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).

seungrokj and others added 2 commits July 14, 2026 18:11
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@seungrokj seungrokj added AMD agentx AgentX benchmarks, recipes, and infrastructure full-sweep-fail-fast labels Jul 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentx AgentX benchmarks, recipes, and infrastructure AMD full-sweep-fail-fast

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant