fix(trt_llm): BEI max_num_tokens upgrade is a no-op - #2559
Open
chuenchen309 wants to merge 1 commit into
Open
Conversation
_bei_specfic_migration logs that it is raising max_num_tokens to the BEI
minimum, but rebinds the local name instead of assigning the field:
self = self.model_copy(update={"max_num_tokens": BEI_REQUIRED_MAX_NUM_TOKENS})
The copy is discarded when the method returns, so the value stays as the user
set it:
TrussTRTLLMBuildConfiguration(base_model="encoder", max_num_tokens=1024, ...)
WARNING build.max_num_tokens=1024, upgrading to 16384
.max_num_tokens # 1024
Two things show this is a slip rather than intent. The next two lines of the
same method assign fields directly and do stick
(plugin_configuration.paged_kv_cache = False). And the only other model_copy
in the file assigns the result back to a field:
self.runtime = self.runtime.model_copy(update={...})
Assign the field. Non-encoder builds are unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
cretz
reviewed
Jul 17, 2026
cretz
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the contribution! One question below and also CLA will need to be signed (click link above), thanks!
| self = self.model_copy( | ||
| update={"max_num_tokens": BEI_REQUIRED_MAX_NUM_TOKENS} | ||
| ) | ||
| self.max_num_tokens = BEI_REQUIRED_MAX_NUM_TOKENS |
Contributor
There was a problem hiding this comment.
Thanks for the fix!
Can you help me understand how you came across this bug? Specifically the steps you took on the platform to trigger it?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
_bei_specfic_migrationlogs that it is raisingmax_num_tokensto the BEI minimum, but the value never changes:The upgrade rebinds the local name instead of assigning the field:
model_copyreturns a new object; rebindingselfinsidemodel_post_initdiscards it when the method returns.Why this looks like a slip rather than intent
model_copyin the file assigns the result back to a field:logger.warningannounces an upgrade that then doesn't happen.Impact
serving_image_builder.pynotes it is "enforcing max_num_tokens here and in engine-builder". The engine-builder half is what silently doesn't apply: the router is configured for the BEI minimum while the engine is built for the user's lower value.Fix
Assign the field. Verified across the paths:
The adjacent
paged_kv_cache = Falsestill applies.Tests
Two cases: the encoder path upgrades, the decoder path does not. The encoder one fails on
mainand passes here.grep max_num_tokens truss/tests/previously returned nothing — the field had no coverage, which is why this went unnoticed.pytest truss/tests/trt_llm/— 23 passed, 1 skippeduv run ruff check/ruff format --checkclean, per AGENTS.mdAI disclosure: drafted with Claude Code (Opus 4.8), including the root-cause trace and the tests. I ran the repro and the suite locally and reviewed the diff before opening.