Skip to content

fix: revert required srt frontend variables #2706

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

Merged
cquil11 merged 1 commit into from
Aug 21, 2026
Merged

fix: revert required srt frontend variables #2706

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 2 additions & 7 deletions benchmarks/multi_node/agentic_srt.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,13 @@
# srt-slurm owns server startup; this script runs as benchmark.type=custom
# against the already-ready frontend on the head node.

INFMAX_CONTAINER_WORKSPACE="${INFMAX_CONTAINER_WORKSPACE:-/infmax-workspace}"
source "$INFMAX_CONTAINER_WORKSPACE/benchmarks/benchmark_lib.sh"

check_env_vars \
MODEL MODEL_PREFIX FRAMEWORK PRECISION CONC RESULT_FILENAME DURATION \
SRT_FRONTEND_HOST SRT_FRONTEND_PORT

export AIPERF_SERVER_URL="http://${SRT_FRONTEND_HOST}:${SRT_FRONTEND_PORT}"
echo "Using srt-slurm frontend endpoint: $AIPERF_SERVER_URL"
check_env_vars MODEL MODEL_PREFIX FRAMEWORK PRECISION CONC RESULT_FILENAME DURATION

BASE_RESULT_DIR="${RESULT_DIR:-/logs/agentic}"
BASE_RESULT_FILENAME="$RESULT_FILENAME"

Check failure on line 15 in benchmarks/multi_node/agentic_srt.sh

View check run for this annotation

Claude / Claude Code Review

Revert breaks GB300 dsv4 AgentX recipes with split frontend/client placement 

This revert hardcodes the AgentX frontend at `http://localhost:$PORT` (both the drain probe here and the `--url` fallback in `benchmark_lib.sh:1999`), but the 6 GB300 DeepSeek-V4 dynamo-trt recipes added in the same #2690 commit (`agentx-gb300-20260811/*.yaml`) set `orchestrator_placement: first_decode` / `client_placement: last_decode` with `decode_nodes >= 4`, so the frontend and benchmark client always run on different nodes and never set `SRT_FRONTEND_HOST/PORT` themselves. Reverting uncondi
Comment on lines 9 to 15

@claude claude Bot Aug 21, 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.

🔴 This revert hardcodes the AgentX frontend at http://localhost:$PORT (both the drain probe here and the --url fallback in benchmark_lib.sh:1999), but the 6 GB300 DeepSeek-V4 dynamo-trt recipes added in the same #2690 commit (agentx-gb300-20260811/*.yaml) set orchestrator_placement: first_decode / client_placement: last_decode with decode_nodes >= 4, so the frontend and benchmark client always run on different nodes and never set SRT_FRONTEND_HOST/PORT themselves. Reverting unconditionally re-breaks exactly the points #2690 added (traffic and drain probe both hit a non-listening localhost); a better fix is to make AIPERF_SERVER_URL conditional — set it only when SRT_FRONTEND_HOST/SRT_FRONTEND_PORT are present, falling back to localhost otherwise — so both recipe families work.

Extended reasoning...

