Skip to content

[AMD] Qwen3.5-FP8 MI355X SGLang disagg baseline: image bump to v0.5.16, TP4P+TP8D / Qwen3.5-FP8 MI355X SGLang disagg 基线优化：镜像升级至v0.5.16，TP4P+TP8D拓扑优化 #2606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChangLiu0709 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[AMD] Qwen3.5-FP8 MI355X SGLang disagg baseline: image bump to v0.5.16, TP4P+TP8D / Qwen3.5-FP8 MI355X SGLang disagg 基线优化：镜像升级至v0.5.16，TP4P+TP8D拓扑优化 #2606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c964ef
[AMD] Qwen3.5-FP8 MI355X SGLang disagg baseline: image bump v0.5.16, …
ChangLiu0709 Aug 14, 2026
28e5ddc
Merge branch 'main' into chang/qwen3.5-mi355-di-baseline
ChangLiu0709 Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions benchmarks/multi_node/amd_utils/docker_sg.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env bash
# Run `docker` under the 'docker' group.
#
# Why: Slurm launches job steps without the user's 'docker' supplementary group
# in the active credential set. The user IS a docker-group member (getent group
# docker lists them) but the group is missing from `id -G` inside the step, so
# the group-owned socket (/var/run/docker.sock, 0660 root:docker) is unreachable
# via a plain `docker` call. `sg docker -c` re-activates the group for this one
# command — no sudo, no password, no persistent host change.
#
# Used by job.slurm's DOCKER_CMD detection as the fallback when plain `docker`
# fails but `sg docker -c 'docker ps'` succeeds.
#
# argv is passed across the `sg` shell hop by NUL-delimited base64 (NOT string
# re-quoting): naive `printf %q` mangles the big multiline `docker run ... bash
# -lc '<script>'` argument (trailing newline became a literal 'n', spawning a
# stray `$n`). base64 round-trips arbitrary bytes (newlines, quotes) exactly,
# and only the base64 blob (safe chars) is interpolated into the sg command.
b64=$(printf '%s\0' "$@" | base64 | tr -d '\n')
exec sg docker -c "bash -c 'mapfile -d \"\" -t __A < <(printf %s \"$b64\" | base64 -d); exec docker \"\${__A[@]}\"'"
48 changes: 45 additions & 3 deletions benchmarks/multi_node/amd_utils/job.slurm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,11 @@ SELECTED_NODELIST_STR=$(echo "$SELECTED_NODES" | tr '\n' ',' | sed 's/,$//')

# Docker privilege detection — evaluated per-node since group membership varies.
# Exported as a snippet so every srun participant resolves it locally.
export DOCKER_CMD_DETECT='if docker ps &>/dev/null 2>&1; then DOCKER_CMD=docker; else DOCKER_CMD="sudo docker"; fi'
# Order: (1) plain docker if the socket is reachable; (2) sg-docker wrapper if the
# user is a docker-group member but Slurm didn't activate the group in the step's
# credentials (common here); (3) sudo docker as a last resort.
SG_DOCKER_WRAPPER="${DI_REPO_DIR}/benchmarks/multi_node/amd_utils/docker_sg.sh"
export DOCKER_CMD_DETECT="if docker ps &>/dev/null 2>&1; then DOCKER_CMD=docker; elif sg docker -c 'docker ps' &>/dev/null 2>&1; then DOCKER_CMD='${SG_DOCKER_WRAPPER}'; else DOCKER_CMD='sudo docker'; fi"

