[#13697][feat] Add model YAML defaults loader#13898
Open
guan404ming wants to merge 1 commit intoNVIDIA:mainfrom
Open
[#13697][feat] Add model YAML defaults loader#13898guan404ming wants to merge 1 commit intoNVIDIA:mainfrom
guan404ming wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
svc-trtllm-gh-bot
added
the
Community want to contribute
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new YAML-based model configuration loader that computes merged defaults from architecture-level YAML files and optional model-code-provided defaults. The loader is integrated into model initialization and includes comprehensive test coverage for validation, precedence, and error cases. ChangesModel Configuration Defaults Loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_model_config_loader.py (1)
33-162: QA list updates are unnecessary for this PR scope.This PR adds/updates unit tests only; no integration test-list entry under
tests/integration/test_lists/qa/is needed.As per coding guidelines: “If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional.”
🤖 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_model_config_loader.py` around lines 33 - 162, Update the PR metadata to explicitly state QA list updates are unnecessary for this change: add a short note to the PR description (or commit message) saying this PR only modifies unit tests (e.g. references test_returns_empty_when_no_files / test_merge_model_defaults_right_wins_and_skips_empty in tests/unittest/llmapi/test_model_config_loader.py) and therefore no integration QA list entry under tests/integration/test_lists/qa/ is required. Keep the note concise and place it in the PR description near the changelog or QA checklist so reviewers see the justification immediately.
🤖 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/model_config_loader.py`:
- Around line 55-71: Sanitize the incoming arch string before using it to
construct file paths: in the code that checks if not arch and then iterates
search_dirs, validate/normalize arch (e.g., ensure it matches a safe pattern
like r'^[A-Za-z0-9_.-]+$' or use Path(arch).name and reject if arch contains
path separators or '..'); if invalid, return {} or raise ValueError. Then use
the sanitized name (e.g., sanitized_arch) when calling _load_yaml_if_exists for
f"{sanitized_arch}.yaml" and f"{sanitized_arch}_ad_default.yaml" so
_load_yaml_if_exists and _deep_merge only operate on safe filenames.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_model_config_loader.py`:
- Around line 33-162: Update the PR metadata to explicitly state QA list updates
are unnecessary for this change: add a short note to the PR description (or
commit message) saying this PR only modifies unit tests (e.g. references
test_returns_empty_when_no_files /
test_merge_model_defaults_right_wins_and_skips_empty in
tests/unittest/llmapi/test_model_config_loader.py) and therefore no integration
QA list entry under tests/integration/test_lists/qa/ is required. Keep the note
concise and place it in the PR description near the changelog or QA checklist so
reviewers see the justification immediately.
🪄 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: 4f5be979-12a5-4eb9-a71a-49786e665ea0
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/model_loader.pytensorrt_llm/llmapi/model_config_loader.pytests/unittest/llmapi/test_model_config_loader.py
Comment on lines
+55
to
+71
| if not arch: | ||
| return {} | ||
|
|
||
| if search_dirs is None: | ||
| search_dirs = (DEFAULT_MODEL_CONFIGS_DIR,) | ||
|
|
||
| merged: Dict[str, Any] = {} | ||
| for directory in search_dirs: | ||
| directory = Path(directory) | ||
| if include_ad_default: | ||
| ad_layer = _load_yaml_if_exists(directory / f"{arch}_ad_default.yaml") | ||
| if ad_layer: | ||
| merged = _deep_merge(merged, ad_layer) | ||
|
|
||
| model_layer = _load_yaml_if_exists(directory / f"{arch}.yaml") | ||
| if model_layer: | ||
| merged = _deep_merge(merged, model_layer) |
Contributor
There was a problem hiding this comment.
Sanitize arch before building YAML file paths to prevent path traversal.
arch is used directly in f"{arch}.yaml" / f"{arch}_ad_default.yaml". A malicious value containing path separators or .. can make the loader read files outside intended config directories.
🔒 Proposed hardening patch
+import re
from pathlib import Path
from typing import Any, Dict, Optional, Sequence
@@
DEFAULT_MODEL_CONFIGS_DIR: Path = Path(__file__).resolve().parent.parent / "model_configs"
+_ARCH_NAME_PATTERN = re.compile(r"^[A-Za-z0-9_]+$")
@@
def load_model_defaults(
@@
- if not arch:
+ if not arch:
return {}
+ if not _ARCH_NAME_PATTERN.fullmatch(arch):
+ raise ValueError(f"Invalid architecture name for model defaults: {arch!r}")🤖 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/model_config_loader.py` around lines 55 - 71, Sanitize
the incoming arch string before using it to construct file paths: in the code
that checks if not arch and then iterates search_dirs, validate/normalize arch
(e.g., ensure it matches a safe pattern like r'^[A-Za-z0-9_.-]+$' or use
Path(arch).name and reject if arch contains path separators or '..'); if
invalid, return {} or raise ValueError. Then use the sanitized name (e.g.,
sanitized_arch) when calling _load_yaml_if_exists for f"{sanitized_arch}.yaml"
and f"{sanitized_arch}_ad_default.yaml" so _load_yaml_if_exists and _deep_merge
only operate on safe filenames.
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
2 participants
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.
Summary by CodeRabbit
Release Notes
New Features
Tests
Description
related to #13697
I add a shared loader for per-model default YAML configs.
<model>.yamldefaults for PyTorch flow and adds the merge behavior needed for a follow-up AutoDeploy PR to load<model>_ad_default.yamlbefore<model>.yaml.architectures[0]name as the model key.Test Coverage
Added unit tests for:
<model>.yamlloading<model>_ad_default.yamlloadingPR 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)
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.