Skip to content

Test AIPerf empty-content TTFT on MiniMax TRT - #2682

Open
cquil11 wants to merge 2 commits into
mainfrom
cquil11/test-aiperf-empty-ttft
Open

Test AIPerf empty-content TTFT on MiniMax TRT#2682
cquil11 wants to merge 2 commits into
mainfrom
cquil11/test-aiperf-empty-ttft

Conversation

@cquil11

@cquil11 cquil11 commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Pin utils/aiperf to the exact head of SemiAnalysisAI/agentx-harness PR [NVIDIA] Update B200 SGL FP8 and add FP4 TP4 #42 (e8ece5d29bd0285af1ced36fbc4c670328b2d58c).
  • Enable --allow-empty-content only for the MiniMax-M3 NVFP4 B300 TensorRT-LLM AgentX recipe.
  • Use the existing TP8, concurrency-1 configuration to test whether the parser-suppressed first generation event establishes TTFT.

Why

The existing MiniMax-M3 TRT-LLM configuration uses stream_interval: 100. Its first generated reasoning delimiter is suppressed by the reasoning parser, so current AIPerf waits for later non-empty content before recording TTFT. This PR tests AIPerf PR #42 against that concrete serving path.

Validation

  • bash -n benchmarks/single_node/agentic/minimaxm3_fp4_b300_trt_mtp.sh
  • Focused AIPerf tests: 26 passed
  • Matrix generation selects exactly MiniMax-M3 TRT, B300, TP8, concurrency 1.
  • A one-off AgentX fast run will be linked after dispatch.

@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 19, 2026 21:39
@cquil11
cquil11 requested a review from a team August 19, 2026 21:39
@cquil11 cquil11 changed the title Test AIPerf empty-content TTFT on MiniMax TRT / 测试 MiniMax TRT 的 AIPerf 空内容 TTFT Test AIPerf empty-content TTFT on MiniMax TRT Aug 19, 2026
@cquil11
cquil11 force-pushed the cquil11/test-aiperf-empty-ttft branch from a5960e3 to ecb89bc Compare August 19, 2026 21:40
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 3m 49s —— View job


PR Review

  • Gather context (diff, changed files)
  • Verify perf-changelog entry placement and rules
  • Verify aiperf submodule pin (reachability, diff vs current pin)
  • Review benchmark script change
  • Post review feedback

Summary

Reviewed all 3 changed files. The benchmark script change is sound: REPLAY_CMD is a plain string built via += appends in benchmarks/benchmark_lib.sh:1998, and appending --allow-empty-content after build_replay_cmd (benchmarks/single_node/agentic/minimaxm3_fp4_b300_trt_mtp.sh:176) matches the established pattern other agentic recipes use for flags like --apply-chat-template and --server-metrics. The EVAL_ONLY path is correctly unaffected. The perf-changelog.yaml entry is correctly appended at the end of the file.

One 🟡 warning posted inline on utils/aiperf: the submodule is shared by every agentic recipe, and it's being pinned to an unmerged PR head. Please confirm (1) e8ece5d29 is a fast-forward descendant of the current pin 754356e9a with all behavior changes gated behind the opt-in --allow-empty-content flag — otherwise this silently changes aiperf behavior for all other agentic sweeps while the changelog scopes it to MiniMax only; and (2) the commit is reachable from the configured submodule remote SemiAnalysisAI/aiperf.git (the PR description references SemiAnalysisAI/agentx-harness PR #42, a different repo name). Plan to re-pin to the merged mainline aiperf commit once PR #42 lands. I could not verify the ancestry or the flag's existence at the pinned commit directly (no network access to the aiperf repo from this environment); the queued sweep run will exercise both.

No blocking issues found.

@cquil11

cquil11 commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

+viz @ajcasagrande

@github-actions

Copy link
Copy Markdown
Contributor

@cquil11
cquil11 force-pushed the cquil11/test-aiperf-empty-ttft branch from ecb89bc to f17aecd Compare August 19, 2026 21:43
Comment thread utils/aiperf
@@ -1 +1 @@
Subproject commit 754356e9a39acc6cc6afb242d123bb57c3fb6f75
Subproject commit e8ece5d29bd0285af1ced36fbc4c670328b2d58c

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.

🟡 WARNING: Shared submodule pinned to an unmerged PR head

Why it matters: utils/aiperf is consumed by every agentic recipe (all of them build REPLAY_CMD from the same submodule), not just minimaxm3-fp4-b300-trtllm-agentic-mtp. Two things can't be verified from this diff alone:

  1. Ancestry: if e8ece5d29 is not a fast-forward descendant of the current pin 754356e9a (e.g. aiperf PR [NVIDIA] Update B200 SGL FP8 and add FP4 TP4 #42 is based on an older or diverged base), this pin silently changes or rolls back aiperf behavior for all other agentic sweeps, while the perf-changelog entry scopes the change to the MiniMax config only. It's only safe if every behavior change in the 754356e9a..e8ece5d29 range is gated behind the opt-in --allow-empty-content flag.
  2. Reachability: the submodule remote is SemiAnalysisAI/aiperf.git, but the PR description says the commit is the head of SemiAnalysisAI/agentx-harness PR [NVIDIA] Update B200 SGL FP8 and add FP4 TP4 #42 — a different repo name. If the commit only exists outside the aiperf repo network, git submodule update will fail on fresh checkouts. Additionally, once PR [NVIDIA] Update B200 SGL FP8 and add FP4 TP4 #42 is rebased or squash-merged, this dangling commit stays reachable only through the PR ref, which is fragile for long-term reproducibility on main.

Fix: Confirm in the aiperf repo that git merge-base --is-ancestor 754356e9a e8ece5d29 holds and that the range is fully gated behind --allow-empty-content; then re-pin to the merged mainline aiperf commit once PR #42 lands, before (or promptly after) this PR merges to main.

@cquil11
cquil11 force-pushed the cquil11/test-aiperf-empty-ttft branch from f17aecd to 1f037ea Compare August 19, 2026 21:45
Pin the AIPerf submodule to PR #42 and enable --allow-empty-content only for the MiniMax-M3 B300 TensorRT-LLM AgentX recipe.

Signed-off-by: Cam Quilici <cjquilici@gmail.com>
@cquil11
cquil11 force-pushed the cquil11/test-aiperf-empty-ttft branch from 1f037ea to e874f66 Compare August 19, 2026 21:45
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

2 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant