[#10710][feat] Make explicit CLI flags take precedence over --config / --extra_llm_api_options YAML - #14812
Conversation
📝 WalkthroughWalkthroughThis pull request implements CLI-over-YAML precedence enforcement across TensorRT-LLM serve, eval, and benchmark commands. It adds parameter tracking to identify explicitly set command-line flags, updates configuration merging logic to preserve CLI values during deep-merge of nested configs, and integrates this mechanism into all affected commands with consistent help text updates and comprehensive test coverage. ChangesCLI Precedence Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (2)
tensorrt_llm/bench/dataclasses/configuration.py (1)
5-5: 💤 Low valueConsider using PEP 585 built-in generic
setinstead oftyping.Set.Same as the comment in
__init__.py: for Python 3.10+, preferset[str]overSet[str]as per coding guidelines. However, this file already usesDict,List, etc., fromtyping, so the current change is consistent with the existing style.♻️ Proposed refactor to use built-in generic
-from typing import Any, Dict, List, Literal, Optional, Set, Union +from typing import Any, Dict, List, Literal, Optional, UnionThen update line 39:
- explicit_cli_keys: Optional[Set[str]] = None + explicit_cli_keys: Optional[set[str]] = NoneAs per coding guidelines: "Prefer built-in types
list,dict,tupleover legacytyping.List,typing.Dict,typing.Tuple" (applies tosetas well).🤖 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/bench/dataclasses/configuration.py` at line 5, Replace typing.Set with the PEP 585 built-in generic set: remove Set from the import list on the top import line and change any type annotations that use typing.Set (the Set[...] occurrence referenced around the current line 39) to use the built-in set[...] form; keep other typing imports (Dict, List, etc.) unchanged to preserve file style.tensorrt_llm/bench/benchmark/__init__.py (1)
3-3: 💤 Low valueConsider using PEP 585 built-in generic
setinstead oftyping.Set.The coding guidelines state: "Prefer built-in types
list,dict,tupleover legacytyping.List,typing.Dict,typing.Tuple". For Python 3.10+, you can useset[str]directly instead ofSet[str].However, the existing code in this file uses
Dict,Optional, etc., fromtyping, so this change would be consistent with the current style but not with the guideline preference.♻️ Proposed refactor to use built-in generic
-from typing import Callable, Dict, Optional, Set +from typing import Callable, Dict, OptionalThen update line 31:
-def collect_explicit_cli_keys() -> Set[str]: +def collect_explicit_cli_keys() -> set[str]:As per coding guidelines: "Prefer built-in types
list,dict,tupleover legacytyping.List,typing.Dict,typing.Tuple; use|syntax instead oftyping.Union" (applies tosetas well in Python 3.10+).🤖 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/bench/benchmark/__init__.py` at line 3, The import currently pulls Set from typing ("from typing import Callable, Dict, Optional, Set"); replace usage of typing.Set with the PEP 585 built-in generic by removing Set from the typing import and using the built-in "set" in type annotations (e.g., change any "Set[str]" to "set[str]"); update the import line to "from typing import Callable, Dict, Optional" (or prefer "dict" and "Optional" changes separately) and adjust any annotations elsewhere in this module (e.g., the annotation referenced around the existing use on/near line 31) to use the built-in set generic.
🤖 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 28-29: The import set_spawn_proxy_process_ipc_hmac_key is unused
and causes autoflake/pre-commit failures; update the import statement in
serve.py to remove that symbol so only LlmLauncherEnvs is imported from
tensorrt_llm.executor.utils (i.e., change the from ... import line to import
LlmLauncherEnvs only).
---
Nitpick comments:
In `@tensorrt_llm/bench/benchmark/__init__.py`:
- Line 3: The import currently pulls Set from typing ("from typing import
Callable, Dict, Optional, Set"); replace usage of typing.Set with the PEP 585
built-in generic by removing Set from the typing import and using the built-in
"set" in type annotations (e.g., change any "Set[str]" to "set[str]"); update
the import line to "from typing import Callable, Dict, Optional" (or prefer
"dict" and "Optional" changes separately) and adjust any annotations elsewhere
in this module (e.g., the annotation referenced around the existing use on/near
line 31) to use the built-in set generic.
In `@tensorrt_llm/bench/dataclasses/configuration.py`:
- Line 5: Replace typing.Set with the PEP 585 built-in generic set: remove Set
from the import list on the top import line and change any type annotations that
use typing.Set (the Set[...] occurrence referenced around the current line 39)
to use the built-in set[...] form; keep other typing imports (Dict, List, etc.)
unchanged to preserve file style.
🪄 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: 8da6af56-6405-4faa-966d-70336b3aa1b2
📒 Files selected for processing (12)
docs/source/commands/trtllm-serve/trtllm-serve.rstdocs/source/release-notes.mdtensorrt_llm/bench/benchmark/__init__.pytensorrt_llm/bench/benchmark/low_latency.pytensorrt_llm/bench/benchmark/throughput.pytensorrt_llm/bench/dataclasses/configuration.pytensorrt_llm/commands/eval.pytensorrt_llm/commands/serve.pytensorrt_llm/commands/utils.pytensorrt_llm/llmapi/llm_args.pytests/unittest/llmapi/apps/_test_trtllm_serve_duplicated_args.pytests/unittest/llmapi/test_llm_args.py
Ensure explicitly provided CLI flags take precedence over config YAML values across the command entry points, with shared tracking of explicit keys and focused coverage for precedence behavior. Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
67b9960 to
85f1e74
Compare
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #51386 [ run ] triggered by Bot. Commit: |
|
PR_Github #51386 [ run ] completed with state |
…m_args extra-dict Address review (Devin): in `disaggregated_mpi_worker`, the previous commits passed `explicit_cli_keys=set(server_cfg.other_args)` to BOTH `get_llm_args` AND `update_llm_args_with_extra_dict`. The first call is correct (bypasses the value-based filter for default-valued disagg-YAML fields like `tensor_parallel_size: 1`). The second was wrong: in this path `llm_args_extra_dict` is just the catch-all for kwargs that didn't match `get_llm_args`'s named signature (`quant_config`, `lora_config`, `pytorch_backend_config`, `decoding_config`, ...), not a separate YAML being overridden. Passing the explicit set tripped the merge function's "drop YAML keys claimed by explicit CLI" filter at `llm_args.py:4640-4644` and silently dropped every catch-all kwarg. This regressed disagg serving for any config that uses kwargs outside `get_llm_args`'s named params — i.e. most realistic disagg configs. Refactor: extract `_build_llm_args_from_disagg_server_cfg(other_args)` in `serve.py` and use it from both call sites in `disaggregated_mpi_worker` (the client-mode and leader-mode branches). The helper passes `explicit_cli_keys` only to `get_llm_args` and not to the merge step, with a docstring explaining why. The leader branch's `llm_args` was unused on `main` (never passed to `_launch_disaggregated_leader`); a TODO comment is added to clarify but the call is kept symmetric with the client branch for safety. Tests: add `TestDisaggLauncherKwargsPreservation` with two cases: - `test_extra_kwargs_survive` — a `quant_config` and `lora_config` from `server_cfg.other_args` must reach the final llm_args. Fails on the buggy state, passes after the fix. - `test_default_valued_named_params_survive` — pre-existing-bug regression guard for `tensor_parallel_size: 1` (the original motivation for adding `explicit_cli_keys` to `get_llm_args`). `pytest tests/unittest/llmapi/test_llm_args.py -q`: 128 passed, 3 skipped (skips are pre-existing). Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
|
/bot run |
|
PR_Github #51662 [ run ] triggered by Bot. Commit: |
|
PR_Github #51662 [ run ] completed with state
|
…L value
Address review (JunyiXu-nv): since flipping precedence to "CLI > YAML"
may break workflows that relied on YAML overriding CLI, emit a warning
when an explicit CLI flag actually overrides a value set in the YAML
config.
Warnings are added at the two override sites in
update_llm_args_with_extra_dict, each guarded so it fires only on a
genuine override (CLI value differs from the YAML value it replaces):
1. The "drop YAML keys claimed by explicit CLI flags" step -- covers
top-level YAML scalars (e.g. `tensor_parallel_size: N`,
`max_batch_size: N`). These keys are applied via the bulk dict merge,
so the drop step is the only per-key processing point for them.
2. The build_config dual-location loop -- covers the nested
`build_config:` block form (e.g. `build_config: {max_batch_size: N}`),
which the top-level drop step does not see. This is the path the
reviewer commented on. The pre-existing "set to X from explicit CLI
flag" info log is kept for the non-override case (warn on override,
info otherwise -- never both).
Identical values, and CLI flags absent from the YAML, stay silent. The
disagg launcher path does not pass explicit_cli_keys to the merge step,
so it produces no spurious warnings.
Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
|
/bot run |
|
PR_Github #51916 [ run ] triggered by Bot. Commit: |
|
PR_Github #51916 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51995 [ run ] triggered by Bot. Commit: |
|
PR_Github #51995 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52046 [ run ] triggered by Bot. Commit: |
|
PR_Github #52046 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52077 [ run ] triggered by Bot. Commit: |
|
PR_Github #52077 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52104 [ run ] triggered by Bot. Commit: |
|
PR_Github #52104 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52585 [ run ] triggered by Bot. Commit: |
|
PR_Github #52585 [ run ] completed with state |
…onfig / --extra_llm_api_options YAML (NVIDIA#14812) Signed-off-by: marinayanov <256585945+marinayanov@users.noreply.github.com>
4 participants
Summary by CodeRabbit
trtllm-serve,trtllm-eval, andtrtllm-benchcommands.Description
Title
[#10710][feat] Make explicit CLI flags take precedence over --config / --extra_llm_api_options YAML
Body
@coderabbitai summary
Description
Today,
--config/--extra_llm_api_optionsYAML values silently overrideexplicit CLI flags in
trtllm-serve,trtllm-eval, andtrtllm-bench.For example,
trtllm-serve <model> --tensor_parallel_size 4 --config foo.yamlwith
tensor_parallel_size: 8infoo.yamlruns at TP=8. This isunconventional and inconsistent with AutoDeploy's documented
CLI > YAML > defaults. See #10710.This PR flips the precedence to explicit CLI > YAML > defaults, using
click.Context.get_parameter_source()to detect which flags the usertyped. Un-set CLI flags still fall back to YAML.
Backward-compatible: programmatic callers of
update_llm_args_with_extra_dict(Triton backend, LLM API, third-party)that don't pass the new
explicit_cli_keysargument keep today'sbehavior.
Why this is the right direction, briefly:
exactly this order — see
docs/source/features/auto_deploy/advanced/expert_configurations.md:"CLI Arguments (highest priority) > YAML Configs > Default Settings (lowest priority)."
This PR brings the rest of the CLI surface in line.
and ~every argparse-based Python CLI implement
CLI > config-file > defaults.The current "YAML > CLI" is the surprise.
trtllm-serve'sis_non_default_or_required, and thereviewers explicitly flagged the remaining limitation:
"CLI params can make this a bit tricky, since you need to
distinguish between a CLI param being explicitly set vs. defaulting
to a value." This PR closes that gap with
click.Context.get_parameter_source().Test Coverage
tests/unittest/llmapi/test_llm_args.py:TestExplicitCliKeysPrecedence,TestEvalTranslationMap,TestBenchTranslationMap— cover precedence, deep-merges, build_configtiered rule, and per-tool Click-flag → LlmArgs translations.
TestTelemetryConfigPrecedenceandTestPyTorchBackendModelDefaultspass unchanged (back-compat).tests/unittest/llmapi/apps/_test_trtllm_serve_duplicated_args.py:--tp_size 1plus YAMLtensor_parallel_size: 99; serverstarting at TP=1 is the assertion that CLI wins.
pytest tests/unittest/llmapi/test_llm_args.py -qPR 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.