# Update SLURM environment variables
export SLURM_NNODES=$NUM_NODES
Expand Down Expand Up @@ -382,7 +386,7 @@ cleanup() {
# on every allocated node. Scoped to $DOCKER_CONT_NAME so it never touches
# other users' containers. (Ported from InferenceY 51ebfa88.)
srun --nodelist="$SELECTED_NODELIST_SRUN" \
bash -c 'eval "$DOCKER_CMD_DETECT"; $DOCKER_CMD rm -f '"$DOCKER_CONT_NAME"' 2>/dev/null || true' 2>/dev/null || true
bash -c 'eval "$DOCKER_CMD_DETECT"; $DOCKER_CMD rm -f '"$DOCKER_CONT_NAME"' 2>/dev/null || true; _pid=$($DOCKER_CMD inspect --format "{{.State.Pid}}" '"$DOCKER_CONT_NAME"' 2>/dev/null || true); if [ -n "$_pid" ] && [ "$_pid" != "0" ]; then sudo kill -9 "$_pid" 2>/dev/null || true; sleep 2; $DOCKER_CMD rm -f '"$DOCKER_CONT_NAME"' 2>/dev/null || true; fi' 2>/dev/null || true
rm -rf ${SLURM_SUBMIT_DIR}/logs 2>/dev/null || true
echo "[${SLURM_JOB_ID}] cleanup done."
}
Expand Down Expand Up @@ -584,6 +588,14 @@ if [[ -n "${CLIENT_IMAGE:-}" ]]; then
srun --nodelist="$SELECTED_NODELIST_SRUN" bash -c 'eval "$DOCKER_CMD_DETECT"; $DOCKER_CMD pull '"$CLIENT_IMAGE"' >/dev/null 2>&1 || true' 2>/dev/null || true
fi

# Pre-pull the main Docker image on every node so the container creation
# barrier (300s) doesn't race against a multi-GB image download on nodes
# where the image isn't cached. Best-effort: failure here is non-fatal
# since docker run will pull as a fallback.
echo "[pre-pull] Pulling $DOCKER_IMAGE_NAME on all nodes..."
srun --nodelist="$SELECTED_NODELIST_SRUN" \
bash -c 'eval "$DOCKER_CMD_DETECT"; echo "[pre-pull] $(hostname): pulling..."; $DOCKER_CMD pull '"$DOCKER_IMAGE_NAME"' && echo "[pre-pull] $(hostname): done" || echo "[pre-pull] $(hostname): pull failed (will retry on docker run)"' || true

srun \
--nodelist="$SELECTED_NODELIST_SRUN" \
--kill-on-bad-exit=1 \
Expand Down Expand Up @@ -681,6 +693,23 @@ fi # end: if ENGINE == atom-disagg
\$DOCKER_CMD ps -aq --filter \"$CONT_FILTER\" | xargs -r \$DOCKER_CMD rm -f || true
\$DOCKER_CMD ps -aq | xargs -r \$DOCKER_CMD stop -t 15 || true
\$DOCKER_CMD ps -aq | xargs -r \$DOCKER_CMD rm -f || true

