[https://nvbugs/6189416][fix] Add a Blackwell-specific reference entry (extra_acc_spec=sm100_fp8, accuracy=46. - #14484
Conversation
📝 WalkthroughWalkthroughUpdates MMMU accuracy reference and test configuration to support FP8 inference on Blackwell hardware (SM100+). Adds a lower FP8 baseline (46.0), detects Blackwell at runtime, and re-enables the previously-waived test. ChangesGemma-3-27B FP8 Blackwell Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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)
317-327: QA list impact looks unchanged for this routing update.This modifies selection logic for an existing integration test node, not a new test definition, so QA list additions appear unnecessary for this cohort; just keep node-id signatures consistent where already referenced.
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/."
🤖 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 317 - 327, This change only reroutes accuracy reference via extra_acc_spec using get_sm_version and does not add a new integration test definition, so do not add entries to the QA lists; instead ensure existing node-id signatures referenced elsewhere remain unchanged — verify the test invocation (the MMMU(...) instance and the call to task.evaluate with extra_acc_spec, sampling_params, and the _make_llm context using MODEL_PATH/MODEL_NAME) preserves the existing node id and signature strings used by tests/integration/test_lists/qa/* and update any string constants if they were inadvertently modified so downstream references still match.
🤖 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 317-327: This change only reroutes accuracy reference via
extra_acc_spec using get_sm_version and does not add a new integration test
definition, so do not add entries to the QA lists; instead ensure existing
node-id signatures referenced elsewhere remain unchanged — verify the test
invocation (the MMMU(...) instance and the call to task.evaluate with
extra_acc_spec, sampling_params, and the _make_llm context using
MODEL_PATH/MODEL_NAME) preserves the existing node id and signature strings used
by tests/integration/test_lists/qa/* and update any string constants if they
were inadvertently modified so downstream references still match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8fb91d5-81f6-4166-8487-67f2780bdf0e
📒 Files selected for processing (3)
tests/integration/defs/accuracy/references/mmmu.yamltests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
c407beb to
dd9f966
Compare
dd9f966 to
1ee9fb7
Compare
|
/bot run |
|
PR_Github #50460 [ run ] triggered by Bot. Commit: |
|
PR_Github #50460 [ run ] completed with state
|
|
/bot run |
2 similar comments
|
/bot run |
|
/bot run |
|
PR_Github #50598 [ run ] triggered by Bot. Commit: |
|
PR_Github #50598 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #50612 [ run ] triggered by Bot. Commit: |
|
PR_Github #50612 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #50719 [ run ] triggered by Bot. Commit: |
|
PR_Github #50721 [ run ] triggered by Bot. Commit: |
|
PR_Github #50719 [ run ] completed with state |
|
PR_Github #50721 [ run ] completed with state
|
|
@tensorrt-cicd can you revert your original changes to tests/integration/test_lists/waives.txt and apply them again on top of the latest tests/integration/test_lists/waives.txt from main? |
… Gemma3-27B multimodal The Hopper-tuned MMMU reference (50.0) for FP8 Gemma3-27B is unreachable on Blackwell because the FlashInfer FP8 attention path emits cubins with different numerics (~5pt drop on MMMU). Rather than relax the Hopper reference (which would mask future Hopper regressions) or skip the test on Blackwell (which leaves no FP8 multimodal coverage on the new arch), add a separate sm100_fp8 reference at 46.0 and route the test to it via extra_acc_spec when running on SM>=100. The waiver pointing at this NVBug is removed. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
Signed-off-by: Michal Guzek <moraxu@users.noreply.github.com>
Reverted the prior waives.txt edits and re-applied only the intended change (removal of the nvbugs/6189416 waiver line) on top of the latest origin/main version of the file, per reviewer request. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
2c71dfd to
64690df
Compare
|
/bot run |
|
PR_Github #50923 [ run ] triggered by Bot. Commit: |
|
PR_Github #50923 [ run ] completed with state |
Summary
Test plan
Links
Summary by CodeRabbit