[None][doc] document dtype='auto' resolution for model and KV cache#16240
[None][doc] document dtype='auto' resolution for model and KV cache#16240lonexreb wants to merge 2 commits into
Conversation
Expand the Field descriptions for the model dtype and KvCacheConfig.dtype so users can predict the resolved value without reading the source: - model dtype 'auto' infers from the checkpoint (HF torch_dtype/dtype), falling back to bfloat16; any other value forces a torch dtype. - KvCacheConfig.dtype 'auto' follows the checkpoint's quant metadata (leaves quant_config.kv_cache_quant_algo untouched); 'fp8'/'nvfp4' override it. Fixes NVIDIA#15512 Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughUpdates Changesdtype documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
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: 3
🤖 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_args.py`:
- Around line 2493-2494: The documentation text near the LLM argument
description references the deprecated `--extra_llm_api_options` option; update
it to use the preferred `--config` option while preserving the surrounding
explanation about configuration files and quantization settings.
- Around line 2490-2492: Update the `kv_cache_dtype` documentation in `LLMArgs`
to clarify that `auto` follows checkpoint quantization metadata and may populate
`quant_config.kv_cache_quant_algo` when it is unset, while preserving any
explicitly configured value; retain that `fp8` and `nvfp4` override the setting.
- Around line 2810-2813: Update the dtype argument documentation near
BaseLlmArgs.validate_dtype() to state that although “auto” falls back to
bfloat16 when unspecified, it is converted to float16 on GPUs with compute
capability below SM80; retain the existing description of explicit torch dtype
values.
🪄 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: 0a225d25-b656-4087-862c-3b35d013f40c
📒 Files selected for processing (1)
tensorrt_llm/llmapi/llm_args.py
| "The data type to use for the KV cache. 'auto' (default) follows the checkpoint's own " | ||
| "quantization metadata, leaving quant_config.kv_cache_quant_algo untouched. 'fp8' or " | ||
| "'nvfp4' override it, setting quant_config.kv_cache_quant_algo to that KV-cache " |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify that auto may populate quant_config.kv_cache_quant_algo.
The construction path sets quant_config.kv_cache_quant_algo from checkpoint metadata when it is unset; auto only avoids overriding an explicitly configured value.
Suggested wording
- "quantization metadata, leaving quant_config.kv_cache_quant_algo untouched. 'fp8' or "
+ "quantization metadata. If quant_config.kv_cache_quant_algo is unset, it may be "
+ "populated from the checkpoint; 'fp8' or "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "The data type to use for the KV cache. 'auto' (default) follows the checkpoint's own " | |
| "quantization metadata, leaving quant_config.kv_cache_quant_algo untouched. 'fp8' or " | |
| "'nvfp4' override it, setting quant_config.kv_cache_quant_algo to that KV-cache " | |
| "The data type to use for the KV cache. 'auto' (default) follows the checkpoint's own " | |
| "quantization metadata. If quant_config.kv_cache_quant_algo is unset, it may be " | |
| "populated from the checkpoint; 'fp8' or " | |
| "'nvfp4' override it, setting quant_config.kv_cache_quant_algo to that KV-cache " |
🤖 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_args.py` around lines 2490 - 2492, Update the
`kv_cache_dtype` documentation in `LLMArgs` to clarify that `auto` follows
checkpoint quantization metadata and may populate
`quant_config.kv_cache_quant_algo` when it is unset, while preserving any
explicitly configured value; retain that `fp8` and `nvfp4` override the setting.
| "quantization algorithm. Applied at LLM construction time, including via " | ||
| "trtllm-serve --extra_llm_api_options.") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the preferred --config option in the documentation.
The repository guidelines require --config instead of --extra_llm_api_options when documenting configuration files.
- "trtllm-serve --extra_llm_api_options.")
+ "trtllm-serve --config.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "quantization algorithm. Applied at LLM construction time, including via " | |
| "trtllm-serve --extra_llm_api_options.") | |
| "quantization algorithm. Applied at LLM construction time, including via " | |
| "trtllm-serve --config.") |
🤖 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_args.py` around lines 2493 - 2494, The documentation
text near the LLM argument description references the deprecated
`--extra_llm_api_options` option; update it to use the preferred `--config`
option while preserving the surrounding explanation about configuration files
and quantization settings.
Source: Coding guidelines
| "The data type to use for the model. 'auto' (default) infers the dtype from the " | ||
| "checkpoint: the HF config's 'torch_dtype' (or the deprecated 'dtype' field), falling " | ||
| "back to 'bfloat16' when the checkpoint specifies none. Any other value must be a valid " | ||
| "torch dtype string (e.g. 'float16', 'bfloat16') and forces that dtype." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the pre-SM80 float16 override.
BaseLlmArgs.validate_dtype() converts 'auto' to 'float16' on GPUs with compute capability below 8.0, so the documented fallback to bfloat16 is not universal.
Suggested wording
"back to 'bfloat16' when the checkpoint specifies none. Any other value must be a valid "
- "torch dtype string (e.g. 'float16', 'bfloat16') and forces that dtype."
+ "torch dtype string (e.g. 'float16', 'bfloat16') and forces that dtype. On GPUs with "
+ "compute capability below 8.0, 'auto' is overridden to 'float16'."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "The data type to use for the model. 'auto' (default) infers the dtype from the " | |
| "checkpoint: the HF config's 'torch_dtype' (or the deprecated 'dtype' field), falling " | |
| "back to 'bfloat16' when the checkpoint specifies none. Any other value must be a valid " | |
| "torch dtype string (e.g. 'float16', 'bfloat16') and forces that dtype." | |
| "The data type to use for the model. 'auto' (default) infers the dtype from the " | |
| "checkpoint: the HF config's 'torch_dtype' (or the deprecated 'dtype' field), falling " | |
| "back to 'bfloat16' when the checkpoint specifies none. Any other value must be a valid " | |
| "torch dtype string (e.g. 'float16', 'bfloat16') and forces that dtype. On GPUs with " | |
| "compute capability below 8.0, 'auto' is overridden to 'float16'." |
🤖 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_args.py` around lines 2810 - 2813, Update the dtype
argument documentation near BaseLlmArgs.validate_dtype() to state that although
“auto” falls back to bfloat16 when unspecified, it is converted to float16 on
GPUs with compute capability below SM80; retain the existing description of
explicit torch dtype values.
…config - Document that 'auto' model dtype resolves to float16 on pre-Ampere (compute capability < 8.0) GPUs (BaseLlmArgs.validate_dtype). - Clarify KV cache 'auto' may populate quant_config.kv_cache_quant_algo from the checkpoint when unset (preserving an explicit value). - Use the preferred 'trtllm-serve --config' over --extra_llm_api_options. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
|
Addressed in 6901f55: documented the pre-SM80 |
1 participant
Description
Fixes #15512.
The
dtypeandKvCacheConfig.dtypedocstrings said "auto" without explaining what it resolves to, forcing users to readllm_args.py/model_config.pyto predict the effective dtype.Change
Expand both
Fielddescriptions (docstring-only, no behavior change):dtype:'auto'infers from the checkpoint — HF configtorch_dtype(or deprecateddtype), falling back tobfloat16when unspecified (model_config.pytorch_dtypeproperty); any other value forces a valid torch dtype.KvCacheConfig.dtype:'auto'follows the checkpoint's own quant metadata, leavingquant_config.kv_cache_quant_algountouched;'fp8'/'nvfp4'override it (verified insync_quant_config_with_kv_cache_config_dtype).Note
Deliberately omitted the reporter's mentioned pre-Ampere
float16override — it is not present in the current PyTorchmodel_config.pypath and the reporter also could not verify it, so documenting it would be inaccurate.Test
Docstring-only; no code path changes.
tests/unittest/api_stabilitysnapshots onlyannotation/default(not descriptions), so it is unaffected.Summary by CodeRabbit
trtllm-serve.bfloat16.