[None][fix] Fix Qwen3 w4a8 model execution failure and add unit test - #14527
[None][fix] Fix Qwen3 w4a8 model execution failure and add unit test#14527leo0519 wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds backend quantization config synchronization to ensure per-layer quantization settings propagate correctly during weight creation, and introduces an accuracy test validating W4A8 quantization for Qwen3 30B. ChangesW4A8 Quantization Support
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
🚥 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: 1
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (1)
4913-4935: ⚡ Quick winAdd this new accuracy test to QA scheduling lists (if not already done in another file).
Since this adds an integration accuracy test under
tests/integration/defs/, ensure there is a matching entry in QA functional lists (and exact test target formatting), otherwise scheduled QA runs may not execute it.As per coding guidelines, “If the change adds or materially alters an integration test under tests/integration/defs/, call out whether an entry is needed under tests/integration/test_lists/qa/,” and the referenced QA docs require exact pytest target matching.
🤖 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 4913 - 4935, You added a new integration accuracy test function test_w4a8 (which constructs LLM and runs MMLU.evaluate) under the integration defs; update the QA scheduling lists so this pytest target is executed by CI: add the exact pytest target string for this test to the appropriate QA list file under tests/integration/test_lists/qa (or the project’s equivalent QA scheduling list), following the exact formatting used by other entries so the test runner discovers it (use the full pytest target for the test function/test module and ensure any TP/PP/etc. variants are covered if required).
🤖 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/integration/defs/accuracy/test_llm_api_pytorch.py`:
- Around line 4919-4931: The test test_w4a8 currently only exercises W4A8
quantization; update the LLM instantiation in test_w4a8 to enable an FP8 KV
cache by passing kv_cache_config=KvCacheConfig(dtype="fp8", ...) to LLM so the
test covers the W4A8 + FP8 KV cache regression; ensure you construct
KvCacheConfig with the same required FP8 parameters used elsewhere in tests (or
import the shared FP8 config), keep the existing model path and other args, and
add any necessary import for KvCacheConfig so the test actually exercises the
faulty combination.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/test_llm_api_pytorch.py`:
- Around line 4913-4935: You added a new integration accuracy test function
test_w4a8 (which constructs LLM and runs MMLU.evaluate) under the integration
defs; update the QA scheduling lists so this pytest target is executed by CI:
add the exact pytest target string for this test to the appropriate QA list file
under tests/integration/test_lists/qa (or the project’s equivalent QA scheduling
list), following the exact formatting used by other entries so the test runner
discovers it (use the full pytest target for the test function/test module and
ensure any TP/PP/etc. variants are covered if required).
🪄 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: e69a564d-eef0-402f-b69b-6fcdcc73c67e
📒 Files selected for processing (2)
tensorrt_llm/_torch/modules/fused_moe/configurable_moe.pytests/integration/defs/accuracy/test_llm_api_pytorch.py
| def test_w4a8(self, tp_size, pp_size, ep_size, attention_dp, cuda_graph, | ||
| overlap_scheduler): | ||
| pytorch_config = dict( | ||
| disable_overlap_scheduler=not overlap_scheduler, | ||
| cuda_graph_config=CudaGraphConfig() if cuda_graph else None) | ||
|
|
||
| llm = LLM( | ||
| f"{llm_models_root()}/Qwen3/saved_models_Qwen3-30B-A3B_w4a8_hf", | ||
| tensor_parallel_size=tp_size, | ||
| pipeline_parallel_size=pp_size, | ||
| moe_expert_parallel_size=ep_size, | ||
| **pytorch_config, | ||
| enable_attention_dp=attention_dp) |
There was a problem hiding this comment.
Cover the actual regression trigger (w4a8 + fp8 kv cache) in this test.
This test currently runs W4A8 only, but the PR fix is specifically about W4A8 combined with FP8 KV cache. Add kv_cache_config=KvCacheConfig(dtype="fp8", ...) (and keep this test path) so it actually guards the reported failure mode.
As per coding guidelines, QA/test coverage should include relevant failure modes for the feature/fix, and this PR objective explicitly targets W4A8 + FP8 KV cache behavior.
🤖 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 4919 -
4931, The test test_w4a8 currently only exercises W4A8 quantization; update the
LLM instantiation in test_w4a8 to enable an FP8 KV cache by passing
kv_cache_config=KvCacheConfig(dtype="fp8", ...) to LLM so the test covers the
W4A8 + FP8 KV cache regression; ensure you construct KvCacheConfig with the same
required FP8 parameters used elsewhere in tests (or import the shared FP8
config), keep the existing model path and other args, and add any necessary
import for KvCacheConfig so the test actually exercises the faulty combination.
|
/bot run |
|
/bot run |
|
/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 run |
1 similar comment
|
/bot run |
|
/bot run --add-multi-gpu-test |
|
/bot run |
3 similar comments
|
/bot run |
|
/bot run |
|
/bot run |
|
PR_Github #56343 [ run ] triggered by Bot. Commit: |
|
PR_Github #56346 [ run ] triggered by Bot. Commit: |
|
PR_Github #56343 [ run ] completed with state |
|
PR_Github #56346 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #58847 [ run ] triggered by Bot. Commit: |
|
PR_Github #58847 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60212 [ run ] triggered by Bot. Commit: |
|
PR_Github #60212 [ run ] completed with state
|
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
LGTM — the create_weights change (propagate quant_config to .backend) is a no-op for uniform-quant MoE (same global config) and only corrects the per-layer/mixed-precision path where the .backend-suffixed module name couldn't be matched — i.e. it fixes the already-broken Qwen3 w4a8 path. Default MoE unchanged. 2 approvals.
|
/bot run |
|
PR_Github #61872 [ run ] triggered by Bot. Commit: |
|
PR_Github #61872 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61913 [ run ] triggered by Bot. Commit: |
|
PR_Github #61913 [ run ] completed with state
|
|
/bot run |
|
/bot run |
|
PR_Github #62055 [ run ] triggered by Bot. Commit: |
|
PR_Github #62055 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62078 [ run ] triggered by Bot. Commit: |
|
PR_Github #62078 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62149 [ run ] triggered by Bot. Commit: |
ZhanruiSunCh
left a comment
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part.
|
PR_Github #62149 [ run ] completed with state
|
10 participants
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.
Summary by CodeRabbit
Bug Fixes
Tests