[None][fix] Fix Qwen3 w4a8 model execution failure and add unit test (re-open of #14527) - #17196
[None][fix] Fix Qwen3 w4a8 model execution failure and add unit test (re-open of #14527)#17196rosenrodt wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThe change updates ChangesQwen3 W4A8 quantization and accuracy coverage
Estimated code review effort: 3 (Moderate) | ~20 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.
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (1)
4772-4773: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new test method.
Annotate
tp_size,pp_size, andep_sizeasint. Annotateattention_dp,cuda_graph, andoverlap_schedulerasbool. Add-> None.Proposed change
- def test_w4a8(self, tp_size, pp_size, ep_size, attention_dp, cuda_graph, - overlap_scheduler): + def test_w4a8( + self, + tp_size: int, + pp_size: int, + ep_size: int, + attention_dp: bool, + cuda_graph: bool, + overlap_scheduler: bool, + ) -> None:As per coding guidelines, “Annotate every function, use
Nonefor non-returning functions.”🤖 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/integration/defs/accuracy/test_llm_api_pytorch.py` around lines 4772 - 4773, Update the test_w4a8 method signature to annotate tp_size, pp_size, and ep_size as int, attention_dp, cuda_graph, and overlap_scheduler as bool, and its return type as None.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.
Nitpick comments:
In `@tests/integration/defs/accuracy/test_llm_api_pytorch.py`:
- Around line 4772-4773: Update the test_w4a8 method signature to annotate
tp_size, pp_size, and ep_size as int, attention_dp, cuda_graph, and
overlap_scheduler as bool, and its return type as None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d54d4a9-bff6-4607-b802-d0a6888d4879
📒 Files selected for processing (5)
tensorrt_llm/_torch/modules/fused_moe/configurable_moe.pytests/integration/defs/accuracy/references/mmlu.yamltests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_dgx_h100.yml
|
Reviewed the whole change. The diagnosis is right — 1. It can clobber a deliberate 2. The eager path desyncs instead of syncing. When Both point at the same thing: two mechanisms now decide the backend's quant config and the winner depends on call order. Worth considering making the wrapper's 3. Competing implementation. 4. Title says "add unit test", but the added coverage is a 2-GPU H100 accuracy run needing No blossom run on |
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/unittest/_torch/modules/fused_moe/test_configurable_moe.py`:
- Around line 70-87: Add two focused tests alongside
test_layerwise_quant_config_is_applied_before_weight_creation: cover the
non-empty exclude_modules branch in the relevant backend allocation flow, and
verify that an explicit _override_quant_config remains authoritative over other
quantization configuration sources. Keep each case isolated and assert the
resulting quant_config and weight-creation behavior, including the expected
allocation/deferment outcome for exclusions.
🪄 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: 5d8bfb29-3378-4f39-9603-6f3d6f526164
📒 Files selected for processing (3)
tensorrt_llm/_torch/modules/fused_moe/configurable_moe.pytests/integration/defs/accuracy/test_llm_api_pytorch.pytests/unittest/_torch/modules/fused_moe/test_configurable_moe.py
| def test_layerwise_quant_config_is_applied_before_weight_creation() -> None: | ||
| global_config = QuantConfig() | ||
| layer_config = QuantConfig() | ||
| model_config = ModelConfig( | ||
| quant_config=global_config, | ||
| quant_config_dict={"model.layers.0.mlp.experts": layer_config}, | ||
| ) | ||
| wrapper = _wrapper() | ||
| wrapper.quant_config = global_config | ||
|
|
||
| backend = _create_backend(wrapper, model_config) | ||
|
|
||
| backend.create_weights.assert_not_called() | ||
| wrapper.quant_config = layer_config | ||
| wrapper.create_weights() | ||
|
|
||
| assert backend.quant_config is layer_config | ||
| backend.create_weights.assert_called_once_with() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for the other quantization lifecycle branches.
This test covers quant_config_dict deferral. It does not cover a non-empty exclude_modules value at Line 354. It also does not verify that _override_quant_config remains authoritative at Lines 664-668.
Add one focused case for exclusions and one for explicit override precedence.
Test coverage summary: insufficient. The added unit test covers layerwise configuration propagation. It does not cover all changed allocation branches. Integration test-list registration does not apply to this unit-test module.
As per path instructions, changed test code requires a coverage verdict.
🤖 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/_torch/modules/fused_moe/test_configurable_moe.py` around
lines 70 - 87, Add two focused tests alongside
test_layerwise_quant_config_is_applied_before_weight_creation: cover the
non-empty exclude_modules branch in the relevant backend allocation flow, and
verify that an explicit _override_quant_config remains authoritative over other
quantization configuration sources. Keep each case isolated and assert the
resulting quant_config and weight-creation behavior, including the expected
allocation/deferment outcome for exclusions.
Source: Path instructions
|
PR_Github #63426 [ run ] triggered by Bot. Commit: |
|
PR_Github #63426 [ run ] completed with state
|
|
Re-reviewed
One new thing, and it is the reason I'm not approving yet: The deferral is much broader than the comment says. The comment reads as if only unusual models defer and "directly-created MoE modules ... retain their historical eager allocation behavior". But Inside a
Also still open from last time: Finally, there's no blossom run on this head yet — only the lightweight checks — so nothing has exercised any of the above. Note |
|
/bot run |
|
PR_Github #63462 [ run ] triggered by Bot. Commit: |
|
PR_Github #63462 [ run ] completed with state
|
Signed-off-by: ylichen <ylichen@nvidia.com>
Signed-off-by: ylichen <ylichen@nvidia.com>
- Add test_w4a8[tp2_ep1] to llm_function_core.txt QA test list - Add test to l0_dgx_h100.yml 2-GPU pre_merge section - Add W4A8_AWQ accuracy reference (79.53) to mmlu.yaml Signed-off-by: ylichen <ylichen@nvidia.com>
Register the Qwen3 W4A8 checkpoint using its global mixed-precision quantization metadata so accuracy reference lookup matches the runtime configuration. Changes - replace the W4A8_AWQ-only MMLU key with MIXED_PRECISION - include the checkpoint's FP8 KV-cache quantization key Validation - git diff --check - parse mmlu.yaml and assert the exact reference entry Result - the reported accuracy lookup resolves MIXED_PRECISION plus FP8 to the 79.53 reference Signed-off-by: Anthony Chang <27950904+rosenrodt@users.noreply.github.com>
|
/bot run |
|
/bot kill |
|
PR_Github #63593 [ run ] triggered by Bot. Commit: |
|
PR_Github #63595 [ kill ] triggered by Bot. Commit: |
|
PR_Github #63593 [ run ] completed with state |
|
PR_Github #63595 [ kill ] completed with state |
Defer ConfigurableMoE weight creation until layerwise quantization has selected the final configuration while preserving eager allocation for unrelated module exclusions. Preserve explicit backend overrides as authoritative. Changes - defer backend weight allocation only for post-init layerwise quantization - delegate weight-state resets so matching exclusions recreate backend weights - preserve eager allocation for unrelated FP8-block-scale exclusions - add focused lifecycle tests and annotate the W4A8 acceptance test Validation - pre-commit run --files tensorrt_llm/_torch/models/modeling_utils.py tensorrt_llm/_torch/modules/fused_moe/configurable_moe.py tests/unittest/_torch/modules/fused_moe/test_configurable_moe.py - python -m pytest -q tests/unittest/_torch/modules/fused_moe/test_configurable_moe.py - LLM_MODELS_ROOT=/home/scratch.trt_llm_data/llm-models python -m pytest -q -s tests/integration/defs/accuracy/test_llm_api_pytorch.py -k 'TestQwen3_30B_A3B and test_w4a8' Result - focused layerwise and exclusion lifecycle suite passes 2 tests - prior 2xH100 W4A8 acceptance produced MMLU accuracy 80.263 above the 77.713 threshold Signed-off-by: Anthony Chang <27950904+rosenrodt@users.noreply.github.com>
223b76e to
342b7a0
Compare
|
/bot run |
|
PR_Github #63624 [ run ] triggered by Bot. Commit: |
|
PR_Github #63624 [ run ] completed with state
|
|
Re-reviewed The deferral guard is now just One thing worth a line in the comment, because it is load-bearing: dropping the Still open from the first pass, not blocking: the per-model workaround at Not approving on this head yet, and the reason is CI, not the code: build 51579 came back FAILURE with 77 passed / 0 failed and every GPU test stage |
|
/bot run |
|
PR_Github #63655 [ run ] triggered by Bot. Commit: |
|
PR_Github #63655 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63711 [ run ] triggered by Bot. Commit: |
|
PR_Github #63711 [ run ] completed with state
|
4 participants
Dev Engineer Review
ConfigurableMoEdefers backend weight creation until layer-wise quantization is applied._weights_createdand recreate weights with the updated configuration.QA Engineer Review
test_layerwise_quant_config_is_applied_before_weight_creation.test_exclusions_only_recreate_matching_moe_weights.TestQwen3_30B_A3B.test_w4a8.tests/integration/test_lists/qa/llm_function_core.txt.tests/integration/test_lists/test-db/l0_dgx_h100.yml.Description
Re-open of #14527
Models utilizing w4a8 + fp8 kvcache failed to correctly retrieve quant_config. As a result, the system failed to trigger and use the correct load weight method during model initialization or weight loading.
Test Coverage
pytest tests/integration/defs/accuracy/test_llm_api_pytorch.py::TestQwen3_30B_A3B::test_w4a8[tp2_ep1]
pytest tests/unittest/_torch/modules/fused_moe/test_configurable_moe.py
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.