# Fallback for stuck containers: if docker rm -f fails (\"did not receive an
# exit event\"), the container's shim/init PID is stuck in uninterruptible
# sleep (common after a GPU hang). Kill the container PIDs directly so the
# daemon can clean up the cgroup and release GPU VRAM.
_stuck=\$(\$DOCKER_CMD ps -aq 2>/dev/null || true)
if [[ -n \"\$_stuck\" ]]; then
echo \"[pre-clean] containers still present after docker rm -f; attempting direct PID kill\"
for _cid in \$_stuck; do
_pid=\$(\$DOCKER_CMD inspect --format '{{.State.Pid}}' \"\$_cid\" 2>/dev/null || true)
if [[ -n \"\$_pid\" && \"\$_pid\" != \"0\" ]]; then
sudo kill -9 \"\$_pid\" 2>/dev/null || true
fi
done
sleep 3
\$DOCKER_CMD ps -aq | xargs -r \$DOCKER_CMD rm -f 2>/dev/null || true
fi
sleep 2

# GPU sanity gate: containers are stopped, so any remaining VRAM use is a bare
Comment on lines 693 to 715

@claude claude Bot Aug 14, 2026

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 'docker inspect Pid → guard nonzero → sudo kill -9 → sleep → docker rm -f' stuck-container cleanup sequence is written out three separate times in this PR (cleanup() trap ~L386-392, pre-clean fallback ~L693-715, and final KEEP_CONTAINERS cleanup ~L807-826), each with slightly different quoting/looping/sleep duration. Consider factoring this into a shared helper under amd_utils/helpers/ (following the existing gpu_sanity.sh/rdma_check.sh pattern) parameterized by container name(s), so a future change to the kill/retry policy only needs to be made once.

Extended reasoning...

This PR introduces the same "if docker rm -f didn't work, read the container's PID via docker inspect --format {{.State.Pid}}, guard it's nonzero, sudo kill -9 it, sleep, then retry docker rm -f" recipe in three distinct places in job.slurm:

  1. cleanup() trap (~L386-392): the diff appends the inspect/kill-9/sleep/rm-f tail onto the existing srun ... $DOCKER_CMD rm -f $DOCKER_CONT_NAME one-liner, all inline inside a single-quoted bash -c string with sleep 2.
  2. Pre-clean stuck-container fallback (~L693-715, entirely new): a for _cid in $_stuck loop inside the big bash -lc "..." heredoc (double-quote-escaped), inspecting/killing each container returned by docker ps -aq, with sleep 3.
  3. Final KEEP_CONTAINERS cleanup (~L807-826): the old one-line rm -f $DOCKER_CONT_NAME $CLIENT_CONT_NAME is rewritten into a for _cont in ... loop that does rm -f, inspect, sudo kill -9, sleep 2, rm -f again per container name.

The inner "inspect → nonzero-check → kill -9 → sleep → rm -f" core is functionally identical across all three sites — only the outer scoping differs (single named container vs. a for loop over container names vs. a for loop over docker ps -aq output), and even the sleep duration silently drifts (2s in two spots, 3s in the third) despite being logically the same "give the kernel time to release the PID before retrying rm" step.

The problem this causes: any future change to the kill/retry policy — e.g. escalating from SIGKILL to a signal sequence, tuning the sleep duration, changing how a nonzero-but-stale PID is detected, or adding a log line when the kill actually fires — has to be hand-applied in three separate places, each with different quoting contexts (plain single-quoted bash -c, an escaped heredoc, and a here-string). It's easy for these to drift further over time (the 2s vs 3s sleep is already an example of this happening within the same PR), and a subtle bug fixed in one copy is likely to be missed in the other two.

Existing precedent in this same file for factoring shared shell logic already exists: gpu_sanity.sh and rdma_check.sh live under amd_utils/helpers/ and are invoked via bash $DI_REPO_DIR/.../helpers/<script>.sh from within the job. A similar helper (e.g. helpers/kill_stuck_container.sh), parameterized by container name(s) and invoked the same way, would let all three call sites share one implementation. The differing escaping contexts (plain bash -c, escaped heredoc, here-string) are a real complication but not a blocker — since the helper would be its own file invoked via bash .../helper.sh "$cont1" "$cont2" ..., each call site only needs to pass the container name(s) as argv, sidestepping the quoting divergence that currently forces three hand-written inline copies.

As a concrete illustration: suppose the kill/retry policy needs to change from sleep 2 to sleep 5 to give slower NICs/GPUs more time to release the PID. Today that edit must be made in the cleanup() trap, the pre-clean fallback (which uses sleep 3, a different existing value, adding confusion about whether it should also become 5), and the final KEEP_CONTAINERS loop — three edits across two different quoting styles, with no test coverage to catch a missed copy. With a shared helper, it's one line in one file.

This is a maintainability/quality observation only — the duplicated logic is not incorrect in any of its three call sites, so it does not affect correctness or block merging.

Expand Down Expand Up @@ -778,7 +807,20 @@ exit \$DOCKER_EXIT_CODE
"

if [[ "${KEEP_CONTAINERS}" != "1" ]]; then
srun --nodelist="$SELECTED_NODELIST_SRUN" bash -c 'eval "$DOCKER_CMD_DETECT"; $DOCKER_CMD rm -f '"$DOCKER_CONT_NAME"' '"$CLIENT_CONT_NAME"' 2>/dev/null || true'
srun --nodelist="$SELECTED_NODELIST_SRUN" bash -c '
eval "$DOCKER_CMD_DETECT"
for _cont in '"$DOCKER_CONT_NAME"' '"$CLIENT_CONT_NAME"'; do
$DOCKER_CMD rm -f "$_cont" 2>/dev/null || true
if $DOCKER_CMD inspect "$_cont" &>/dev/null; then
_pid=$($DOCKER_CMD inspect --format "{{.State.Pid}}" "$_cont" 2>/dev/null || true)
if [ -n "$_pid" ] && [ "$_pid" != "0" ]; then
sudo kill -9 "$_pid" 2>/dev/null || true
sleep 2
$DOCKER_CMD rm -f "$_cont" 2>/dev/null || true
fi
fi
done
' || true

# Clean up vLLM external router container on node 0
if [[ "$ENGINE" == "vllm-disagg" && "$ROUTER_TYPE" == "vllm-router" ]]; then
Expand Down
2 changes: 1 addition & 1 deletion benchmarks/multi_node/amd_utils/submit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ fi
# Optional: exclude specific nodes (e.g. nodes with broken Docker sockets).
# Set SLURM_EXCLUDE_NODES env var to a comma-separated list of hostnames.
EXCLUDE_OPT=()
SLURM_EXCLUDE_NODES="${SLURM_EXCLUDE_NODES:-mia1-p01-g09,mia1-p01-g10,mia1-p01-g11,mia1-p01-g12}"
SLURM_EXCLUDE_NODES="${SLURM_EXCLUDE_NODES:-mia1-p01-g09,mia1-p01-g10,mia1-p01-g11,mia1-p01-g12,mia1-p01-g14,mia1-p01-g16,mia1-p01-g17,mia1-p01-g19,mia1-p01-g31,mia1-p01-g37}"
if [[ -n "${SLURM_EXCLUDE_NODES:-}" ]]; then
EXCLUDE_OPT=(--exclude "$SLURM_EXCLUDE_NODES")
fi
Expand Down
19 changes: 9 additions & 10 deletions configs/amd-master.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ qwen3.5-fp8-mi355x-atom-mtp:
- { tp: 8, ep: 1, conc-start: 4, conc-end: 256, spec-decoding: mtp }

qwen3.5-fp8-mi355x-sglang-disagg:
image: lmsysorg/sglang:v0.5.14-rocm720-mi35x
image: lmsysorg/sglang:v0.5.16-rocm720-mi35x
model: Qwen/Qwen3.5-397B-A17B-FP8
model-prefix: qwen3.5
runner: mi355x-disagg
Expand All @@ -298,21 +298,20 @@ qwen3.5-fp8-mi355x-sglang-disagg:
- isl: 8192
osl: 1024
search-space:
# 1P+1D TP8/EP1 low-concurrency sweep.
# dp-attn intentionally false (matches the 1k1k row): with
# --enable-dp-attention + --moe-a2a-backend mori, sglang auto-promotes
# moe_ep_size=tp_size=8, but is_deepep_class_backend() excludes MoRI,
# so num_shared_slots stays at the global value (1) and the
# 1P+1D TP4P+TP8D/EP1 baseline (no speculative decoding).
# TP4 prefill saves 4 GPUs vs TP8P while delivering identical decode
# interactivity and 24-31% better throughput/GPU (12 vs 16 GPUs).
# dp-attn intentionally false: with --enable-dp-attention +
# --moe-a2a-backend mori, sglang auto-promotes moe_ep_size=tp_size,
# but is_deepep_class_backend() excludes MoRI, so
# num_shared_slots stays at the global value (1) and the
# (num_experts - num_shared_slots) % moe_ep_size assertion in
# fused_moe_triton/layer.py fires for Qwen3.5 (512 routed + 1 shared).
# Track upstream sglang for a fix; flip back to dp-attn=true once
# MoRI is added to is_deepep_class_backend() or shared-slot
# accounting is reconciled.
- spec-decoding: "none"
conc-list: [ 8, 16, 32, 64, 128 ]
prefill:
num-worker: 1
tp: 8
tp: 4
ep: 1
dp-attn: false
additional-settings:
Expand Down
7 changes: 7 additions & 0 deletions perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5960,3 +5960,10 @@
- "Use native EAGLE MTP (3 steps, top-k 1, 4 draft tokens) and golden synthetic acceptance length 2.49 for throughput; eval retains real verification."
- "Follow the official SGLang DeepSeek-V4 Blackwell recipe, require nonempty SGLang server metrics, keep pooled AgentX connections alive, let AIPerf own HiCache warmup, and reserve transient MoE workspace at DEP8 c512."
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2577

- config-keys:
- qwen3.5-fp8-mi355x-sglang-disagg
description:
- "Bump qwen3.5-fp8-mi355x-sglang-disagg image from v0.5.14 to v0.5.16 (3-5% throughput improvement) and switch from TP8P+TP8D (16 GPU) to TP4P+TP8D (12 GPU) — TP4 prefill delivers identical decode interactivity with 24-31% better throughput/GPU."
- "Infra: add docker_sg.sh wrapper and update job.slurm DOCKER_CMD_DETECT to use sg-docker fallback for nodes where Slurm doesn't activate the docker supplementary group."
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2557
Comment on lines +5966 to +5969

@claude claude Bot Aug 14, 2026

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 new perf-changelog.yaml entry for qwen3.5-fp8-mi355x-sglang-disagg sets pr-link to pull/2557, but this change is being made in PR #2606 — #2557 is the parent PR this one was split from, not the PR adding the entry. Per docs/configuration-procedures.md, pr-link must point to the PR that actually adds the entry, so this should be pull/2606.

Extended reasoning...

The newly appended changelog block (config-key qwen3.5-fp8-mi355x-sglang-disagg, perf-changelog.yaml:5959-5962) sets:

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2557

but this changelog entry is being added by this PR, which is #2606 (per the PR metadata). #2557 is explicitly identified in the PR description itself as the parent PR this change was "split from" ("Split from #2557 — baseline only, no MTP/speculative decoding changes") — it is not the PR that introduces this entry.

Why this violates the documented convention: docs/configuration-procedures.md (the template around line 251, plus the guidance at line 254) states that pr-link must be the real URL of the PR that adds the entry, and that pr-link: TBD is only acceptable before the PR exists — to be "replace[d] with the real URL immediately after creating the PR." Every other existing entry in perf-changelog.yaml follows this rule and links to its own introducing PR number (e.g., the three entries immediately preceding this one link to #2577, #2578, and #2580 respectively — each entry's own PR, not a predecessor). This new entry breaks that pattern by pointing to the PR it was split from instead of itself.

Why nothing catches this today: CI's check-changelog job only validates that the changelog is append-only (per the PR's own test plan checklist: "CI passes check-changelog (append-only)"). It does not validate that pr-link resolves to the correct/current PR number, so this mismatch will pass CI silently.

Step-by-step proof:

  1. PR metadata says number=2606 — this is the PR under review, and it is the PR that appends the new changelog block (the diff shows the block added directly under this PR's diff of perf-changelog.yaml).
  2. The PR description states: "Split from [AMD] Qwen3.5-FP8 MI355X SGLang disagg perf tuning: image bump to v0.5.16, TP4P+TP8D baseline, add MTP / Qwen3.5-FP8 MI355X SGLang disagg 性能调优：镜像升级至v0.5.16，TP4P+TP8D基线优化，新增MTP配置 #2557 — baseline only, no MTP/speculative decoding changes," confirming [AMD] Qwen3.5-FP8 MI355X SGLang disagg perf tuning: image bump to v0.5.16, TP4P+TP8D baseline, add MTP / Qwen3.5-FP8 MI355X SGLang disagg 性能调优：镜像升级至v0.5.16，TP4P+TP8D基线优化，新增MTP配置 #2557 is a different, earlier PR that this one was carved out of.
  3. The new block's pr-link field reads https://github.com/SemiAnalysisAI/InferenceX/pull/2557.
  4. Per the documented rule, this field should read https://github.com/SemiAnalysisAI/InferenceX/pull/2606 — the PR actually introducing this changelog entry.
  5. Comparing to sibling entries a few lines above (linking to Refresh DeepSeek-V4 B300 SGLang AgentX MTP #2577, Refresh DeepSeek-V4 B200 SGLang AgentX MTP #2578, [AMD][AgentX] Add MI300X MiniMax-M3 EAGLE3 MXFP8 #2580 — each their own introducing PR) confirms the established, correct pattern that this entry deviates from.

Impact: Purely a provenance/traceability metadata issue. It doesn't break CI, doesn't affect the benchmark run, and doesn't change runtime behavior — but it does make the changelog's PR attribution incorrect for anyone auditing history (a reader following the link from this entry would land on the older, unrelated split-parent PR rather than this one).

Fix: Change pr-link in the new block from .../pull/2557 to .../pull/2606.

Loading