[#10710][fix] clarify and align trtllm-bench runtime logging - #15254
Conversation
Ensure explicit benchmark runtime flags (max_batch_size / max_num_tokens) are honored independently of one another and reflected in the runtime settings after the config merge, for both throughput and low-latency paths. Also clarify get_settings logging to distinguish user-provided from heuristic values. Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR refactors benchmark runtime configuration to explicitly propagate user-provided batch size and token limits. The ChangesBenchmark Settings Override Flow
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@tensorrt_llm/bench/benchmark/utils/general.py`:
- Around line 117-118: The CLI override checks currently use truthiness (e.g.,
"if user_max_batch_size and user_max_num_tokens:") so explicit zeros are treated
as missing; update those conditionals to use explicit presence checks like "is
not None" (for example change the check for user_max_batch_size and
user_max_num_tokens to "if user_max_batch_size is not None and
user_max_num_tokens is not None:") and do the same for the other user_* CLI
variables in this module (replace truthy checks with "is not None" or validate
as positive with a clear error) so zero values are honored or rejected
explicitly.
- Around line 117-121: The branch that sets max_batch_size, max_num_tokens from
user_max_batch_size/user_max_num_tokens skips the chunked-prefill minimum-token
floor currently enforced in the later else block; update the user-provided
branch to apply the same floor logic to max_num_tokens (i.e., run the same
min/max adjustment used in the else block that enforces the chunked-prefill
floor when chunked prefill is disabled) so max_num_tokens is raised to the
required minimum before returning/using it; ensure you reference and adjust the
same variables max_num_tokens (from user_max_num_tokens) and max_batch_size
(from user_max_batch_size) and reuse the existing floor-check logic rather than
duplicating inconsistent code.
🪄 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: 79801fd6-1fce-49c1-a0ad-56d5a4605376
📒 Files selected for processing (3)
tensorrt_llm/bench/benchmark/low_latency.pytensorrt_llm/bench/benchmark/throughput.pytensorrt_llm/bench/benchmark/utils/general.py
|
PR_Github #53524 [ run ] triggered by Bot. Commit: |
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot kill |
marinayanov
changed the title
|
PR_Github #53528 [ kill ] triggered by Bot. Commit: |
|
PR_Github #53524 [ run ] completed with state |
|
PR_Github #53528 [ kill ] completed with state |
Use `is not None` for max_batch_size / max_num_tokens presence checks so an explicit 0 is not treated as "not provided". Enforce the chunked-prefill max_num_tokens floor on every path (including when both limits are user-provided) and only warn when the value is actually below the floor. Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
|
/bot run |
|
PR_Github #53667 [ run ] triggered by Bot. Commit: |
|
PR_Github #53667 [ run ] completed with state
|
|
PR_Github #53945 [ run ] triggered by Bot. Commit: |
|
PR_Github #53945 [ run ] completed with state
|
galagam
left a comment
galagam
left a comment
There was a problem hiding this comment.
Overall looks good, see comments
…h_size and max_num_tokens, ensuring defaults are preserved Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
|
/bot run |
|
PR_Github #55236 [ run ] triggered by Bot. Commit: |
|
PR_Github #55236 [ run ] completed with state
|
|
/bot run |
|
PR_Github #55243 [ run ] triggered by Bot. Commit: |
|
PR_Github #55243 [ run ] completed with state
|
|
/bot run |
|
PR_Github #55249 [ run ] triggered by Bot. Commit: |
|
PR_Github #55249 [ run ] completed with state |
…VIDIA#15254) Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com> Signed-off-by: GitLab CI Bot <gitlab-ci@nvidia.com>
…VIDIA#15254) Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
4 participants
Description
Follow-up to #14812. That PR made explicit CLI flags win over YAML config; this
PR fixes the logging and reporting of
max_batch_size/max_num_tokensintrtllm-benchso what's printed matches what the engine actually runs with.Problem
After #14812, the values reported by
trtllm-benchcould diverge from thevalues the engine actually used:
Single CLI flag silently ignored.
get_settings()only honored--max_batch_size/--max_num_tokenswhen both were passed. Providingjust one fell back to the heuristic for both, discarding the user's value
with no clear log.
Report showed pre-merge values.
settings_configwas populated beforethe
extra_llm_api_optionsYAML merge inget_llm_args(), but the finalreport reads from
settings_config. So when YAML supplied a value the userdidn't set on the CLI, the report (e.g.
Max Runtime Tokens) showed thestale pre-merge number while the engine ran the merged one.
Ambiguous logging. The settings log didn't make clear which values were
user-provided vs. heuristic, and why the final report might differ.
Changes
utils/general.py: honor a single CLI flag — use the user value for theone provided and the heuristic for the other. Unified all four cases into a
consistent
Initial settings: max_batch_size=... (user-provided|heuristic), max_num_tokens=... (user-provided|heuristic).line so each value's source isexplicit and the "initial/pre-merge" nature is clear.
throughput.py/low_latency.py: syncsettings_config.max_batch_sizeand
max_num_tokensto the post-mergekwargs(afterget_llm_args(),before
get_llm()), so the final report reflects the values actually passedto the engine.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
[ v] Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Summary by CodeRabbit