test(e2e): don't assert exact image count for duplicate images - #1821
Conversation
github-actions
Bot
added
grpc
📝 WalkthroughWalkthroughIn ChangesMultimodal E2E Test Assertion Relaxation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function disambiguate_mm_hashes in multimodal.rs to ensure that duplicate images in a request receive distinct hash entries by suffixing repeats (e.g., -dup1). This prevents vLLM from collapsing identical images and losing the user-sent image count, while still allowing the first occurrence to hit the cross-request encoder cache. A corresponding unit test was also added to verify this behavior. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
👋 The PR description doesn't fully follow PULL_REQUEST_TEMPLATE.md:
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix. The disambiguate_mm_hashes approach is sound — keeping the first occurrence's content hash preserves cross-request encoder caching while suffixing repeats ensures vLLM treats each image as distinct. Unit test covers the key cases well.
0 issues found (0 🔴 Important · 0 🟡 Nit · 0 🟣 Pre-existing)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
`test_multi_images_mixed` sends the same pug twice (mixed base64 + URL) and asserted the model counts "3". Engines legitimately differ on byte-identical multimodal inputs: vLLM deduplicates them (encodes the duplicate once), sglang keeps both — so the literal count is engine-dependent. This surfaced after #1604 made image decode bit-deterministic, so identical inputs now produce identical pixel tensors that vLLM dedups (sglang is unaffected). Drop the exact-count assertion and keep the duplicate-detection assertion (which both engines satisfy) plus the dog/pug content checks, so the test still validates multi-image + mixed base64/URL + duplicate handling. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
be3bd19 to
82ba66d
Compare
github-actions
Bot
added
tests
slin1237
changed the title
|
Repurposed this PR: the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ba66d907
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Don't assert an exact image count: the two pug inputs are byte-identical, | ||
| # and engines legitimately differ on whether identical multimodal inputs are | ||
| # deduplicated (vLLM encodes the duplicate once; sglang keeps both). The | ||
| # duplicate-detection assertion below covers the intent of this test. |
There was a problem hiding this comment.
This test sends three image parts (dog, URL pug, and the same pug as base64), and the documented regression is vLLM collapsing the byte-identical pugs so the model only sees two images. Replacing the count check with this comment makes test_multi_images_mixed accept that exact broken behavior; the later duplicate check can still pass on wording like same/both even when only two images are visible. Please keep an assertion that the response counts all three user-supplied images, or make the expected count engine-specific, so duplicate-image loss is caught.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e_test/chat_completions/test_multimodal.py`:
- Around line 145-148: The image count assertion has been removed in favor of a
comment explaining backend differences, which loses direct regression coverage
for the "duplicate images collapse" case. Restore the image count assertion but
make it backend-aware by conditionally asserting different expected counts based
on the engine being tested: vLLM should show fewer images due to deduplication
while sglang should preserve both copies. This preserves the regression guard
while remaining cross-engine compatible, ensuring the model still receives all
user-sent images regardless of backend.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bdfac13-d3eb-4de7-ad8c-c87792f372b2
📒 Files selected for processing (1)
e2e_test/chat_completions/test_multimodal.py
| # Don't assert an exact image count: the two pug inputs are byte-identical, | ||
| # and engines legitimately differ on whether identical multimodal inputs are | ||
| # deduplicated (vLLM encodes the duplicate once; sglang keeps both). The | ||
| # duplicate-detection assertion below covers the intent of this test. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore a backend-aware image-count assertion to keep regression coverage.
Removing the count assertion drops the only direct guard for the “duplicate images collapse into fewer visible images” regression. The duplicate-language check does not validate that the model still sees all user-sent images. Keep this test cross-engine compatible by asserting count conditionally (or by splitting per-engine expectations) instead of removing the count check entirely.
Suggested direction
- # Don't assert an exact image count: the two pug inputs are byte-identical,
- # and engines legitimately differ on whether identical multimodal inputs are
- # deduplicated (vLLM encodes the duplicate once; sglang keeps both). The
- # duplicate-detection assertion below covers the intent of this test.
+ # Keep explicit count coverage for the regression where duplicate images were
+ # collapsed on some engines. If behavior differs by engine, assert per-engine
+ # expectations rather than removing count validation.
+ # Example: vLLM path should report 3 after disambiguating duplicate mm_hashes.
+ # (Use the backend selector already available in setup_backend/markers.)
+ assert any(k in text_lower for k in ["3", "three"]), (
+ f"Expected explicit 3-image count, got: {text}"
+ )🤖 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 `@e2e_test/chat_completions/test_multimodal.py` around lines 145 - 148, The
image count assertion has been removed in favor of a comment explaining backend
differences, which loses direct regression coverage for the "duplicate images
collapse" case. Restore the image count assertion but make it backend-aware by
conditionally asserting different expected counts based on the engine being
tested: vLLM should show fewer images due to deduplication while sglang should
preserve both copies. This preserves the regression guard while remaining
cross-engine compatible, ensuring the model still receives all user-sent images
regardless of backend.
1 participant
Description
Problem
e2e_test/chat_completions/test_multimodal.py::TestMultimodalQwen3VL::test_multi_images_mixed[grpc]has been failing onmainfor the vLLM engine (Expected model to count 3 images, got: "two images").Root cause
Bisected to #1604 (e2e passed on the parent commit, failed on the merge; the test + fixtures +
mm_hasheswere unchanged across that boundary). #1604 switched the Qwen image resize to a PIL-exact deterministic bicubic for HF/vLLM parity. A side effect: the two byte-identical pug inputs (sent as base64 + URL) now produce byte-identical pixel tensors.Engines then legitimately differ on byte-identical multimodal inputs:
So the literal "count 3" assertion is engine-dependent for duplicate inputs. (This is not a gateway bug — the gateway emits 3 distinct image regions; it's vLLM's content-level dedup, which is defensible.)
Fix
Drop the exact-count assertion in
test_multi_images_mixed. Keep the duplicate-detection assertion (both engines satisfy it) and the dog/pug content checks, so the test still validates multi-image + mixed base64/URL + duplicate handling without depending on engine-specific dedup of identical images.Test Plan
ruff check+ruff format --checkclean.e2e-1gpu-chat (vllm)test_multi_images_mixed[grpc]pass (it already passes on sglang); verified by this PR's GPU e2e.Checklist
ruffcleanSummary by CodeRabbit