Skip to content

Add DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM recipes / 添加 DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM 配方 - #2596

Open
RohitNagraj wants to merge 3 commits into
mainfrom
dsv4-fp4-b300-dynamo-trt-recipes
Open

Add DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM recipes / 添加 DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM 配方#2596
RohitNagraj wants to merge 3 commits into
mainfrom
dsv4-fp4-b300-dynamo-trt-recipes

Conversation

@RohitNagraj

Copy link
Copy Markdown
Collaborator

Description

中文说明

Related Issue

Reference / 参考:#2552

Type of Change

  • Bug fix
  • New feature
  • Configuration change
  • Documentation update
  • Other (please describe)

Checklist

  • I have tested my changes locally
  • I have updated documentation if necessary
  • For every change that can affect benchmark performance and every recipe addition or modification, I have appended a new entry to the physical end of perf-changelog.yaml and have not edited historical entries
  • Before merging via reuse, an authorized maintainer (OWNER/MEMBER/COLLABORATOR) has commented /reuse-sweep-run on this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the sweep label will no longer automatically kick off new sweeps. Remove and re-add the label to force one.

添加 DSV4 FP4 B300 Dynamo TensorRT-LLM 配置及配套配方。
@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 关于重新运行失败任务的文档

1 similar comment
@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 关于重新运行失败任务的文档

