Skip to content

[AMD] fix: add --exclusive to MI355X multi-node sbatch for accurate benchmarks #932

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

Closed
cquil11 wants to merge 4 commits into from
Closed

[AMD] fix: add --exclusive to MI355X multi-node sbatch for accurate benchmarks #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13dd1d4
slurm command fix
seungrokj Mar 23, 2026
e4f812f
update perf changelog
cquil11 Mar 23, 2026
2657489
add --exclusive to mi355x multinode
cquil11 Mar 23, 2026
3328ac6
fix: add --exclusive to MI355X multi-node sbatch for accurate benchmarks
cquil11 Mar 23, 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
1 change: 1 addition & 0 deletions benchmarks/multi_node/amd_utils/submit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ fi
sbatch_cmd=(
sbatch
--parsable
--exclusive
-N "$NUM_NODES"
-n "$NUM_NODES"
"${NODELIST_OPT[@]}"
Expand Down
24 changes: 24 additions & 0 deletions perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,20 @@
- config-keys:
- dsr1-fp4-mi355x-atom
- dsr1-fp4-mi355x-atom-mtp
- dsr1-fp4-mi355x-sglang
- dsr1-fp4-mi355x-sglang-disagg
- dsr1-fp4-mi355x-sglang-disagg-mtp
- dsr1-fp8-mi355x-sglang
- dsr1-fp8-mi355x-sglang-disagg
- dsr1-fp8-mi355x-sglang-disagg-mtp
- gptoss-fp4-mi355x-atom
- gptoss-fp4-mi355x-vllm
- minimaxm2.5-fp8-mi355x-vllm
description:
- "Add --exclusive flag to MI355X multi-node sbatch to prevent node sharing during benchmarks"
- "Only non-TP8 configs are affected; TP8 already uses all GPUs on the node"
pr-link: TBD

Comment on lines +1 to +17

@claude claude Bot Mar 23, 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 first changelog entry has two documentation inaccuracies: pr-link: TBD should be https://github.com/SemiAnalysisAI/InferenceX/pull/932, and the entry description ("multi-node sbatch") incorrectly lists 7 single-node configs (dsr1-fp4-mi355x-atom, dsr1-fp4-mi355x-atom-mtp, dsr1-fp4-mi355x-sglang, dsr1-fp8-mi355x-sglang, gptoss-fp4-mi355x-atom, gptoss-fp4-mi355x-vllm, minimaxm2.5-fp8-mi355x-vllm) that route through salloc, not sbatch. The config-keys list should contain only the 4 -disagg variants affected by the submit.sh change; the single-node configs are already covered by the PR #929 entry at the bottom of the file.

Extended reasoning...

Issue 1 — pr-link: TBD placeholder

The first changelog entry sets pr-link: TBD (line 16) instead of the actual PR URL. The project convention visible throughout the file is either a full URL (https://github.com/SemiAnalysisAI/InferenceX/pull/NNN) or the in-progress placeholder https://github.com/SemiAnalysisAI/InferenceX/pull/XXX. The bare string TBD is the only such occurrence in the entire file. Since this PR is #932, the correct value is https://github.com/SemiAnalysisAI/InferenceX/pull/932.

One verifier noted that the Pydantic model defines pr_link as a plain str with no URL validation, so TBD does pass schema validation. This is accurate — the bug is not tooling-breaking. However, it is a clear inconsistency with every other entry and a straightforward oversight to fix.

Issue 2 — Single-node configs listed under multi-node sbatch description

The only code change in submit.sh adds --exclusive to the sbatch call in the multi-node disaggregated path. Yet the changelog entry lists 11 config keys, 7 of which are single-node configs that never invoke submit.sh:

  • dsr1-fp4-mi355x-atom, dsr1-fp4-mi355x-atom-mtp — use salloc in launch_mi355x-amds.sh (the else branch, confirmed in the diff)
  • dsr1-fp4-mi355x-sglang, dsr1-fp8-mi355x-sglang — likewise salloc in the else branch
  • gptoss-fp4-mi355x-atom, gptoss-fp4-mi355x-vllm, minimaxm2.5-fp8-mi355x-vllm — also single-node salloc path

Only the 4 -disagg configs (dsr1-fp4-mi355x-sglang-disagg, dsr1-fp4-mi355x-sglang-disagg-mtp, dsr1-fp8-mi355x-sglang-disagg, dsr1-fp8-mi355x-sglang-disagg-mtp) actually route through IS_MULTINODE=true into the sbatch path in submit.sh.

Step-by-step proof:

  1. In launch_mi355x-amds.sh, the top-level if [[ "$IS_MULTINODE" == "true" ]] branch calls bash "benchmarks/multi_node/${SCRIPT_NAME}", which invokes submit.sh containing the new --exclusive flag.
  2. All other configs fall into the else branch which calls salloc --partition=$PARTITION --gres=gpu:$TP --exclusive ... — this salloc --exclusive flag was already added by PR [AMD] Fix slurm command for AMD devices #929.
  3. The PR [AMD] Fix slurm command for AMD devices #929 entry at the bottom of perf-changelog.yaml uses "*mi355x*" and "*mi325x*" wildcards, which already match all single-node MI355X configs including every one incorrectly listed in the first entry.
  4. Therefore, including single-node configs in the first entry not only has an inaccurate description but also duplicates coverage already provided by the PR [AMD] Fix slurm command for AMD devices #929 entry.

Impact and fix:

Both issues are documentation-only — the actual --exclusive flag in submit.sh is correctly placed and functional. The fix is: (1) update pr-link: TBD to https://github.com/SemiAnalysisAI/InferenceX/pull/932, and (2) remove the 7 single-node config keys from the first entry, leaving only the 4 -disagg configs.

- config-keys:
- dsr1-fp8-b200-dynamo-trt
- dsr1-fp8-h200-dynamo-trt
Expand Down Expand Up @@ -1007,3 +1024,10 @@
- "EAGLE speculative decoding: num-steps 3, draft-tokens 4, topk 1"
- "New script: benchmarks/single_node/qwen3.5_fp8_b200_mtp.sh"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/898

- config-keys:
- "*mi355x*"
- "*mi325x*"
description:
- "Add --exclusive option to salloc commands on MI325X and MI355X nodes which may yield performance improvements"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/929
2 changes: 1 addition & 1 deletion runners/launch_mi325x-amd.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ LOCK_FILE="${SQUASH_FILE}.lock"

set -x

JOB_ID=$(salloc --partition=$PARTITION --gres=gpu:$TP --cpus-per-task=256 --time=480 --no-shell --job-name="$RUNNER_NAME" 2>&1 | tee /dev/stderr | grep -oP 'Granted job allocation \K[0-9]+')
JOB_ID=$(salloc --partition=$PARTITION --gres=gpu:$TP --exclusive --cpus-per-task=256 --time=480 --no-shell --job-name="$RUNNER_NAME" 2>&1 | tee /dev/stderr | grep -oP 'Granted job allocation \K[0-9]+')

if [ -z "$JOB_ID" ]; then
echo "ERROR: salloc failed to allocate a job"
Expand Down
2 changes: 1 addition & 1 deletion runners/launch_mi355x-amds.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ else
LOCK_FILE="${SQUASH_FILE}.lock"

set -x
salloc --partition=$PARTITION --gres=gpu:$TP --cpus-per-task=128 --time=180 --no-shell --job-name="$RUNNER_NAME"
salloc --partition=$PARTITION --gres=gpu:$TP --exclusive --cpus-per-task=128 --time=180 --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)"
Expand Down