Conversation
|
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 关于重新运行失败任务的文档 |
8d65951 to
2c09431
Compare
|
Claude finished @cquil11's task in 12m 0s —— View job PR Review
SummaryFound 2 blocking issues and 1 warning (posted as inline comments):
Checked and clean: enroot |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - config-keys: | ||
| - dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp | ||
| description: | ||
| - "Port the merged MI355X DeepSeek V4 AgentX MTP 1P1D search space from the legacy AMD multinode launcher to srt-slurm." | ||
| - "Use srt-slurm's native SGLang Router frontend and AMD MoRI transport without Dynamo, NATS, etcd, or NIXL." | ||
| - "Preserve TP8/EP1 c1,c2,c4,c8,c16,c32 and TP8/EP8/DPA c96,c128, EAGLE MTP, the golden simulated average length, HiCache settings, and logical prefill/decode server-metrics collection through the custom AgentX benchmark." | ||
| - "Remove the superseded DSV4-specific amd_utils benchmark wrapper, model stanza, HiCache override plumbing, and hard-coded synthetic-acceptance injection; the srt-slurm recipes now own those settings directly." | ||
| - "Allocate all 128 logical CPUs per MI355X backend node so SGLang TP8 affinity follows the complete host topology, and pin srt-slurm diagnostics that preserve failure-log tails containing non-UTF-8 ROCm compiler output." | ||
| - "Move the known-green DeepSeek V4 ROCm kernel-routing contract into each self-contained srt-slurm recipe; this avoids the unsupported AITer fused-MoE heuristic dispatch path while keeping legacy harness-only environment indirection removed." | ||
| - "Declare the exact SGLang and AMD MoRI runtime fingerprint reported by the pinned ROCm image so srt-slurm identity verification is reproducible." | ||
| - "Rebase the AMD runtime on the active native-router branch and wait for every advertised SGLang HTTP worker to become healthy before launching Model Gateway, preventing slow ROCm startup from permanently dropping a static prefill worker after the router's one-shot registration timeout." | ||
| - "Pin the MI355X launcher to SemiAnalysisAI/srt-slurm commit 5c0d5a718194198447e8ceea5c37048c2fb8eaf5, including the native-router base and SGLang worker-readiness gate." | ||
| - "Document the MI355X Ionic DSCP/PFC and RDMA health contract required by MoRI, including the drain-before-repair and verify-before-resume procedure." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2555 | ||
|
|
||
| - config-keys: | ||
| - dsv4-fp4-b300-vllm-agentic-mtp | ||
| description: | ||
| - "Update the B300 DSV4 AgentX MTP image and SimpleCPU sweep through c576." |
There was a problem hiding this comment.
🔴 BLOCKING: New perf-changelog.yaml entries must be appended to the END of the file. The file is read chronologically (oldest at top, newest at bottom), so inserting in the middle or prepending breaks the ordering. The #2536 entry (merged to main via the branch update) now sits after this PR's #2555 entry — please move the #2555 entry to the bottom of the file.
Fix:
| - config-keys: | |
| - dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp | |
| description: | |
| - "Port the merged MI355X DeepSeek V4 AgentX MTP 1P1D search space from the legacy AMD multinode launcher to srt-slurm." | |
| - "Use srt-slurm's native SGLang Router frontend and AMD MoRI transport without Dynamo, NATS, etcd, or NIXL." | |
| - "Preserve TP8/EP1 c1,c2,c4,c8,c16,c32 and TP8/EP8/DPA c96,c128, EAGLE MTP, the golden simulated average length, HiCache settings, and logical prefill/decode server-metrics collection through the custom AgentX benchmark." | |
| - "Remove the superseded DSV4-specific amd_utils benchmark wrapper, model stanza, HiCache override plumbing, and hard-coded synthetic-acceptance injection; the srt-slurm recipes now own those settings directly." | |
| - "Allocate all 128 logical CPUs per MI355X backend node so SGLang TP8 affinity follows the complete host topology, and pin srt-slurm diagnostics that preserve failure-log tails containing non-UTF-8 ROCm compiler output." | |
| - "Move the known-green DeepSeek V4 ROCm kernel-routing contract into each self-contained srt-slurm recipe; this avoids the unsupported AITer fused-MoE heuristic dispatch path while keeping legacy harness-only environment indirection removed." | |
| - "Declare the exact SGLang and AMD MoRI runtime fingerprint reported by the pinned ROCm image so srt-slurm identity verification is reproducible." | |
| - "Rebase the AMD runtime on the active native-router branch and wait for every advertised SGLang HTTP worker to become healthy before launching Model Gateway, preventing slow ROCm startup from permanently dropping a static prefill worker after the router's one-shot registration timeout." | |
| - "Pin the MI355X launcher to SemiAnalysisAI/srt-slurm commit 5c0d5a718194198447e8ceea5c37048c2fb8eaf5, including the native-router base and SGLang worker-readiness gate." | |
| - "Document the MI355X Ionic DSCP/PFC and RDMA health contract required by MoRI, including the drain-before-repair and verify-before-resume procedure." | |
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2555 | |
| - config-keys: | |
| - dsv4-fp4-b300-vllm-agentic-mtp | |
| description: | |
| - "Update the B300 DSV4 AgentX MTP image and SimpleCPU sweep through c576." | |
| - config-keys: | |
| - dsv4-fp4-b300-vllm-agentic-mtp | |
| description: | |
| - "Update the B300 DSV4 AgentX MTP image and SimpleCPU sweep through c576." | |
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2536 | |
| - config-keys: | |
| - dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp | |
| description: | |
| - "Port the merged MI355X DeepSeek V4 AgentX MTP 1P1D search space from the legacy AMD multinode launcher to srt-slurm." | |
| - "Use srt-slurm's native SGLang Router frontend and AMD MoRI transport without Dynamo, NATS, etcd, or NIXL." | |
| - "Preserve TP8/EP1 c1,c2,c4,c8,c16,c32 and TP8/EP8/DPA c96,c128, EAGLE MTP, the golden simulated average length, HiCache settings, and logical prefill/decode server-metrics collection through the custom AgentX benchmark." | |
| - "Remove the superseded DSV4-specific amd_utils benchmark wrapper, model stanza, HiCache override plumbing, and hard-coded synthetic-acceptance injection; the srt-slurm recipes now own those settings directly." | |
| - "Allocate all 128 logical CPUs per MI355X backend node so SGLang TP8 affinity follows the complete host topology, and pin srt-slurm diagnostics that preserve failure-log tails containing non-UTF-8 ROCm compiler output." | |
| - "Move the known-green DeepSeek V4 ROCm kernel-routing contract into each self-contained srt-slurm recipe; this avoids the unsupported AITer fused-MoE heuristic dispatch path while keeping legacy harness-only environment indirection removed." | |
| - "Declare the exact SGLang and AMD MoRI runtime fingerprint reported by the pinned ROCm image so srt-slurm identity verification is reproducible." | |
| - "Rebase the AMD runtime on the active native-router branch and wait for every advertised SGLang HTTP worker to become healthy before launching Model Gateway, preventing slow ROCm startup from permanently dropping a static prefill worker after the router's one-shot registration timeout." | |
| - "Document the MI355X Ionic DSCP/PFC and RDMA health contract required by MoRI, including the drain-before-repair and verify-before-resume procedure." | |
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2555 |
| amd-mori: "0.5.15.post1.dev20260719+ga03ca46a28" | ||
|
|
||
| slurm: | ||
| time_limit: "04:00:00" |
There was a problem hiding this comment.
🔴 BLOCKING: This row runs six concurrency points (conc-list: [1,2,4,8,16,32]) inside one srt-slurm job, but the standard sweep passes DURATION=3600 per point (DEFAULT_AGENTIC_DURATION_SECONDS in utils/matrix_logic/validation.py:16; the 1200s fast path only applies with the agentx-fast label). build_replay_cmd passes that straight to --benchmark-duration, so profiling alone is ≥6h — before DSV4 model load, per-point dataset configuration (AIPERF_DATASET_CONFIGURATION_TIMEOUT=1800), per-point warmup grace (up to 1800s), and inter-point drain waits.
Why it matters: time_limit: "04:00:00" guarantees the Slurm job hits its wall and is killed mid-sweep on any non-fast run (main-branch sweeps, full-sweep-enabled PR runs). The legacy path this ports from ran under #SBATCH --time=24:00:00 (benchmarks/multi_node/amd_utils/job.slurm:8).
Fix:
| time_limit: "04:00:00" | |
| time_limit: "24:00:00" |
| benchmark: | ||
| type: custom | ||
| command: | | ||
| set -euo pipefail | ||
| mkdir -p /infmax-workspace/LOGS/agentic | ||
| export RESULT_DIR=/infmax-workspace/LOGS/agentic | ||
| export AGENTIC_OUTPUT_DIR=/infmax-workspace | ||
| exec bash /infmax-workspace/benchmarks/multi_node/agentic_srt.sh |
There was a problem hiding this comment.
🟡 WARNING: This is the first lane to run agentic_srt.sh against the native SGLang Router + SGLang backends, and its inter-point drain gate (wait_for_agentic_servers_idle, benchmarks/multi_node/agentic_srt.sh:32-98) only knows Dynamo/vLLM metric names: it polls the frontend for dynamo_frontend_active_requests and the workers for vllm:num_requests_running / vllm:num_requests_waiting.
Why it matters: with this stack neither name exists, so between concurrency points one of two things happens:
- If
GET /metricson the router port returns 200, all sums are 0 and the gate trivially passes after 3 polls — no actual drain, so in-flight requests from the previous point bleed into the next point's measurements. - If the router does not serve
/metricson the request port (sglang-router exposes Prometheus on a separate port), every poll raises, and after 1800s the script exits non-zero with "Agentic servers did not drain" — killing the run after the first point. Withconc-list: [1,2,4,8,16,32]this path executes 5 times (the c96/c128 rows are single-point and unaffected).
Fix: teach the drain check the SGLang names (sglang:num_running_reqs on the worker metrics endpoints srt-slurm injects via AIPERF_SERVER_METRICS_URLS, and skip or adapt the frontend query for FRAMEWORK=sglang-disagg), or at minimum confirm the in-progress live validation covers a multi-point transition on this row before undraft. Fix this →
| forwarded = ( | ||
| "AIPERF_EXPERIMENTAL_FAST", | ||
| "CONC", | ||
| "CONC_LIST", | ||
| "DURATION", | ||
| "EVAL_CONC", | ||
| "EVAL_LIMIT", | ||
| "EVAL_ONLY", | ||
| "FRAMEWORK", | ||
| "IS_AGENTIC", | ||
| "KV_OFFLOADING", | ||
| "MAX_MODEL_LEN", | ||
| "MODEL", | ||
| "MODEL_PREFIX", | ||
| "PRECISION", | ||
| "RESULT_FILENAME", | ||
| "RUN_EVAL", | ||
| "RUNNER_TYPE", | ||
| "SPEC_DECODING", | ||
| "TOTAL_CPU_DRAM_GB", | ||
| ) |
There was a problem hiding this comment.
🔴 runners/launch_mi355x-amds-srt.sh:100-121 defines the srt-slurm forwarding list that copies the launcher's env into the recipe's benchmark.env, but it omits PREFILL_NUM_WORKERS, PREFILL_TP, PREFILL_EP, PREFILL_DP_ATTN, DECODE_NUM_WORKERS, DECODE_TP, DECODE_EP, DECODE_DP_ATTN, DISAGG, and IMAGE. Since the new dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp recipes hardcode IS_MULTINODE: "true", process_agentic_result.py's _gpu_shape() reads these via env_int()/env_bool() inside the container and gets all-default (0/false/empty) values, so every concurrency point of this new lane will publish num_prefill_gpu=0, num_decode_gpu=0, tp=0, disagg=false, image='', and silently drop the per_gpu throughput block.
Extended reasoning...
The bug. runners/launch_mi355x-amds-srt.sh builds the srt-slurm recipe's benchmark.env from a curated forwarded tuple (the launcher's own env is not otherwise visible inside the srt-slurm benchmark container — this curated list is the only channel):
forwarded = (
"AIPERF_EXPERIMENTAL_FAST", "CONC", "CONC_LIST", "DURATION", "EVAL_CONC",
"EVAL_LIMIT", "EVAL_ONLY", "FRAMEWORK", "IS_AGENTIC", "KV_OFFLOADING",
"MAX_MODEL_LEN", "MODEL", "MODEL_PREFIX", "PRECISION", "RESULT_FILENAME",
"RUN_EVAL", "RUNNER_TYPE", "SPEC_DECODING", "TOTAL_CPU_DRAM_GB",
)It never forwards PREFILL_NUM_WORKERS, PREFILL_TP, PREFILL_EP, PREFILL_DP_ATTN, DECODE_NUM_WORKERS, DECODE_TP, DECODE_EP, DECODE_DP_ATTN, DISAGG, or IMAGE — even though all of them are present in the launcher's own environment (IMAGE is even required at line 18 via : "${IMAGE:?...}", and DISAGG/PREFILL_NUM_WORKERS/PREFILL_TP/DECODE_NUM_WORKERS/DECODE_TP are read later in the same script at lines 207-212 to compute TOTAL_GPUS).
The code path that triggers it. The three new recipe YAMLs added in this PR (disagg-1p1d-tp8-mtp-hicache.yaml, disagg-1p1d-dep8-mtp-hicache-c96.yaml, disagg-1p1d-dep8-mtp-hicache-c128.yaml) all hardcode IS_MULTINODE: "true" in their benchmark.env block, and their benchmark.command execs agentic_srt.sh, which runs the aiperf trace replay and then calls write_agentic_result_json -> python3 -m utils.agentic.aggregation.process_agentic_result inside the srt-slurm container. process_agentic_result.py::_gpu_shape() checks is_multinode = env_bool("IS_MULTINODE") (line 99); since it's true, it takes the multinode branch (lines 116-150) and reads PREFILL_NUM_WORKERS/PREFILL_TP/PREFILL_EP and DECODE_NUM_WORKERS/DECODE_TP/DECODE_EP via env_int() (defaults to 0 when unset), PREFILL_DP_ATTN/DECODE_DP_ATTN via env_bool() (defaults to False), and image = os.environ.get("IMAGE", "") and disagg = env_bool("DISAGG") (both empty/false by default) in build_agg().
Why nothing catches it. These vars do exist in the launcher's shell environment (configs/amd-master.yaml's search-space entries set PREFILL_TP, PREFILL_NUM_WORKERS, DECODE_TP, etc. as job-level env, and the workflow sets DISAGG/IMAGE), so a naive read of the launcher script would suggest the topology is known. But the container running process_agentic_result.py only sees whatever was written into the recipe's benchmark.env by this curated forwarded list — the srt-slurm container does not inherit the launcher's ambient shell environment. The workflow's success check (benchmark-multinode-tmpl.yml, globbing result files and checking num_requests_successful) only verifies that files exist, not that their topology fields are correct, so the run "passes" while quietly emitting corrupted metadata.
Impact. For every concurrency point of the new dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp config (c1/c2/c4/c8/c16/c32 TP8, plus c96/c128 DEP8), the agg_*.json will record num_prefill_gpu=0, num_decode_gpu=0, num_gpus=0, tp=prefill_tp+decode_tp=0, ep=1 (fallback), disagg=false, image=''. Because num_gpus=0, the num_gpus > 0 guard in build_agg() (right before agg["request_metrics"] = request_nested) also silently skips computing request_nested["throughput"]["per_gpu"], dropping tput_per_gpu/output_tput_per_gpu/input_tput_per_gpu — the primary per-GPU throughput metrics this benchmark exists to produce. The run does not crash (every division here is guarded), so it reports success with garbage/incomplete data for the entire new lane, requiring a full re-sweep once discovered. Notably, the PR's own job.slurm diff documents fixing this exact class of bug for the legacy AMD launcher path (missing PREFILL_TP/DECODE_TP/DISAGG forwarding causing tp=0/prefill_tp=0/disagg=false); it is reintroduced here in the new srt-slurm Python forwarding list.
Step-by-step proof. Take the c1 arm of dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp (TP8/EP1, non-DP): (1) the launcher's shell env has PREFILL_TP=8, PREFILL_NUM_WORKERS=1, DECODE_TP=8, DECODE_NUM_WORKERS=1, DISAGG=true, IMAGE=lmsysorg/sglang-rocm:.... (2) The forwarded tuple copies only MODEL, CONC, RUNNER_TYPE, etc. into the recipe's benchmark.env; none of the six topology vars or DISAGG/IMAGE are copied. (3) srt-slurm launches the benchmark container using only that benchmark.env. (4) Inside the container, agentic_srt.sh runs the trace replay then calls process_agentic_result.py. (5) _gpu_shape() calls env_int("PREFILL_NUM_WORKERS") -> 0 (unset), env_int("PREFILL_TP") -> 0, likewise for decode -> num_prefill_gpu = 0*0 = 0, num_decode_gpu = 0, num_gpus = 0, tp = 0+0 = 0. (6) build_agg() sets disagg = env_bool("DISAGG") -> False, image = os.environ.get("IMAGE", "") -> "". (7) Because num_gpus == 0, the if "total_tput_tps" in request_flat and num_gpus > 0: branch is skipped, so request_nested["throughput"] never gets a per_gpu key. (8) The resulting agg_*.json for conc1 (and every other concurrency point) has num_prefill_gpu: 0, num_decode_gpu: 0, tp: 0, disagg: false, image: "" and no tput_per_gpu/output_tput_per_gpu/input_tput_per_gpu, even though the run itself served real traffic across 16 real GPUs.
Fix. Add PREFILL_NUM_WORKERS, PREFILL_TP, PREFILL_EP, PREFILL_DP_ATTN, DECODE_NUM_WORKERS, DECODE_TP, DECODE_EP, DECODE_DP_ATTN, DISAGG, and IMAGE to the forwarded tuple in runners/launch_mi355x-amds-srt.sh so they land in the recipe's benchmark.env the same way the other process_agentic_result.py inputs (MODEL, RUNNER_TYPE, etc.) already do.
| mkdir -p "${SRT_REPO_DIR}/$(dirname "$CONFIG_PATH")" | ||
| cp "$LOCAL_RECIPE" "${SRT_REPO_DIR}/${CONFIG_PATH}" | ||
| cp "$CLUSTER_PROFILE" "${WORK_DIR}/srtslurm.yaml" | ||
| python3 - "${WORK_DIR}/srtslurm.yaml" "$GITHUB_WORKSPACE" "$SHARED_RESULTS" <<'PY' | ||
| python3 - "${WORK_DIR}/srtslurm.yaml" "${SRT_REPO_DIR}/${CONFIG_PATH}" \ | ||
| "$GITHUB_WORKSPACE" "$SHARED_RESULTS" "$SHARED_AIPERF_CACHE" "$SHARED_IMAGE" <<'PY' | ||
| import os | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| path = Path(sys.argv[1]) | ||
| workspace, results = sys.argv[2:] | ||
| import yaml | ||
|
|
||
| profile_path = Path(sys.argv[1]) | ||
| recipe_path = Path(sys.argv[2]) | ||
| workspace, results, aiperf_cache, image_path = sys.argv[3:] | ||
| needle = " /it-share/hf-hub-cache: /hf_hub_cache\n" | ||
| text = path.read_text() | ||
| text = profile_path.read_text() | ||
| if text.count(needle) != 1: | ||
| raise SystemExit("expected exactly one Hugging Face cache mount") | ||
| path.write_text( | ||
| profile_path.write_text( | ||
| text.replace( | ||
| needle, | ||
| needle + f" {workspace}: /infmax-workspace\n {results}: /results\n", | ||
| needle | ||
| + f" {aiperf_cache}: /aiperf_mmap_cache\n" | ||
| + f" {workspace}: /infmax-workspace\n" | ||
| + f" {results}: /results\n", | ||
| ) | ||
| ) | ||
|
|
||
| recipe = yaml.safe_load(recipe_path.read_text()) | ||
| container_alias = recipe["model"]["container"] |
There was a problem hiding this comment.
🟡 This PR deletes utils/test_mi300x_srt_slurm_contract.py outright instead of just updating the one stale SHA-pin assertion it contained, removing all CI coverage for the still-active runners/launch_mi300x-amds-srt.sh launcher and its MI300X recipes/config entries. It also leaves the substantial new logic in runners/launch_mi355x-amds-srt.sh added by this PR (IMAGE_KEY/SHARED_IMAGE derivation, the recipe-patching Python block, forwarded-env injection, EVAL_ONLY/RUN_EVAL mutation) with no contract test at all.
Extended reasoning...
This PR removes utils/test_mi300x_srt_slurm_contract.py in its entirety (225 lines, commit ab17f87 "drop redundant launcher contract suite"). That file was the only test covering runners/launch_mi300x-amds-srt.sh — it asserted the SRT_SLURM_COMMIT pin, --no-preflight submission mode, VLLM_IMAGE/VLLM_ROUTER_IMAGE values, the enroot/flock atomic-staging sequence, and the exact agg/disagg search-space entries in configs/amd-master.yaml. None of that is duplicated anywhere else in the tree (a repo-wide grep for the launcher, the vllm/qwen3-0.6b/mi300x recipes, and SRTCTL_RUNTIME_SOURCE_DIR turns up nothing else). All of that MI300X code is still present, still active, and entirely untouched by this PR's MI355X-porting scope.
One verifier refutation makes a fair point worth addressing directly: the deleted test was stale — it hard-asserted the old MI300X pin 315e4b06a7e0806194a646ea21832e750e896a46, and this PR's own AMD_BRINGUP.md diff bumps that pin to dd0109d4..., so the assertion would have failed as-is and the author had to do something about it. That's true, but it only justifies fixing one line. The other assertions in the file (--no-preflight, the image tags, the enroot/flock staging sequence, the recipe/config contract) were still accurate at deletion time and covered code this PR doesn't touch. Choosing to delete the whole 225-line suite rather than update the single stale line trades a real, working regression test for no test at all — on a launcher that continues to run in CI.
Compounding this, the substantial new logic this PR adds to runners/launch_mi355x-amds-srt.sh:87-116 (the IMAGE→IMAGE_KEY/SHARED_IMAGE derivation, the inline-Python block that patches the recipe's container alias, forwarded-env, and EVAL_ONLY/RUN_EVAL mutation) ships with zero test coverage of its own — no test_mi355x_srt_slurm_contract.py or equivalent exists anywhere in the tree after this diff.
Concretely, a future regression in launch_mi300x-amds-srt.sh (e.g. an accidental change to --no-preflight, the pinned images, or the enroot staging locking) would previously have been caught by test_mi300x_srt_slurm_contract.py::test_official_matrix_routes_disagg_through_the_pinned_srt_launcher and its siblings; after this PR, nothing in CI would catch it.
This is a coverage-regression concern, not a runtime correctness bug — the code itself still works, so nothing breaks by merging as-is. The straightforward fix is to restore the deleted test with just the one stale pin assertion updated (315e4b06... → dd0109d4...), and separately add a new test_mi355x_srt_slurm_contract.py covering the recipe-patching logic this PR introduces.
| SGLANG_DISAGGREGATION_WAITING_TIMEOUT: "1800" | ||
| decode_environment: | ||
| <<: *common_environment | ||
| SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK: "1024" |
There was a problem hiding this comment.
🟡 In the legacy amd_utils launcher, env.sh unconditionally pins SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE=128 for DeepSeek-V4-Pro, and server_sglang.sh's ${MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE:-...} expansion always preferred that pinned 128 over the DEP8/conc/MTP-derived fallback — so every validated legacy DSV4 AgentX arm actually ran decode at dispatch-tokens-per-rank=128. These three new srt-slurm recipes instead hardcode the derived formula value that was actually shadowed: disagg-1p1d-tp8-mtp-hicache.yaml:97 sets 1024, disagg-1p1d-dep8-mtp-hicache-c96.yaml:97 sets 48, and disagg-1p1d-dep8-mtp-hicache-c128.yaml:97 sets 64 — none matches the validated 128. srt-slurm never sources env.sh, so nothing restores it; this silently changes decode MoE dispatch-buffer sizing on every arm despite the PR description's claim to preserve the validated configuration.
Extended reasoning...
The bug: the legacy launcher's actually-validated decode MoRI dispatch-tokens-per-rank value is 128 for DeepSeek-V4-Pro AgentX, but none of the three new srt-slurm recipes reproduce it — they instead reproduce the derived value that the legacy launcher silently overrode and never actually used.
Code path: benchmarks/multi_node/amd_utils/env.sh (line ~341, in the if [[ \"$MODEL_NAME\" == \"DeepSeek-V4-Pro\" ]] block) unconditionally exports MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE=128, with a comment noting server_sglang.sh prefers these over the MORI_MAX_DISPATCH_* coupling when set. Sure enough, server_sglang.sh's decode launch builds SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK=${MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE:-${MORI_MAX_DISPATCH_TOKENS_DECODE}}. Because the PER_RANK var is always non-empty for this model, the :- fallback to the DEP8/conc/MTP-derived MORI_MAX_DISPATCH_TOKENS_DECODE (the value the DP+EP override block in server_sglang.sh actually computes from concurrency/topology) never fires. Nothing in server_sglang.sh ever reassigns the PER_RANK var. So on the legacy path, every validated DSV4 AgentX arm — TP8, DEP8 c96, DEP8 c128 — launched decode with per-rank dispatch tokens pinned at 128, regardless of what the derived formula would have produced.
Why the PR doesn't prevent it: srt-slurm recipes don't source env.sh at all — they set the decode container's environment directly via decode_environment.SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK in the YAML. The author baked in the derived formula values (which they likely computed from the same conc/DEP8/MTP formula server_sglang.sh uses) not realizing that on the legacy path this derived value was always shadowed by the fixed 128 override. As a result:
disagg-1p1d-tp8-mtp-hicache.yaml:97→\"1024\"disagg-1p1d-dep8-mtp-hicache-c96.yaml:97→\"48\"disagg-1p1d-dep8-mtp-hicache-c128.yaml:97→\"64\"
None of these is 128. Since srt-slurm never sources env.sh, there is no later override to restore the validated value — the recipe's YAML is authoritative.
Corroborating evidence: the same recipes correctly preserve the prefill pin — line 92 in all three sets SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK: \"16384\", exactly matching env.sh's MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_PREFILL=16384. This prefill/decode asymmetry is strong evidence the decode value was re-derived from the formula rather than copied from the validated pinned value, contradicting the PR description's explicit claim to 'preserve the existing validated configuration.'
Step-by-step proof (TP8 arm):
- Legacy launch for DSV4-Pro AgentX TP8 sets
MODEL_NAME=DeepSeek-V4-Pro, which triggersenv.sh's block, exportingMORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE=128. server_sglang.shbuildsDECODE_CMDwithSGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK=${MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE:-...}→ since the var is set to 128 (non-empty), bash's:-never evaluates the fallback → the sglang decode server actually launches withSGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK=128.- In the new srt-slurm recipe
disagg-1p1d-tp8-mtp-hicache.yaml,decode_environment.SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANKis set directly to\"1024\"in the YAML. - srt-slurm passes this environment straight into the decode container; there is no
env.shsourcing step to override it. - Result: the decode server launches with dispatch-tokens-per-rank=1024 instead of the validated 128 — an 8x change to MoE dispatch buffer sizing versus what was actually benchmarked in the referenced PR [AMD][AgentX] DSv4 MI355X agentic disagg: add EAGLE/MTP speculative-decoding arm / DSv4 MI355X 智能体分离式推理：新增 EAGLE/MTP 投机解码分支 #2309.
Impact: this is a benchmark-fidelity divergence, not a crash — all three hardcoded values (1024, 48, 64) are usable per-rank caps for their respective topologies (48/64 match the tight DEP8 per-step token bound; 1024 over-provisions TP8), so the runs will complete and produce results, just under different MoE dispatch sizing than what was actually validated.
Fix: hardcode 128 (matching env.sh's pinned MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK_DECODE) for decode_environment.SGLANG_MORI_NUM_MAX_DISPATCH_TOKENS_PER_RANK in all three recipes, the same way the prefill side already correctly hardcodes 16384.
72ee996 to
2182835
Compare
69c760c to
8084da4
Compare
8084da4 to
1494f62
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33024402380 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33025514490 |
7b6bc7c to
aed36b6
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33419467311 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33435268698 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33460836315 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33464520100 |
|
/stage-results 33464520100 |
|
@cquil11 staged run 33464520100: https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-09-01~r33464520100 This run remains available across future |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d81e0db. Configure here.
| if eval_only: | ||
| recipe["benchmark"]["command"] = _EVAL_COMMAND | ||
| else: | ||
| recipe["benchmark"]["command"] = recipe["benchmark"]["command"].rstrip() + "\n" + _EVAL_COMMAND |
There was a problem hiding this comment.
Eval command skipped after exec
Medium Severity
When RUN_EVAL is set without EVAL_ONLY, _configure_evaluation appends the eval script after the recipe command. Those agentic recipes end with exec bash .../agentic_srt.sh, which replaces the shell, so the appended run_eval path never runs and combined perf-plus-eval jobs silently skip evaluation.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d81e0db. Configure here.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34929111531 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34979900416 |
1 similar comment
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34979900416 |
|
View unofficial run (performance): https://inferencex.semianalysis.com/inference?unofficialRun=35231968727 View unofficial run (accuracy): https://inferencex.semianalysis.com/evaluation?unofficialRun=35231968727 |
|
Validation is gated on upstream checkpoint correction #3170. Earlier port Slurm44317 logs independently report uninitialized required draft weights ( |
cquil11
changed the title
15e61d4 to
723dfba
Compare
0d7ca6f to
6d439d6
Compare
1 participant
Scope
Port the now-merged upstream MI355X DeepSeek-V4-Pro-0813 AgentX DSPARK configuration using native srt-slurm v2. The old #3188 baseline is already present in current InferenceX main.
model_pathsalias for the existing/it-share/data/DeepSeek-V4-Pro-0813checkpoint, as the production launcher did. The shared Hub cache is mounted at/hf_hub_cache/hub; no cache seeding or duplicate model download.lmsysorg/sglang-rocm:v0.5.19-rocm720-mi35x-20260913and separate client image...-20260907.launch_mi355x-amds.shremains the single cluster entrypoint. srt-slurm owns allocation, worker/router/service lifecycle, log streaming, and completion. The adapter ininfx.workflows.srt_slurmmaps CI inputs and collects results.benchmark_serving.pyis unchanged.Orchestration differences
Native SGLang routing and MoRI replace the legacy launch/barrier/process coordination. Model, precision, capacity, MTP, and throughput settings are preserved; startup commands are not byte-identical. srt-slurm assigns its own endpoint/bootstrap ports and handles native-worker startup.
The existing upstream native SGLang backend adds
--skip-server-warmupfor disaggregation, avoiding a local synthetic warmup against a placeholder peer. This startup behavior is not introduced by the AMD stack. Canonical AgentX benchmark warmup is retained, and real P/D serving and evaluations must pass before validation is claimed.The UMBP socket lives in the allocation-specific shared
/logsmount so the service and worker containers can both access it. The host-memory capacity guard is preserved. One managed readiness deadline of 5400 seconds replaces three sequential 1800-second startup waits; workers still wait for the final GPU-registration marker, not merely a listening socket. A failed service is managed by srt-slurm.Caller-owned runtime settings and the separate benchmark image are forwarded into the custom benchmark. Cluster RDMA checks and free-hugepage preparation retain the existing host-setup integration.
Synthetic acceptance is selected by the shared golden-curve connector instead of a hard-coded recipe value.
Stack
Based directly on shared AMD integration #2542, which is rebased onto InferenceX
mainat96bccfe12b637dad23d32f0bf7ceef0ea86de15a. This child contains only its workload configs, schema-2 recipes, and append-only performance changelog entry. Shared launcher/profile/adapter changes live in #2542.The base selects SemiAnalysisAI/srt-slurm
03863ab23804f8a31811d9aaccc54abc8961a41c: #1 → #11 → #7 → #14, synchronized with NVIDIA srt-slurm85086d3c1fe7b319a1d0d7db8781047dd1940772. NVIDIA runners retain their existing runtime selection.Refresh status
Offline matrix validation: 7 topology/workload rows covering 7 benchmark concurrency points; all 14 prepared throughput/eval configurations pass the pinned runtime's schema/topology checks. The seven AgentX points retain concurrency-scaled admission/graphs and native UMBP service coordination.
No fresh E2E tests or sweeps were started or rerun. Sweep-trigger labels are removed. Historical runtime evidence is listed in the base PR and does not validate these new heads. Full GPU validation remains pending authorization; this refresh makes no new performance or merge-readiness claim.
AI model disclosure
The exact AI model/version for this refresh is not exposed by the runtime and could not be verified. No subagents contributed to this refresh; earlier model identities are not reliably recorded.