在变更日志中补充拉取请求链接。
修正变更日志中的拉取请求链接。
Comment thread runners/launch_b300-nv.sh
Comment on lines +257 to 262
if [[ $FRAMEWORK == "dynamo-trt" && $MODEL_PREFIX == "dsv4" && "$MODEL_PATH" == /scratch/models/* ]]; then
SRTCTL_APPLY_ARGS+=(--no-preflight)
fi
if [[ $FRAMEWORK == "dynamo-sglang" && $MODEL_PREFIX == "dsv4" && "$MODEL_PATH" == /scratch/models/* ]]; then
SRTCTL_APPLY_ARGS+=(--no-preflight)
fi

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.

🟡 Lines 257-259 (dynamo-trt) and 260-262 (dynamo-sglang) in runners/launch_b300-nv.sh are two adjacent if blocks that are identical except for the FRAMEWORK check, both appending --no-preflight for MODEL_PREFIX==dsv4 with MODEL_PATH==/scratch/models/*. This is a minor DRY duplication that could be collapsed into one condition (MODEL_PREFIX==dsv4 && (FRAMEWORK==dynamo-trt || FRAMEWORK==dynamo-sglang) && MODEL_PATH==/scratch/models/*), matching the OR pattern already used at line 254 — non-blocking cleanup.

Extended reasoning...

What the duplication is: this PR adds a new if block at runners/launch_b300-nv.sh:257-259 that appends --no-preflight to SRTCTL_APPLY_ARGS when FRAMEWORK==dynamo-trt && MODEL_PREFIX==dsv4 && MODEL_PATH==/scratch/models/*. Immediately below it, at lines 260-262, sits the pre-existing block added for the sglang launch path that does the exact same thing, differing only in the FRAMEWORK check (dynamo-sglang instead of dynamo-trt):

if [[ $FRAMEWORK == "dynamo-trt" && $MODEL_PREFIX == "dsv4" && "$MODEL_PATH" == /scratch/models/* ]]; then
    SRTCTL_APPLY_ARGS+=(--no-preflight)
fi
if [[ $FRAMEWORK == "dynamo-sglang" && $MODEL_PREFIX == "dsv4" && "$MODEL_PATH" == /scratch/models/* ]]; then
    SRTCTL_APPLY_ARGS+=(--no-preflight)
fi

Why it happened: the comment above these blocks (lines 249-253) already documents the rationale for bypassing the preflight check for dsv4 models staged under /scratch on the login host — this PR just copy-pasted that block for the new dynamo-trt framework rather than widening the existing condition, since nothing in the diff or the file's structure forces two separate blocks.

Why the file's own convention argues for collapsing them: line 254 already uses an OR pattern for a similar preflight bypass ($FRAMEWORK == "dynamo-vllm" && $MODEL_PREFIX == "minimaxm3" ... && ( "$CONFIG_FILE" == ... || "$CONFIG_FILE" == ... )), so collapsing the two dsv4 blocks into MODEL_PREFIX==dsv4 && (FRAMEWORK==dynamo-trt || FRAMEWORK==dynamo-sglang) && MODEL_PATH==/scratch/models/* would be consistent with how the file already expresses multi-value conditions, and is a behavior-preserving change — both blocks only ever append the identical --no-preflight flag, with no divergent side effects.

Proof by walkthrough: for FRAMEWORK=dynamo-trt, MODEL_PREFIX=dsv4, MODEL_PATH=/scratch/models/DeepSeek-V4-Pro, the first if matches and appends --no-preflight; the second if (sglang) evaluates false and is skipped. For FRAMEWORK=dynamo-sglang with the same prefix/path, the roles reverse. In both cases exactly one block fires and the net effect on SRTCTL_APPLY_ARGS is identical to what a single merged || condition would produce — confirming the two blocks are truly redundant rather than serving distinct purposes.

Impact and fix: this has no effect on correctness or CI behavior today — it's pure duplication. The risk is only that a third dsv4 framework (e.g. a future dynamo-vllm bypass) would likely be added as yet another copy-pasted block instead of extending a single shared condition. Collapsing now to if [[ $MODEL_PREFIX == "dsv4" && ( $FRAMEWORK == "dynamo-trt" || $FRAMEWORK == "dynamo-sglang" ) && "$MODEL_PATH" == /scratch/models/* ]]; then SRTCTL_APPLY_ARGS+=(--no-preflight); fi removes the near-duplicate and pre-empts that drift, at no cost to readability.

Comment thread perf-changelog.yaml Outdated
Comment on lines +5955 to +5964
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2577

- config-keys:
- dsv4-fp4-b300-dynamo-trt
description:
- "Add the DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM disaggregated configuration with STP and MTP recipe variants."
- "Run on b300 with the checkpoint at /scratch/models/DeepSeek-V4-Pro and batch_1/benchmark Slurm settings."
- "Stage the TensorRT-LLM recipes through launch_b300-nv.sh."
- "Image: nvcr.io#nvidia/tensorrt-llm/release:1.3.0rc23"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX

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.

🟡 The new dsv4-fp4-b300-dynamo-trt entry in perf-changelog.yaml uses a placeholder pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX instead of the real PR number. Since this is PR #2596, it should read pull/2596 to match every other entry in the file.

Extended reasoning...

What the bug is

The newly appended dsv4-fp4-b300-dynamo-trt entry at the end of perf-changelog.yaml ends with:

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX

XXX is a literal, unfilled placeholder. It was never substituted with the actual pull request number.

How it manifests / code path

perf-changelog.yaml is an append-only changelog: every entry records the config keys it introduces, a human-readable description of the change, and a pr-link back to the originating PR so that a later reader (or tool) can trace why a given recipe was added or changed. Scanning the rest of the file confirms every other entry follows this convention with a concrete PR number, e.g.:

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2577
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2578
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2580

This PR is #2596 (per the PR metadata), so the new entry should read pull/2596.

Why nothing else catches this

There is no schema validation or lint step enforcing that pr-link resolves to a real, matching PR number — the file is plain YAML and the checklist item only requires that an entry be appended, not that its pr-link be correct. So a copy-pasted or templated placeholder like XXX silently slips through.

Impact

Low severity: this is a pure metadata/traceability defect. It does not affect benchmark execution, recipe correctness, or any runtime behavior. However, https://github.com/SemiAnalysisAI/InferenceX/pull/XXX is a dead link, and anyone (or any tooling) auditing the changelog to find the PR that introduced dsv4-fp4-b300-dynamo-trt will hit a 404 instead of the actual PR.

Step-by-step proof

  1. Open perf-changelog.yaml and jump to the last entry (lines 5955-5964).
  2. Note config-keys: [dsv4-fp4-b300-dynamo-trt] — this is the new entry added by this PR.
  3. Note the final line: pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX.
  4. Compare against the immediately preceding entry (lines ~5940-5953), which has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2577 — a real, resolvable PR number.
  5. Check the PR metadata for this change: it is PR Add DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM recipes / 添加 DeepSeek-V4-Pro FP4 B300 Dynamo TensorRT-LLM 配方 #2596.
  6. Therefore the correct value is pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2596.

Fix

Replace XXX with 2596:

  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2596

@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