[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 |
|
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 (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 |
xinhe-nv
changed the title
|
/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 |
2ez4bz
changed the title
* 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>
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 |
|
PR_Github #63848 [ run ] triggered by Bot. Commit: |
|
PR_Github #63848 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63878 [ run ] triggered by Bot. Commit: |
|
PR_Github #63878 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63886 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Approach looks right, and switching _Gemma4EncoderCacheHarness to subclass Gemma4MultimodalModelBase is a real improvement — the tests now go through the actual encode_multimodal_inputs. Two things below on the fallback path; neither blocks.
On the unwaive: TestGemma4_26B_A4B::test_nvfp4 is in test-db/l0_b200.yml and encoder_cache_max_bytes defaults to 128 MiB, so the B200 stage does exercise this. Please link a green run of that stage before merging.
Unrelated but adjacent: the encoder_cache_max_bytes description in llm_args.py:569 still says "reuse is all-or-nothing for each request: every item in the request must hit the cache", which stopped being true when partial hits landed. Worth a one-line fix while you're in this area.
I could not run the unit tests locally (no torch in this checkout), so my read is static only.
|
PR_Github #63886 [ run ] completed with state |
Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
9e3a01f to
d3b295b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
6 participants
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 for the changed test functions.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.