refactor(amd): share one fixed-sequence client across multi-node recipes - #3413
Conversation
|
Thanks for the contribution!
中文感谢你的贡献！
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| esac | ||
| done | ||
| case "${CLIENT_BACKEND:=openai}" in | ||
| openai) endpoint=/v1/completions ;; | ||
| openai-chat) endpoint=/v1/chat/completions ;; | ||
| *) echo "ERROR: unsupported CLIENT_BACKEND: $CLIENT_BACKEND" >&2; exit 1 ;; | ||
| esac | ||
| case "${USE_CHAT_TEMPLATE:=false}" in | ||
| true) CLIENT_ARGS+=(--use-chat-template) ;; | ||
| false) ;; |
There was a problem hiding this comment.
🟡 (optional) Violates the repo's mandatory Bash rule (AGENTS.md:29-30): no ${VAR:-default}/${VAR:=default}, and every required input must go through check_env_vars. The new shared script uses ${CLIENT_BACKEND:=openai} (line 15), ${USE_CHAT_TEMPLATE:=false} (line 19), ${TOKENIZER:-$MODEL} (line 40), and ${RANDOM_RANGE_RATIO:-1.0} (line 44), none of which are passed to check_env_vars (only MODEL, ISL, OSL, SRT_FRONTEND_HOST, SRT_FRONTEND_PORT, CONC_LIST, PREFILL_NUM_WORKERS, PREFILL_TP, DECODE_NUM_WORKERS, DECODE_TP are). Fix: pass CLIENT_BACKEND, USE_CHAT_TEMPLATE, TOKENIZER, and RANDOM_RANGE_RATIO through check_env_vars and require every recipe's env block to set them explicitly instead of relying on silent bash defaults, matching every other required input in this script.
Why this was flagged
Any recipe or future caller that omits CLIENT_BACKEND, USE_CHAT_TEMPLATE, TOKENIZER, or RANDOM_RANGE_RATIO in its benchmark.env silently gets openai/false/$MODEL/1.0 instead of failing fast at benchmarks/multi_node/srt_fixed_sequence.sh:15,19,40,44. On the base branch each recipe hardcoded these values inline so there was no way to omit them; after this refactor a typo'd or missing env var (e.g. a future MTP recipe forgetting USE_CHAT_TEMPLATE) produces a benchmark run with the wrong chat-template/backend/tokenizer settings that looks successful but silently reports incorrect data, rather than the clear caller error check_env_vars is supposed to guarantee per AGENTS.md:30.
Verification: nit. The candidate correctly identifies a real, present violation of the repo's mandatory Bash conventions (AGENTS.md:29 "No fallback defaults for caller-supplied configuration. Avoid ${VAR:-default}, ${VAR:=default}... Pass values such as false and 0 explicitly too." and AGENTS.md:30 "Validate every required environment input with check_env_vars before use."). The new… | nit. The candidate's…
| --result-dir "${result_dir}" \ | ||
| --result-filename "results_concurrency_${concurrency}_gpus_$((ctx + gen))_ctx_${ctx}_gen_${gen}.json" | ||
| done | ||
| command: bash /infmax-workspace/benchmarks/multi_node/srt_fixed_sequence.sh --trust-remote-code |
There was a problem hiding this comment.
🟡 (optional) This PR modifies 4 srt-slurm recipes (fp4/fp8 dsr1, qwen3.5 fp8/mxfp4) without adding a perf-changelog.yaml entry, so operators lose automatic CI perf-sweep validation that AGENTS.md:124 requires. AGENTS.md states 'every recipe addition or modification requires a new perf-changelog.yaml entry', and docs/architecture.md:119 says infx.matrix.plan selects sweep keys only from lines added to perf-changelog.yaml between base and head refs. No perf-changelog.yaml change is in this diff, so these four recipe changes will not be auto-selected for the sweep validation gate that would otherwise catch a regression in the new shared script. …
Why this was flagged
…Fix: append a perf-changelog.yaml entry (respecting its append-only, byte-sensitive format) listing the config-keys for these four recipes so the refactor gets swept and validated like any other recipe modification.
The diff modifies benchmark.command in benchmarks/multi_node/srt-slurm-recipes/sglang/dsr1/mi355x/fp4-disagg-fixed-seq.yaml:160, fp8-disagg-fixed-seq.yaml, disagg-1p1d-tp4p-tp8d-fixed-seq.yaml, and disagg-1p1d-tp8-mxfp4-fixed-seq.yaml. AGENTS.md:124 ('Non-negotiable benchmark invariants') requires a new perf-changelog.yaml entry for every recipe addition or modification; none is included in this commit (git diff --stat shows no perf-changelog.yaml change). docs/architecture.md:119 states the main sweep path (infx.matrix.plan) reads only entries ADDED to perf-changelog.yaml between base and head to select which config keys get swept/validated. Without an added entry, these four recipes are not automatically re-validated by that CI gate after this refactor, unlike recipe changes that follow the documented process.
Verification: nit (mandatory-invariant compliance gap, no runtime break). The diff modifies benchmark.command/env in all four recipes (e.g. fp4-disagg-fixed-seq.yaml the inline command: | block becomes command: bash /infmax-workspace/benchmarks/multi_node/srt_fixed_sequence.sh --trust-remote-code, plus USE_CHAT_TEMPLATE '1'->'true' etc.), and git diff --stat 41e8bfc..HEAD shows only the 4 YAMLs and… | nit.…
The four MI355X multi-node recipes each inlined the same benchmark loop. Move it to benchmarks/multi_node/srt_fixed_sequence.sh, like the single-node client; recipes set only the model, tokenizer, client backend and chat template in benchmark.env.
Default the multi-node client to the repo standard: range ratio 0.8, chat template on, --trust-remote-code, 10x prompts and 2x warmup at request rate inf. Keep the client backend configurable. When srt-slurm exports SRT_MEASUREMENT_WINDOW_DIR, write each concurrency's measured boundary as a custom-benchmark window so power lanes validate without sa-bench.
Replace the inline Python in srt_fixed_sequence.sh with infx.results.power.window, which writes srt-slurm's custom-benchmark measurement window from the client's result.
Recipes hardcoded MODEL, but the served name depends on the cluster's model path and can differ from both the recipe and the workflow's HF id (Dynamo TRT-LLM served DeepSeek-R1-0528 while the recipe said dsr1, so every request got 404). Read it from /v1/models instead.
0f03491 to
1898327
Compare
1 participant
Stacked on #2542.
Adds
benchmarks/multi_node/srt_fixed_sequence.sh, the standard InferenceX fixed-sequence client for multi-node srt-slurm recipes (the multi-node counterpart ofbenchmarks/single_node/srt_fixed_sequence.sh), and moves the four MI355X recipes onto it. It is meant to replace srt-slurm'ssa-bench; the NVIDIA recipes move in the next PR in this stack.Client behavior (defaults are the repo standard):
CONC_LIST; results go to/logs/sa-bench_isl_<ISL>_osl_<OSL>/results_concurrency_<c>_gpus_<g>_ctx_<p>_gen_<d>.json, the layoutcopy_fixed_sequence_resultsalready collects.RANDOM_RANGE_RATIO(0.8), chat template on (USE_CHAT_TEMPLATE=falseopts out),--trust-remote-code, 10x prompts and 2x warmup per concurrency, request rateinf.CLIENT_BACKENDselectsopenai(/v1/completions, default) oropenai-chat(/v1/chat/completions)./v1/models), so it always matches what the workers registered; the workflow'sMODELis the HF id and can differ. Recipes setISL,OSL, andTOKENIZERwhen the served name is not a loadable tokenizer.SRT_MEASUREMENT_WINDOW_DIR, the script callsinfx.results.power.window, which writes that interval (from the client'sbenchmark_start_time_unix/benchmark_end_time_unix) as abenchmark_type: customwindow. Power lanes already setbenchmark.concurrenciesfrom the matrix, which is what srt-slurm uses for the expected windows.MI355X recipes now use the standard client settings: range ratio 0.8 instead of 1.0, the chat template on every variant (previously MTP only), and completions for Qwen3.5 as the legacy AMD client used.
Validation: all 18 DeepSeek-R1 variants resolve through srtctl to the script; a stubbed run writes the expected results and windows and srt-slurm's window parser accepts them. No GPU run yet.