The bug: This PR reverts agentic_srt.sh to drop the SRT_FRONTEND_HOST/SRT_FRONTEND_PORT requirement and the AIPERF_SERVER_URL export, restoring the old assumption that the AgentX frontend and the benchmark client are colocated on the same node (hardcoding http://localhost:$PORT/metrics for the drain probe, and relying on benchmark_lib.sh:1999's --url ${AIPERF_SERVER_URL:-http://localhost:$PORT} fallback for the actual replay traffic). That assumption is true for the older colocated recipes this PR is trying to fix, but it is false for the 6 GB300 DeepSeek-V4 dynamo-trt AgentX recipes under benchmarks/multi_node/srt-slurm-recipes/trtllm/deepseek-v4/agentx-gb300-20260811/, which were added in the very same #2690 commit that introduced the SRT_FRONTEND_* machinery this PR now reverts.\n\nThe code path: Verified directly against the recipe files — all six recipes set frontend.orchestrator_placement: first_decode and benchmark.client_placement: last_decode, and every one has decode_nodes >= 4 (values are 8, 8, 6, 8, 4, 4 across the six files). Since first_decode and last_decode index into a decode-node list of size >= 4, they always resolve to two different physical nodes. None of these recipes set SRT_FRONTEND_HOST/SRT_FRONTEND_PORT in their benchmark.env (only PORT: 8000 etc.) — that injection was expected to come from the external srt-slurm harness based on the placement fields, which is exactly what #2690's check_env_vars ... SRT_FRONTEND_HOST SRT_FRONTEND_PORT + export AIPERF_SERVER_URL was there to consume.\n\nWhy nothing else prevents this: build_replay_cmd in benchmark_lib.sh:1999 still reads --url ${AIPERF_SERVER_URL:-http://localhost:$PORT}; this PR only touches agentic_srt.sh, so once AIPERF_SERVER_URL is no longer exported, that fallback silently kicks in. There's also a smoking gun in runners/launch_gb300-nv.sh:252-260, which for the eval-only path explicitly rewrites orchestrator_placement: first_decode -> head with the comment 'Keep AgentX frontends on first_decode for throughput, but co-locate the eval-only frontend with lm-eval so loopback resolves.' That comment only makes sense if loopback does not resolve to the frontend in the normal (non-eval-only) AgentX path — confirming the frontend and client are genuinely on separate nodes for these recipes.\n\nStep-by-step proof (using dynamo-disagg-gb300-1p1d-dep8-dep32-c388-b4-mtp.yaml, decode_nodes=8):\n1. The srt-slurm harness allocates 8 decode nodes; first_decode = decode node #1 (runs the dynamo frontend/orchestrator on port 8000), last_decode = decode node #8 (runs agentic_srt.sh as the benchmark client).\n2. Before this PR: SRT_FRONTEND_HOST/SRT_FRONTEND_PORT are injected pointing at decode node #1; agentic_srt.sh exports AIPERF_SERVER_URL=http://<decode-node-1-ip>:8000, and both the replay traffic and the drain probe correctly target that remote node.\n3. After this PR: SRT_FRONTEND_HOST/SRT_FRONTEND_PORT are no longer required or consumed, so AIPERF_SERVER_URL is never set.\n4. build_replay_cmd falls back to --url http://localhost:8000 — but the script is running on decode node #8, where no dynamo frontend is listening on port 8000.\n5. The drain probe likewise hits http://localhost:8000/metrics on decode node #8, which also isn't serving that endpoint.\n6. Result: every request in the AgentX replay fails to connect, and/or the drain probe never gets valid metrics — the recipe fails, for all 6 of #2690's new points.\n\nThe fix: Don't revert unconditionally. Make AIPERF_SERVER_URL conditional — export it as http://${SRT_FRONTEND_HOST}:${SRT_FRONTEND_PORT} only when both vars are present (and drop them from check_env_vars's hard-required list), otherwise fall back to http://localhost:$PORT for the colocated recipes. Same for the drain probe URL. That satisfies both the older colocated recipes (the actual regression target of this PR, per the linked discussion) and the 6 split-placement GB300 dsv4 recipes from #2690, instead of trading one compatibility break for another.

read -r -a CONCURRENCIES <<< "${CONC_LIST:-$CONC}"

if [ "${#CONCURRENCIES[@]}" -eq 0 ]; then
Expand All @@ -37,7 +32,7 @@
wait_for_agentic_servers_idle() {
local timeout_seconds="${AIPERF_DRAIN_TIMEOUT_SECONDS:-1800}"
local poll_seconds="${AIPERF_DRAIN_POLL_SECONDS:-10}"
local frontend_metrics_url="${AIPERF_SERVER_URL%/}/metrics"
local frontend_metrics_url="http://localhost:${PORT}/metrics"

"$AIPERF_PYTHON" - \
"$timeout_seconds" \
Expand Down