[None][feat] Refactor to support legacy and 1.x modelopt quant config format - #14088
Conversation
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThis PR introduces a unified ModelOpt quantization-config parser and integrates it into three quantization-handling code paths. A new ChangesModelOpt Quantization Config Parsing and Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 4
🧹 Nitpick comments (2)
tests/unittest/llmapi/test_modelopt_quant_config.py (2)
96-274: ⚡ Quick winAdd regression tests for legacy inline format through the loader call paths.
This file validates parser utilities well, but the PR also changes
ModelLoader._update_from_hf_quant_configandModelConfig.load_hf_quant_config. Please add tests that feed legacy inline ModelOpt configs (producer+quantization) through those consumers to prevent format-detection regressions.As per coding guidelines: "Coverage expectations: Assess whether new/changed tests cover happy path, important edge cases, and failure modes relevant to the feature or fix."
🤖 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_modelopt_quant_config.py` around lines 96 - 274, Add tests that exercise the legacy inline ModelOpt format by invoking ModelConfig.load_hf_quant_config and ModelLoader._update_from_hf_quant_config with legacy-style dicts (include "producer" and "quantization"/legacy fields) to ensure format detection still works; create at least one happy-path case asserting the resulting ModelOptQuantConfig fields (e.g., quant_algo, exclude_modules, quantized_layers, kv_cache_quant_algo) match expectations and one failure/edge case (unparseable inline) asserting no crash and/or proper warning via warn_if_inline_quant_config_differs; reference the functions ModelConfig.load_hf_quant_config and ModelLoader._update_from_hf_quant_config when locating code to test.
1-274: QA list update is unnecessary for this PR scope.This change set adds unit tests under
tests/unittest/only; notests/integration/defs/additions are present, sotests/integration/test_lists/qa/*updates are not required.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_modelopt_quant_config.py` around lines 1 - 274, The reviewer notes the QA list change is unnecessary because the PR only adds unit tests; update the PR description (or the commit message) to explicitly state "QA list updates are unnecessary for this PR — changes are limited to tests/unittest/* (e.g., tests/unittest/llmapi/test_modelopt_quant_config.py) and do not touch integration/defs", referencing the guideline phrase "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional." Keep the sentence concise and place it at the top of the PR description so CI/reviewers see it 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/_torch/model_config.py`:
- Around line 395-399: The branch that detects ModelOpt only checks
hf_quant_config["quant_method"], missing legacy inline ModelOpt patterns; update
the detection in the same conditional (the block that calls
parse_modelopt_quant_config and ModelConfig._build_quant_config_from_modelopt)
to also detect legacy configs by checking for hf_quant_config.get("producer",
{}).get("name") that contains "modelopt" or the presence of a top-level
"quantization" wrapper indicating legacy inline ModelOpt; if a legacy inline
ModelOpt is found, skip parse_modelopt_quant_config and return a default quant
config (same shape as used elsewhere for defaults) instead of attempting
modelopt parsing.
- Around line 324-332: The current try/except around
transformers.utils.hub.cached_file and json.load swallows all exceptions; change
it to only handle expected "file-not-found" and OS-access errors (e.g.,
FileNotFoundError, PermissionError, OSError) when calling
transformers.utils.hub.cached_file so you fall back to hf_quant_config.json, but
let JSON parsing errors (json.JSONDecodeError) and other unexpected exceptions
propagate (or log and raise) so malformed quant_cfg.json is not silently ignored
— update the block that uses cached_file, ext_file and json.load (variables
ext_file and extended) to catch specific exceptions accordingly and handle each
case separately (missing file -> fallback; parse/IO errors -> surface error).
In `@tensorrt_llm/llmapi/llm_utils.py`:
- Around line 446-454: The branch handling ModelOpt should also accept legacy
inline ModelOpt configs wrapped with a producer/quantization envelope; update
the condition so that if hf_quant_config either has a flat quant_method starting
with "modelopt" or contains a legacy wrapper (e.g., keys like "producer" or
"quantization"), extract the inner modelopt payload and pass it to
parse_modelopt_quant_config, then call self._apply_modelopt_quant_config(...)
and set self.llm_args.quant_config as currently done; reference
parse_modelopt_quant_config and _apply_modelopt_quant_config to locate and reuse
the same parsing/apply logic.
In `@tensorrt_llm/quantization/modelopt_config.py`:
- Around line 116-127: In _read_legacy, validate that raw["quantization"] is a
mapping before dereferencing: check the type of q (the local variable assigned
from raw["quantization"]) and if it's not a dict/Mapping raise a ValueError with
a clear parse message rather than letting AttributeError propagate; then proceed
to construct and return the ModelOptQuantConfig as before (using q.get(...),
q.get("quantized_layers") etc.) so callers receive a deterministic parse error
for invalid legacy payloads.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_modelopt_quant_config.py`:
- Around line 96-274: Add tests that exercise the legacy inline ModelOpt format
by invoking ModelConfig.load_hf_quant_config and
ModelLoader._update_from_hf_quant_config with legacy-style dicts (include
"producer" and "quantization"/legacy fields) to ensure format detection still
works; create at least one happy-path case asserting the resulting
ModelOptQuantConfig fields (e.g., quant_algo, exclude_modules, quantized_layers,
kv_cache_quant_algo) match expectations and one failure/edge case (unparseable
inline) asserting no crash and/or proper warning via
warn_if_inline_quant_config_differs; reference the functions
ModelConfig.load_hf_quant_config and ModelLoader._update_from_hf_quant_config
when locating code to test.
- Around line 1-274: The reviewer notes the QA list change is unnecessary
because the PR only adds unit tests; update the PR description (or the commit
message) to explicitly state "QA list updates are unnecessary for this PR —
changes are limited to tests/unittest/* (e.g.,
tests/unittest/llmapi/test_modelopt_quant_config.py) and do not touch
integration/defs", referencing the guideline phrase "If the PR only touches
unittest/ or narrow unit scope, say explicitly whether QA list updates are
unnecessary or optional." Keep the sentence concise and place it at the top of
the PR description so CI/reviewers see it 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: beb49594-5844-486a-b291-39a08cea8994
📒 Files selected for processing (5)
tensorrt_llm/_torch/auto_deploy/models/quant_config_reader.pytensorrt_llm/_torch/model_config.pytensorrt_llm/llmapi/llm_utils.pytensorrt_llm/quantization/modelopt_config.pytests/unittest/llmapi/test_modelopt_quant_config.py
|
PR_Github #48156 [ run ] triggered by Bot. Commit: |
|
PR_Github #48156 [ run ] completed with state
|
4c654fd to
9e00555
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #48325 [ run ] triggered by Bot. Commit: |
|
PR_Github #48325 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #48523 [ run ] triggered by Bot. Commit: |
|
PR_Github #48523 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #48667 [ run ] triggered by Bot. Commit: |
|
PR_Github #48667 [ run ] completed with state |
dc31228 to
1d76cf7
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49398 [ run ] triggered by Bot. Commit: |
|
PR_Github #49316 [ run ] completed with state |
1d76cf7 to
dcf74cd
Compare
… format Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
dcf74cd to
e8dc03d
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49420 [ run ] triggered by Bot. Commit: |
|
PR_Github #49398 [ run ] completed with state |
tcherckez-nvidia
left a comment
tcherckez-nvidia
left a comment
There was a problem hiding this comment.
LGTM, please run the extra AD stages
|
PR_Github #49420 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #49566 [ run ] triggered by Bot. Commit: |
|
PR_Github #49566 [ run ] completed with state
|
|
/bot skip --comment 'most tests were passed at the last commit, while the failed tests were passed before and unrelated with my changes' |
|
|
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot skip --comment "last CI passed mostly and the remaining passed in previous runs and unrelated with my changes" |
|
PR_Github #49615 [ skip ] triggered by Bot. Commit: |
|
PR_Github #49615 [ skip ] completed with state |
… format (NVIDIA#14088) Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
… format (NVIDIA#14088) Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
5 participants
Summary by CodeRabbit
New Features
Tests
Description
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)
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.