fix(trtllm): honor stop_strings on both TRT-LLM generation paths - #4041
Open
yupengtang wants to merge 1 commit into
Open
fix(trtllm): honor stop_strings on both TRT-LLM generation paths#4041yupengtang wants to merge 1 commit into
yupengtang wants to merge 1 commit into
Conversation
`stop_strings` is part of the shared generation contract -- it is declared on `GenerationConfig` and again per sample on `GenerationDatumSpec` -- and vLLM, SGLang and Dynamo all read it. TRT-LLM built its `SamplingParams` with `stop_token_ids` and never `stop`, on the direct path and over HTTP, so the same config stopped generation on the other three backends and ran on to `max_new_tokens` here. The two controls are not interchangeable: `stop_token_ids` cannot express a multi-token boundary like `</answer>`, which is exactly the shape a chat or agentic rollout stops on. Nothing errors, so a run only shows it as rollouts that overrun their boundary. `TrtSamplingParams` already accepts `stop` alongside `stop_token_ids`, so both paths now pass it. The direct path merges the configured list with the per-sample ones the way `BaseVllmGenerationWorker._merge_stop_strings` does -- one `SamplingParams` is built per batch, so a sample's stop strings apply to the batch, matching vLLM rather than inventing a different rule. An empty result stays `None` so TRT-LLM keeps its own default instead of receiving an empty list. The HTTP helper gains the argument next to `stop_token_ids`, mirroring how NVIDIA-NeMo#3537 threaded the sampling config through after the same drift was found in `top_k`. Signed-off-by: Yupeng Tang <85978465+yupengtang@users.noreply.github.com>
4 tasks
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 does this PR do ?
Makes the TRT-LLM backend honor
stop_strings, which it silently dropped on both of its generation paths.stop_stringsis part of the shared generation contract: it is declared onGenerationConfig(interfaces.py:227) and again per sample onGenerationDatumSpec(:336). vLLM, SGLang and Dynamo all read it. TRT-LLM builds itsSamplingParamswithstop_token_idsand neverstop, so the same config stops generation on the other three backends and runs on tomax_new_tokenshere.The two controls are not interchangeable.
stop_token_idscannot express a multi-token boundary like</answer>, which is exactly the shape a chat or agentic rollout stops on. Nothing raises, so a run shows this only as rollouts that overrun the boundary the config asked for.Both TRT-LLM paths are affected:
generate_async()stop_token_idsonlystopfrom config ∪ per-samplestop_token_idsonlystopfrom configMeasured on the direct path with
stop_strings: ["</answer>"]configured:Issues
No issue filed; found while comparing which generation-config keys each backend reads. TRT-LLM was the only backend not reading
stop_strings.Usage
No config or API change.
policy.generation.stop_stringssimply takes effect on TRT-LLM now, as it already did elsewhere.Design & Code Changes
tensorrt_llm.SamplingParamsalready acceptsstopalongsidestop_token_ids(sampling_params.py:stop: Optional[Union[str, List[str]]] = None), so this is a plumbing fix rather than a new capability.trtllm_worker_async.py: adds_merge_stop_strings, mirroringBaseVllmGenerationWorker._merge_stop_strings. OneSamplingParamsis built per batch here, so a sample's stop strings apply to the batch, the same shape vLLM has. I matched the reference backend rather than inventing a stricter per-sample rule.trtllm_http_server.py:_build_sampling_paramsgainsstop_stringsnext tostop_token_ids, threaded throughcreate_appandstart_server, mirroring how fix(trtllm): Align HTTP server sampling params, use generate_async #3537 threaded the sampling config after the same drift was found intop_k.Nonerather than[], so TRT-LLM keeps its own default.Before your PR is "Ready for review"
Pre checks:
Additional Information
Tests
Five added, and the three existing assertions that pin the exact
SamplingParamskwargs updated for the newstop.test_trtllm_worker_async.py(run with--trtllm-only):test_build_sampling_params_forwards_configured_stop_stringstest_merge_stop_strings_unions_config_and_per_sampletest_merge_stop_strings_returns_none_when_nothing_configuredtest_trtllm_http_server.py(these carry notrtllmmarker, so they run without the flag):test_http_sampling_params_forward_stop_stringstest_http_sampling_params_empty_stop_strings_stay_noneNo failures on either version. The 5 skips are local: my stub
tensorrt_llmhas notensorrt_llm.servesubmodule.CPU only. I don't have a GPU box, so the tests needing a real TRT-LLM runtime did not run here and I could not exercise a rollout end to end. The change is confined to how
SamplingParamsis constructed, which is what the tests above cover.ruff 0.9.9check, import sort and format are clean on all four files.No docs change:
docs/design-docs/sampling-params.mddocumentstop_kspelling across backends and does not enumerate stop-string behaviour. Happy to add a line there if you would rather have it recorded.