[TRTLLM-14953][feat] Add ability to honour generation_config.json sampling defaults - #17213
Conversation
DomBrown
added
the
api-compatible
|
/bot run |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds opt-in loading of explicit sampling values from model ChangesGeneration Configuration Sampling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant _TorchLLM
participant ModelLoader
participant OpenAIProtocol
participant SamplingParams
participant BaseLLM
_TorchLLM->>ModelLoader: load_hf_generation_config_dict(model_dir)
ModelLoader-->>_TorchLLM: explicit generation values
_TorchLLM->>BaseLLM: store raw generation-config values
OpenAIProtocol->>SamplingParams: construct request parameters
OpenAIProtocol->>SamplingParams: record non-None request fields
BaseLLM->>SamplingParams: apply generation-config defaults
SamplingParams-->>BaseLLM: validated sampling parameters
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 4
🧹 Nitpick comments (3)
tests/unittest/llmapi/test_sampling_params.py (1)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a precise mapping type.
Line 41 uses bare
dict. Use a parameterized built-in generic such asMapping[str, object].As per coding guidelines, use precise built-in generic types and avoid unparameterized container types.
🤖 Prompt for 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. In `@tests/unittest/llmapi/test_sampling_params.py` around lines 38 - 42, Update the generation_config_explicit_values parameter in _apply_generation_config_sampling_defaults to use a precise parameterized mapping type, such as Mapping[str, object], instead of bare dict; preserve the function’s existing behavior.Source: Coding guidelines
tensorrt_llm/llmapi/llm_utils.py (1)
314-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the repository typing style and document the public helper.
Use
str | Pathanddict[str, Any]. Add Google-styleArgsandReturnssections formodel_dirand the returned mapping.As per coding guidelines, “Prefer docstrings for external interfaces” and “prefer built-in generic types.”
🤖 Prompt for 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. In `@tensorrt_llm/llmapi/llm_utils.py` around lines 314 - 316, Update load_hf_generation_config_dict to use the built-in type annotations str | Path and dict[str, Any]. Expand its public docstring with Google-style Args and Returns sections documenting model_dir and the returned generation-configuration mapping.Source: Coding guidelines
tests/unittest/llmapi/test_llm_utils.py (1)
31-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate all new test functions.
tests/unittest/llmapi/test_llm_utils.py#L31-L43: add-> Nonereturn annotations and typetmp_pathif typed fixtures are used.tests/unittest/llmapi/test_llm_args.py#L71-L79: add-> Nonetotest_generation_config_mode_defaults_and_validation.tests/unittest/llmapi/test_llm_args.py#L2455-L2479: add-> NonetoTestServeDefaults.test_serve_generation_config_cli_over_yaml_precedence.As per coding guidelines, “Annotate every function.”
🤖 Prompt for 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. In `@tests/unittest/llmapi/test_llm_utils.py` around lines 31 - 43, Annotate all specified new test functions with a None return type: tests/unittest/llmapi/test_llm_utils.py lines 31-43, including typing tmp_path if the fixture is explicitly typed; tests/unittest/llmapi/test_llm_args.py lines 71-79 for test_generation_config_mode_defaults_and_validation; and lines 2455-2479 for TestServeDefaults.test_serve_generation_config_cli_over_yaml_precedence.Source: Coding guidelines
🤖 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/commands/serve.py`:
- Around line 983-989: Ensure the _autodeploy backend does not silently accept
the --generation-config option without applying its sampling defaults: either
reject the option during backend validation or implement generation-config
handling for _autodeploy. Update the relevant validation or generation-defaults
logic near the generation_config option and add a regression test covering
_autodeploy behavior.
In `@tests/unittest/llmapi/test_llm_args.py`:
- Around line 71-79: Update test_generation_config_mode_defaults_and_validation
and TestServeDefaults.test_serve_generation_config_cli_over_yaml_precedence to
include -> None return annotations. In the precedence test, invoke the Click
serve command with --generation-config auto instead of passing explicit_cli_keys
directly, so the test exercises collect_explicit_cli_keys and CLI-over-YAML
precedence while preserving its existing assertions.
In `@tests/unittest/llmapi/test_llm_utils.py`:
- Around line 31-43: Extend the ModelLoader.load_hf_generation_config_dict tests
with cases for malformed JSON and a top-level JSON array in
generation_config.json. Assert the expected handling for each invalid
configuration, and ensure both new test names are included in
tests/integration/test_lists/test-db/l0_a10.yml.
In `@tests/unittest/llmapi/test_sampling_params.py`:
- Around line 85-103: Extend
test_generation_config_applies_all_supported_sampling_fields to cover a null
generation-config value and early_stopping set to "never". Assert that null
preserves the existing SamplingParams default and "never" is handled according
to the dedicated sampling-parameter behavior without passing an unsupported
value through to TRT-LLM.
---
Nitpick comments:
In `@tensorrt_llm/llmapi/llm_utils.py`:
- Around line 314-316: Update load_hf_generation_config_dict to use the built-in
type annotations str | Path and dict[str, Any]. Expand its public docstring with
Google-style Args and Returns sections documenting model_dir and the returned
generation-configuration mapping.
In `@tests/unittest/llmapi/test_llm_utils.py`:
- Around line 31-43: Annotate all specified new test functions with a None
return type: tests/unittest/llmapi/test_llm_utils.py lines 31-43, including
typing tmp_path if the fixture is explicitly typed;
tests/unittest/llmapi/test_llm_args.py lines 71-79 for
test_generation_config_mode_defaults_and_validation; and lines 2455-2479 for
TestServeDefaults.test_serve_generation_config_cli_over_yaml_precedence.
In `@tests/unittest/llmapi/test_sampling_params.py`:
- Around line 38-42: Update the generation_config_explicit_values parameter in
_apply_generation_config_sampling_defaults to use a precise parameterized
mapping type, such as Mapping[str, object], instead of bare dict; preserve the
function’s existing behavior.
🪄 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: 8b9f49df-b70d-49cd-9b46-11daa3f92a5b
📒 Files selected for processing (14)
docs/source/features/sampling.mdtensorrt_llm/commands/serve.pytensorrt_llm/llmapi/llm.pytensorrt_llm/llmapi/llm_args.pytensorrt_llm/llmapi/llm_utils.pytensorrt_llm/sampling_params.pytensorrt_llm/serve/openai_protocol.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/integration/test_lists/test-db/l0_a10.ymltests/unittest/api_stability/references/llm.yamltests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/llmapi/test_llm_args.pytests/unittest/llmapi/test_llm_utils.pytests/unittest/llmapi/test_sampling_params.py
|
PR_Github #63470 [ run ] triggered by Bot. Commit: |
|
/bot kill |
|
PR_Github #63470 [ run ] completed with state
|
|
PR_Github #63480 [ kill ] triggered by Bot. Commit: |
|
PR_Github #63480 [ kill ] completed with state |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tensorrt_llm/llmapi/llm_utils.py (1)
313-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public helper contract.
load_hf_generation_config_dictis a public static method, but its docstring does not documentmodel_diror the return behavior. Add Google-styleArgsandReturnssections. State that non-object JSON and read failures return{}.Proposed docstring
- """Load only values explicitly present in generation_config.json.""" + """Load explicitly provided values from generation_config.json. + + Args: + model_dir: Local model directory containing the configuration file. + + Returns: + Explicit configuration values, or an empty dictionary when the + file is missing or invalid. + """🤖 Prompt for 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. In `@tensorrt_llm/llmapi/llm_utils.py` around lines 313 - 316, Update the docstring for the public static method load_hf_generation_config_dict to use Google-style Args and Returns sections, documenting the model_dir parameter and that the method returns explicitly configured generation values while returning {} for non-object JSON or read failures.Source: Coding guidelines
🤖 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/llmapi/llm_utils.py`:
- Around line 319-327: The except clause handling the generation config file
loading does not catch UnicodeDecodeError, which can be raised by open(..., "r")
when the file contains invalid UTF-8 encoding. Add UnicodeDecodeError to the
exception tuple in the except clause that currently catches (OSError,
json.JSONDecodeError) so that invalid UTF-8 configuration files are handled the
same way as other file-loading errors, returning {} and logging the warning.
---
Nitpick comments:
In `@tensorrt_llm/llmapi/llm_utils.py`:
- Around line 313-316: Update the docstring for the public static method
load_hf_generation_config_dict to use Google-style Args and Returns sections,
documenting the model_dir parameter and that the method returns explicitly
configured generation values while returning {} for non-object JSON or read
failures.
🪄 Autofix
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: 05edf171-90f3-4722-942e-e4f004cd18cb
📒 Files selected for processing (15)
docs/source/features/sampling.mdtensorrt_llm/_torch/auto_deploy/llm_args.pytensorrt_llm/commands/serve.pytensorrt_llm/llmapi/llm.pytensorrt_llm/llmapi/llm_args.pytensorrt_llm/llmapi/llm_utils.pytensorrt_llm/sampling_params.pytensorrt_llm/serve/openai_protocol.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/integration/test_lists/test-db/l0_a10.ymltests/unittest/api_stability/references/llm.yamltests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/llmapi/test_llm_args.pytests/unittest/llmapi/test_llm_utils.pytests/unittest/llmapi/test_sampling_params.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tensorrt_llm/usage/llm_args_golden_manifest.json
- tests/unittest/api_stability/references/trtllm_serve_cli.yaml
- docs/source/features/sampling.md
- tensorrt_llm/llmapi/llm_args.py
- tests/unittest/llmapi/test_llm_args.py
- tensorrt_llm/sampling_params.py
- tensorrt_llm/serve/openai_protocol.py
- tensorrt_llm/llmapi/llm.py
- tests/unittest/llmapi/test_sampling_params.py
- tests/integration/test_lists/test-db/l0_a10.yml
- tests/unittest/api_stability/references/llm.yaml
- tensorrt_llm/commands/serve.py
- tensorrt_llm/_torch/auto_deploy/llm_args.py
juney-nvidia
left a comment
juney-nvidia
left a comment
There was a problem hiding this comment.
Approved from API perspective only.
|
/bot run |
|
PR_Github #64071 [ run ] triggered by Bot. Commit: |
|
PR_Github #64071 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64127 [ run ] triggered by Bot. Commit: |
|
PR_Github #64127 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64275 [ run ] triggered by Bot. Commit: |
|
PR_Github #64275 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64290 [ run ] triggered by Bot. Commit: |
|
PR_Github #64290 [ run ] completed with state
|
Allow PyTorch requests to opt into model-provided sampling defaults while preserving existing TRT-LLM behavior by default. Signed-off-by: Dom Brown <3886319+DomBrown@users.noreply.github.com>
Reject unsupported AutoDeploy defaults and cover CLI precedence and invalid generation configuration values. Signed-off-by: Dom Brown <3886319+DomBrown@users.noreply.github.com>
Signed-off-by: Dom Brown <3886319+DomBrown@users.noreply.github.com>
Signed-off-by: Dom Brown <3886319+DomBrown@users.noreply.github.com>
637013c to
dd072d2
Compare
|
/bot run |
|
PR_Github #64345 [ run ] triggered by Bot. Commit: |
|
PR_Github #64345 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64360 [ run ] triggered by Bot. Commit: |
|
PR_Github #64360 [ run ] completed with state |
11 participants
Dev Engineer Review
generation_config="auto"support for PyTorch sampling.generation_config="trtllm"as the default behavior.trtllm-serve, YAML configuration, and OpenAI serving requests."trtllm".QA Engineer Review
test_sampling_params.py.test_llm_args.py.test_llm_utils.py.test_sampling_params.pyandtest_llm_args.pyare covered bytests/integration/test_lists/test-db/l0_a10.yml.test_llm_utils.pyis not listed in atest-db/orqa/file.Description
generation_config=autosupport for PyTorch sampling.generation_config=trtllm.This is a client request from Poolside and enables behaviour in line with that provided by vLLM.
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.
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.