[https://nvbugs/6550127][fix] Support Gemma4 multimodal cache partial hits - #17231
[https://nvbugs/6550127][fix] Support Gemma4 multimodal cache partial hits#172312ez4bz wants to merge 2 commits into
Conversation
|
/bot run |
WalkthroughGemma4 now applies modality-specific encoder-cache handling. Image and audio partial hits use sliced inputs with aligned metadata. Partial video hits trigger full re-encoding with a one-time warning. Tests cover these paths, and the related waiver is removed. ChangesGemma4 multimodal cache handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Request
participant Gemma4MultimodalModelBase
participant TensorLRUCache
participant MultimodalEncoder
Request->>Gemma4MultimodalModelBase: submit multimodal items
Gemma4MultimodalModelBase->>TensorLRUCache: partition cache keys
TensorLRUCache-->>Gemma4MultimodalModelBase: return hits and misses
Gemma4MultimodalModelBase->>MultimodalEncoder: encode sliced image or audio inputs
MultimodalEncoder-->>Gemma4MultimodalModelBase: return embeddings
Gemma4MultimodalModelBase-->>Request: return ordered embeddings
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/models/modeling_gemma4mm.py (1)
599-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the override contract.
Add Google-style
ArgsandReturnssections forparam,item_indices, and the residualMultimodalParams. This method is a public override point for the encoder-cache flow.As per coding guidelines, “Prefer docstrings for external interfaces, use Google-style docstrings, document public function arguments.”
🤖 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 `@tensorrt_llm/_torch/models/modeling_gemma4mm.py` around lines 599 - 612, Update the docstring for build_multimodal_encoder_input to add Google-style Args entries describing param and item_indices, plus a Returns entry describing the residual MultimodalParams produced for the selected items. Keep the existing behavior description and document this public encoder-cache override contract without changing implementation logic.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 `@tensorrt_llm/_torch/models/modeling_gemma4mm.py`:
- Around line 599-612: Update the docstring for build_multimodal_encoder_input
to add Google-style Args entries describing param and item_indices, plus a
Returns entry describing the residual MultimodalParams produced for the selected
items. Keep the existing behavior description and document this public
encoder-cache override contract without changing implementation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 060df0c7-4077-4117-9301-cacdb3d0864a
📒 Files selected for processing (3)
tensorrt_llm/_torch/models/modeling_gemma4mm.pytests/integration/test_lists/waives.txttests/unittest/_torch/modeling/test_gemma4_multimodal.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
PR_Github #63584 [ run ] triggered by Bot. Commit: |
|
PR_Github #63584 [ run ] completed with state
|
|
Two things before this lands. The video decision is wider than the comment says. The comment reads "retain the pre-cache behavior", but on Title is failing CI. The rest looks right to me. The residual |
|
/bot run --only-qa-verify test accuracy/test_llm_api_pytorch_multimodal.py::TestGemma4_26B_A4B::test_nvfp4 |
|
PR_Github #63615 [ run ] triggered by Bot. Commit: |
|
PR_Github #63615 [ run ] completed with state |
* Why? The generic multimodal cache path cannot slice Gemma4 image and audio layouts, causing partial encoder-cache hits to fail during input construction. * What? Override partial-hit input construction for Gemma4 images and audio, keeping their per-item metadata aligned. Bypass persistent caching for videos with a warning until frame-level slicing is supported, and remove the obsolete accuracy waiver. Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
ceb71dc to
9e3a01f
Compare
|
/bot run |
|
@BowenFu addressed, please take another look. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/models/modeling_gemma4mm.py (1)
828-838: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the unrelated draft-model changes.
The declared encoder-cache objective does not establish a dependency on draft-model delegation or speculative/resource-manager forwarding. Move these changes to a dedicated PR unless a direct dependency on encoder-cache behavior is documented.
tensorrt_llm/_torch/models/modeling_gemma4mm.py#L828-L838: Move the draft configuration, draft model, and draft-weight delegation.tensorrt_llm/_torch/models/modeling_gemma4mm.py#L850-L881: Move the speculative-decoding, resource-manager, and original-input forwarding.As per coding guidelines: “Keep each pull request focused on one concern and avoid unrelated scope; split unrelated changes into separate PRs.”
🤖 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 `@tensorrt_llm/_torch/models/modeling_gemma4mm.py` around lines 828 - 838, Remove the unrelated draft-model delegation from modeling_gemma4mm.py lines 828-838, including draft_config, draft_model, and load_draft_weights. Also remove the speculative-decoding, resource-manager, and original-input forwarding changes from modeling_gemma4mm.py lines 850-881; retain only encoder-cache-related changes in this PR and move these features to a separate PR unless a direct dependency is documented.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.
Outside diff comments:
In `@tensorrt_llm/_torch/models/modeling_gemma4mm.py`:
- Around line 828-838: Remove the unrelated draft-model delegation from
modeling_gemma4mm.py lines 828-838, including draft_config, draft_model, and
load_draft_weights. Also remove the speculative-decoding, resource-manager, and
original-input forwarding changes from modeling_gemma4mm.py lines 850-881;
retain only encoder-cache-related changes in this PR and move these features to
a separate PR unless a direct dependency is documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 009402b2-e5e6-440f-b569-f81d5abc7de6
📒 Files selected for processing (3)
tensorrt_llm/_torch/models/modeling_gemma4mm.pytests/integration/test_lists/waives.txttests/unittest/_torch/modeling/test_gemma4_multimodal.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unittest/_torch/modeling/test_gemma4_multimodal.py
|
PR_Github #63802 [ run ] triggered by Bot. Commit: |
|
PR_Github #63802 [ run ] completed with state
|
|
/bot run |
Dev Engineer Review
QA Engineer Review
Gemma4MultimodalModelBasecache tests.TestGemma4_26B_A4B::test_nvfp4is covered by the removed waiver entry.test-db/orqa/entries are reported.Description
The generic multimodal cache path cannot slice Gemma4 image and audio layouts, causing partial encoder-cache hits to fail during input construction.
Override partial-hit input construction for Gemma4 images and audio, keeping their per-item metadata aligned. Bypass persistent caching for videos with a warning until frame-level slicing is supported, and remove the obsolete accuracy waiver.
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.