[https://nvbugs/6379636][fix] Fix Gemma4 MoE weight loading - #16108
Conversation
📝 WalkthroughWalkthroughAdds NVFP4 per-expert MoE checkpoint key remapping to Gemma4HfWeightMapper with validation for unsupported expert keys, corresponding unit tests, a new B200-only NVFP4 accuracy integration test for Gemma4-26B-A4B, an updated MMMU accuracy reference, and a B200 test list entry. ChangesGemma4 NVFP4 MoE remapping
Gemma4 NVFP4 B200 accuracy test
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.py (1)
404-406: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable dict class attribute (RUF012).
EXTRA_EVALUATOR_KWARGSis a mutable dict literal assigned directly as a class attribute; static analysis flags this as RUF012. Low risk here since it's read-only, but consider annotating asClassVar[dict]to make the immutability intent explicit and silence the lint warning.🤖 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_multimodal.py` around lines 404 - 406, `EXTRA_EVALUATOR_KWARGS` is a mutable dict class attribute that triggers RUF012; update the test class in `test_llm_api_pytorch_multimodal` to make the intent explicit by annotating this attribute as a `ClassVar[dict]` (or equivalent typed class-level constant) so the linter recognizes it as intentionally shared and read-only.Source: Linters/SAST tools
🤖 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_multimodal.py`:
- Around line 404-406: `EXTRA_EVALUATOR_KWARGS` is a mutable dict class
attribute that triggers RUF012; update the test class in
`test_llm_api_pytorch_multimodal` to make the intent explicit by annotating this
attribute as a `ClassVar[dict]` (or equivalent typed class-level constant) so
the linter recognizes it as intentionally shared and read-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ac85bc0-ef63-4fbf-8084-1f07799d0709
📒 Files selected for processing (5)
tensorrt_llm/_torch/models/checkpoints/hf/gemma4_weight_mapper.pytests/integration/defs/accuracy/references/mmmu.yamltests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.pytests/integration/test_lists/test-db/l0_b200.ymltests/unittest/_torch/modeling/test_modeling_gemma4.py
lfr-0531
left a comment
There was a problem hiding this comment.
Added a minor comment, but LGTM overall.
|
/bot run |
|
PR_Github #58269 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #58298 [ run ] triggered by Bot. Commit: |
|
PR_Github #58269 [ run ] completed with state |
|
PR_Github #58298 [ run ] completed with state
|
|
/bot run |
|
PR_Github #58372 [ run ] triggered by Bot. Commit: |
|
PR_Github #58372 [ run ] completed with state
|
0c6435b to
908f825
Compare
|
/bot run |
|
PR_Github #58487 [ run ] triggered by Bot. Commit: |
|
PR_Github #58487 [ run ] completed with state
|
|
/bot run |
|
PR_Github #58508 [ run ] triggered by Bot. Commit: |
908f825 to
c284593
Compare
|
/bot run |
|
PR_Github #58529 [ run ] triggered by Bot. Commit: |
|
PR_Github #58508 [ run ] completed with state
|
This commit fixes some issues in MoE weight loading for Gemma4, and adds an E2E accuracy test for it. Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
c284593 to
bab1853
Compare
|
/bot run |
|
PR_Github #58583 [ run ] triggered by Bot. Commit: |
|
PR_Github #58529 [ run ] completed with state
|
|
/bot run |
|
PR_Github #58615 [ run ] triggered by Bot. Commit: |
|
PR_Github #58583 [ run ] completed with state
|
|
PR_Github #58615 [ run ] completed with state |
|
@2ez4bz Heads-up: Cause: Observed identically in at least four unrelated PR runs (builds #2327/#2328/#2329/#2330 of 🤖 Generated with Claude Code |
…nce) accuracy/test_llm_api_autodeploy.py::TestGemma4MoE::test_bf16 evaluates MMMU with the plain bf16 spec, but references/mmmu.yaml only registers a NVFP4/FP8-KV entry for google/gemma-4-26B-A4B-it (added in NVIDIA#16108), so the accuracy harness raises ValueError (Not registered specs) before scoring. The failure is deterministic and PR-independent: it failed identically in every recent LLM/main/L0_Test-x86_64-Multi-GPU run (builds 2327-2336), blocking pre-merge CI for any PR that triggers the multi-GPU stage. Waive until the bf16 MMMU reference is added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
…nce) accuracy/test_llm_api_autodeploy.py::TestGemma4MoE::test_bf16 evaluates MMMU with the plain bf16 spec, but references/mmmu.yaml only registers a NVFP4/FP8-KV entry for google/gemma-4-26B-A4B-it (added in NVIDIA#16108), so the accuracy harness raises ValueError (Not registered specs) before scoring. The failure is deterministic and PR-independent: it failed identically in every recent LLM/main/L0_Test-x86_64-Multi-GPU run (builds 2327-2336), blocking pre-merge CI for any PR that triggers the multi-GPU stage. Waive until the bf16 MMMU reference is added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
…26B-A4B-it PR NVIDIA#16108 replaced the plain unquantized entry for google/gemma-4-26B-A4B-it with only an NVFP4 entry, which broke TestGemma4MoE::test_bf16: the autodeploy bf16 test looks up the model with no quant (defaults: dtype='auto', quant_algo=None, kv_cache_quant_algo=None, ...) and no reference row matched, raising ValueError: Not registered specs. Add the pre-NVIDIA#16108 bf16 baseline (56.667) back alongside the NVFP4 entry so both TestGemma4MoE::test_bf16 (H100 autodeploy, bf16) and the new PyTorch NVFP4 test resolve their thresholds. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
…racy: 56.667` alongside the existing… (#16310) Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Description
This commit fixes some issues in MoE weight loading for Gemma4, and adds an E2E accuracy test for it.
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)
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.