Skip to content

fix: revert required srt frontend variables - #2706

Merged
cquil11 merged 1 commit into
mainfrom
fix/revert-agentic-srt-frontend-env
Aug 21, 2026
Merged

fix: revert required srt frontend variables#2706
cquil11 merged 1 commit into
mainfrom
fix/revert-agentic-srt-frontend-env

Conversation

@cquil11

@cquil11 cquil11 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

This addresses the compatibility regression identified in #2690 (comment).

Validation

@github-actions

Copy link
Copy Markdown
Contributor

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 As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

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 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

@cquil11
cquil11 marked this pull request as ready for review August 21, 2026 15:53
@cquil11
cquil11 requested a review from a team August 21, 2026 15:53
@cquil11
cquil11 merged commit c0c9156 into main Aug 21, 2026
8 checks passed
@cquil11
cquil11 deleted the fix/revert-agentic-srt-frontend-env branch August 21, 2026 15:53
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 2m 23s —— View job


Review of PR #2706

Verification

The revert is exact: git diff 6816ed05f~1 c0c915672 -- benchmarks/multi_node/agentic_srt.sh is empty, so the file matches its state immediately before #2690, as claimed. Existing multi-node AgentX recipes (GB200/GB300 vLLM, sglang) no longer fail check_env_vars on the unset SRT_FRONTEND_HOST/SRT_FRONTEND_PORT, and their aiperf client and drain probe are back on localhost:$PORT, which works for them because they co-locate the client with the frontend. Note that #2690's companion change in benchmarks/benchmark_lib.sh:1999 (--url ${AIPERF_SERVER_URL:-http://localhost:$PORT}) was intentionally left in place; since nothing exports AIPERF_SERVER_URL anymore, it falls back to the old behavior everywhere, so it's harmless.

One follow-up to be aware of

🟡 WARNING: This revert re-breaks the two multi-decode GB300 Dynamo-TRT AgentX recipes added in #2690.

Why it matters: All six recipes under benchmarks/multi_node/srt-slurm-recipes/trtllm/deepseek-v4/agentx-gb300-20260811/ set client_placement: last_decode with orchestrator_placement: first_decode. For the Np1d topologies (1p1d, 2p1d, 3p1d, 5p1d) there is a single decode group, so the client is co-located with the frontend and http://localhost:$PORT still resolves. But for 1p4d-dep4-tep8-c4-b1-mtp and 1p6d-dep4-tep4-c24-b4-mtp, last_decodefirst_decode: the aiperf client and the drain metrics probe will now target localhost on a node where no frontend is listening, so those two configs will fail to connect when they next run. The SRT_FRONTEND_HOST/SRT_FRONTEND_PORT export that this PR removed was what made those placements work.

Fix: In a follow-up, restore frontend addressing in a backward-compatible way instead of making it required, e.g. in agentic_srt.sh:

if [ -n "${SRT_FRONTEND_HOST:-}" ] && [ -n "${SRT_FRONTEND_PORT:-}" ]; then
    export AIPERF_SERVER_URL="http://${SRT_FRONTEND_HOST}:${SRT_FRONTEND_PORT}"
fi

and derive the drain probe URL from ${AIPERF_SERVER_URL:-http://localhost:$PORT} — or change those two recipes to client_placement: first_decode. Fix this →

Summary

The revert itself is correct, minimal, and restores compatibility for all pre-existing AgentX recipes — no blocking issues in the change as written. The one caveat is that the #2690 1p4d and 1p6d GB300 Dynamo-TRT recipes depended on the removed export and will need the backward-compatible follow-up above (or a placement change) before their next sweep.

@claude claude Bot left a comment

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 unconditionally restores the localhost-only frontend URL, but doing so re-breaks the 6 GB300 dsv4 dynamo-trt AgentX recipes added in #2690 that use split first_decode/last_decode placement (see inline comment for details). A conditional approach (use SRT_FRONTEND_HOST/PORT when present, fall back to localhost otherwise) would satisfy both recipe families instead of trading one regression for another.

Extended reasoning...

Overview

Single-file, small diff to benchmarks/multi_node/agentic_srt.sh reverting the SRT_FRONTEND_HOST/SRT_FRONTEND_PORT requirement and AIPERF_SERVER_URL export introduced in #2690, restoring the old localhost-only frontend assumption for the drain probe and (via benchmark_lib.sh's fallback) the replay traffic URL.

Security risks

None — this is benchmark tooling, no auth/crypto/data-exposure surface.

Level of scrutiny

This is a revert intended to fix a compatibility regression for colocated recipes, but I verified against the actual recipe YAMLs that it introduces a new regression for the 6 GB300 dsv4 split-placement recipes from the same #2690 commit (confirmed orchestrator_placement: first_decode / client_placement: last_decode with decode_nodes >= 4 in all six files, plus a corroborating comment in runners/launch_gb300-nv.sh that only makes sense if loopback doesn't resolve to the frontend in the normal AgentX path). This warrants human review before merging — the fix as written trades one broken recipe family for another.

Other factors

The inline bug comment lays out the full reproduction and a concrete suggested fix (conditional AIPERF_SERVER_URL), so a human reviewer has what they need to resolve this without re-deriving it.

Comment on lines 9 to 15
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"

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.

Klaud-Cold pushed a commit that referenced this pull request Aug 21, 2026
…equirement)

Removes the hard requirement on SRT_FRONTEND_HOST/SRT_FRONTEND_PORT and the
remote-frontend metrics URL, matching the #2706 resolution. Clients fall back
to localhost:$PORT via benchmark_lib.sh.

Co-authored-by: Cameron Quilici <60715037+cquil11@users.noreply.github.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cquil11 added a commit that referenced this pull request Aug 21, 2026
…Dynamo-TRT AgentX 配置 (#2657)

* feat(config): add GLM-5.2 GB300 Dynamo-TRT AgentX

添加 GLM-5.2 GB300 Dynamo-TRT AgentX 配置和多节点 srt-slurm 配方。

* chore(changelog): link PR #2657

在变更日志中补充 PR #2657 链接。

* feat(config): update GLM-5.2 GB300 Pareto01 recipe

更新 GLM-5.2 GB300 Pareto01 配方,并移除重复的可选身份信息。

* fix(config): refresh GLM-5.2 GB300 TRT Pareto points

更新 GLM-5.2 GB300 TensorRT-LLM AgentX Pareto 配置,并关闭所有点的性能指标响应。

* fix(config): scope GB300 batch_3 partition

仅为 GLM-5.2 GB300 Dynamo-TRT AgentX 配置启用 batch_3 分区。

* fix(gb300): align sbatch partition with launcher

使 srtctl 提交的作业使用启动脚本解析后的 Slurm 分区。

* Update perf-changelog.yaml

* revert: restore agentic_srt.sh to main (drop SRT_FRONTEND_HOST/PORT requirement)

Removes the hard requirement on SRT_FRONTEND_HOST/SRT_FRONTEND_PORT and the
remote-frontend metrics URL, matching the #2706 resolution. Clients fall back
to localhost:$PORT via benchmark_lib.sh.

Co-authored-by: Cameron Quilici <60715037+cquil11@users.noreply.github.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Ankur-singh <ankusingh@nvidia.com>
Co-authored-by: Cameron Quilici <cjquilici@gmail.com>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant