Skip to content

[None][doc] Explain benchmark output token length - #17249

Open
zcxGGmu wants to merge 3 commits into
NVIDIA:mainfrom
zcxGGmu:docs/benchmark-output-token-length
Open

[None][doc] Explain benchmark output token length#17249
zcxGGmu wants to merge 3 commits into
NVIDIA:mainfrom
zcxGGmu:docs/benchmark-output-token-length

Conversation

@zcxGGmu

@zcxGGmu zcxGGmu commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • Add a serving benchmark note explaining why actual output token metrics can be lower than the requested synthetic output length.
  • Clarify that requested output length is a maximum generation budget, not a per-request guarantee.
  • Point fixed-OSL benchmark users to --ignore-eos / equivalent ignore-EOS settings when supported.

Test Plan

  • Static doc smoke check for the new output-token-length note and required terms
  • git diff --check
  • Not run: full Sphinx build (python -m sphinx --version fails locally with No module named sphinx)

Refs #8655

Dev Engineer Review

  • Added the standard 2026 NVIDIA SPDX copyright and license header.
  • Documented output length as a maximum generation budget.
  • Documented early EOS termination and actual output token metrics.
  • Documented --ignore-eos handling and the TensorRT-LLM min_length = output_len translation.
  • Documented equivalent settings for external clients.
  • Documented that the AIPerf example has no equivalent ignore-EOS setting.
  • The change is documentation-only and introduces no API, configuration, or code changes.
  • Static documentation checks, reference checks, the added-line secret scan, and git diff --check pass.
  • Full Sphinx validation was not run because Sphinx is unavailable locally.

QA Engineer Review

No test changes.

Signed-off-by: zq <zhouquan1511@163.com>
@zcxGGmu
zcxGGmu requested a review from a team as a code owner August 4, 2026 08:26
@zcxGGmu
zcxGGmu requested review from arysef and nv-guomingz August 4, 2026 08:26
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 492b9944-c97a-4ab5-bcf1-c89f09fbf7d5

📥 Commits

Reviewing files that changed from the base of the PR and between 3d7d82d and 95ed3b5.

📒 Files selected for processing (1)
  • docs/source/commands/trtllm-serve/run-benchmark-with-trtllm-serve.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/source/commands/trtllm-serve/run-benchmark-with-trtllm-serve.md

Walkthrough

The benchmark guide adds an SPDX license header and documents maximum output lengths, early termination, actual generated-token metrics, --ignore-eos, external-client configuration, and AIPerf behavior.

Changes

Benchmark documentation

Layer / File(s) Summary
Output length and EOS guidance
docs/source/commands/trtllm-serve/run-benchmark-with-trtllm-serve.md
Adds an SPDX license header. Documents EOS handling, actual generated-token metrics, --ignore-eos with min_length, external-client configuration, and the AIPerf limitation.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: nv-guomingz, arysef

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description explains the documentation change, its purpose, and validation steps, although it uses Summary and Test Plan headings instead of the template headings.
Title check ✅ Passed The title follows the required format and clearly describes the documentation change about benchmark output token length.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/source/commands/trtllm-serve/run-benchmark-with-trtllm-serve.md`:
- Around line 162-165: Update the documentation paragraph around
benchmark_serving.py and backend_request_func.py to state that ignore_eos is
passed through RequestFuncInput and translated by the TRT-LLM request path into
min_length = output_len, rather than forwarded as a literal ignore_eos request
field.
- Around line 152-169: Add the repository’s standard NVIDIA copyright header at
the beginning of run-benchmark-with-trtllm-serve.md, within the first 12 lines,
and set its copyright year to 2026. Leave the existing benchmark documentation
unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a538ef78-c0df-4f45-acaf-adb255683508

📥 Commits

Reviewing files that changed from the base of the PR and between 2ff7e8b and 1e10c7a.

📒 Files selected for processing (1)
  • docs/source/commands/trtllm-serve/run-benchmark-with-trtllm-serve.md

Comment thread docs/source/commands/trtllm-serve/run-benchmark-with-trtllm-serve.md Outdated

@brnguyen2 brnguyen2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads well and the --ignore-eos claim matches the example at line 105. Two placement notes:

The AIPerf example (examples/serve/aiperf_client.sh, quoted at line 395) sets --output-tokens-mean 128 with no ignore-EOS input, so it's the case most likely to show short outputs — but the new note is ~200 lines above it. Consider a one-line pointer in the AIPerf section, or adding the equivalent --extra-inputs ignore_eos:true to that example script if AIPerf supports it.

budget, not a guarantee that every request will generate exactly that many
tokens. A request can finish earlier if the model emits an EOS token, reaches a
stop condition, or if the benchmark client/server applies stop-token handling.
In that case, metrics such as `Output Sequence Length (tokens)` or `Total

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Output Sequence Length (tokens) doesn't appear in any TensorRT-LLM benchmark output — benchmark_serving.py prints Total generated tokens (see the sample output at line 126), and grep finds no such label under tensorrt_llm/. That name is an AIPerf/GenAI-Perf metric. Either drop it here or attribute it explicitly to the external clients.

Signed-off-by: zq <zhouquan1511@163.com>
@zcxGGmu

zcxGGmu commented Aug 5, 2026

Copy link
Copy Markdown
Author

Thanks for the review — I pushed 3d7d82d53 addressing the feedback.

Changes made:

  • Added the standard 2026 NVIDIA SPDX copyright/license header to the benchmark doc.
  • Clarified that benchmark_serving.py --ignore-eos passes through RequestFuncInput and the TensorRT LLM request path translates it to min_length = output_len.
  • Added a note next to the AIPerf example that it does not pass an ignore-EOS equivalent, so output-length metrics can be shorter than --output-tokens-mean when EOS is emitted early.

Validation:

  • python - <<'PY' ... doc guard — passed, required header and ignore-EOS/AIPerf phrases are present.
  • git grep -n "min_length\|RequestFuncInput\|ignore_eos" -- tensorrt_llm/serve/scripts — confirmed the documented TensorRT LLM request path.
  • git diff --check — passed.
  • Added-line secret scan — passed, no credential-like additions.

Signed-off-by: zq <zhouquan1511@163.com>
@zcxGGmu

zcxGGmu commented Aug 6, 2026

Copy link
Copy Markdown
Author

Thanks for the review — I pushed 95ed3b527 addressing the feedback.

Changes made:

  • Replaced the non-TensorRT-LLM output label with the actual benchmark_serving.py label, Total generated tokens.
  • Clarified that external clients may use different labels for the same actual-generated-token concept.

Validation:

  • python .hermes/pr-feedback-monitor/validate_pr17249_docs.py — passed static doc guards, source label check, git diff --check, and added-line credential scan.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants