Repository navigation
[Bugfix][Multimodal] Fix DeepSeek-OCR zero-sum patch guard in _encode… - #64091
Conversation
…_local_features Fixes vllm-project#64075. _encode_local_features in both deepseek_ocr.py and deepseek_ocr2.py guarded against empty patches with: if torch.sum(patches).item() == 0: return None This is wrong when a tiled image produces crop patches whose normalized pixel values cancel to zero (e.g. a half-white/half-black 1536x768 image after Normalize(0.5, 0.5)). The guard returned None even though patches was non-empty (numel > 0), making _pixel_values_to_embedding build the combined embedding from global features only. That embedding was shorter than the placeholder count expected by _merge_multimodal_embeddings: RuntimeError: shape mismatch: value tensor of shape [257, 1280] cannot be broadcast to indexing result of shape [545, 1280] The engine then raised a fatal error and shut down. PR vllm-project#59417 fixed the equivalent guard in _parse_and_validate_image_input (the pixel_values layer) but explicitly left _encode_local_features unchanged, noting it "is not engine-fatal". Issue vllm-project#64075 shows it is. Fix: replace the sum-based guard with patches.numel() == 0, which is the correct test for "no crop patches were produced" (non-tiled image). Tested on base commit (before fix): test_ocr2_zero_sum_patches_enters_model_body FAILED test_ocr1_zero_sum_patches_enters_model_body FAILED With fix: 4/4 new tests PASSED, 7/7 related tests PASSED Signed-off-by: nrenvo <nrenvo@gmail.com>
c295992 to
91c7398
Compare
|
Thanks for working on this! |
|
Hey, thanks for the note. I already have a fix up with regression tests for both DeepSeek-OCR and OCR-2, validated against the base commit, so I'd prefer to keep this PR open and let the maintainers decide. Happy to compare approaches if that helps. |
|
Thanks, I understand you’d like to keep this PR open. The current unit tests cover the zero-sum guard, but they don’t exercise the end-to-end inference path reported in #64075. My commit adds a I ran the tests on an RTX 5070 Ti and observed:
I’d be happy to contribute the end-to-end test if it would help strengthen this PR. As a minor style suggestion, the commit message is too long maybe. |
|
Thanks, fair point — the unit tests cover the guard, but you're right that they don't exercise the full inference path. Happy to include your e2e test. Could you put the test on top of 91c7398 and open a cross-fork PR against nrenvo:fix/deepseek-ocr-zero-sum-patches with just the test file? I'll merge it and keep your authorship. If that's a hassle, paste the test file here and I'll apply it with a Co-authored-by. And thanks for the commit message note. |
|
FYI @nrenvo Details |
|
Added your test as a separate commit with your authorship — thanks for putting it together. |
| assert torch.equal(image_input.data, pixel_values) | ||
|
|
||
|
|
||
| class TestDeepseekOCREncodeLocalFeaturesGuard: |
There was a problem hiding this comment.
Let's move this to generation directory
There was a problem hiding this comment.
Done, moved TestDeepseekOCREncodeLocalFeaturesGuard to generation/test_deepseek_ocr_guard.py!
| @@ -0,0 +1,60 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
Let's save CI cost and only include the processing tests
There was a problem hiding this comment.
Yep, dropped the vllm_runner e2e test — the guard tests cover the fix without the extra CI cost.
d3e1c56 to
ce39a8b
Compare
Move TestDeepseekOCREncodeLocalFeaturesGuard from tests/models/multimodal/processing/test_deepseek_ocr.py to tests/models/multimodal/generation/test_deepseek_ocr_guard.py per reviewer feedback; remove the heavier vllm_runner e2e test to save CI cost. Signed-off-by: nrenvo <nrenvo@gmail.com>
ce39a8b to
e74ecc9
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #97898 for commit ℹ️ This PR is 78 commits behind upstream |
|
Hi @nrenvo, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
Move TestDeepseekOCREncodeLocalFeaturesGuard from tests/models/multimodal/processing/test_deepseek_ocr.py to tests/models/multimodal/generation/test_deepseek_ocr_guard.py per reviewer feedback; remove the heavier vllm_runner e2e test to save CI cost. Signed-off-by: nrenvo <nrenvo@gmail.com>
Head branch was pushed to by a user without write access
|
/ci run |
|
✅ Triggered Buildkite CI #97902 for commit ℹ️ This PR is 2 commits behind upstream |
Fixes #64075.
_encode_local_features in deepseek_ocr.py and deepseek_ocr2.py used torch.sum(patches).item() == 0 to detect "no crop patches", so a tiled image whose normalized pixels cancel to zero (e.g. half-white/half-black 1536x768) wrongly returned None, dropped local features, and crashed EngineCore with a placeholder/embedding shape mismatch. Replace the guard with patches.numel() == 0; #59417 fixed the equivalent check in _parse_and_validate_image_input but left this one as "not engine-fatal", which #64075 disproves.
New test class TestDeepseekOCREncodeLocalFeaturesGuard fails on the base commit (2 failed) and passes with the fix (13 passed in tests/models/multimodal/processing/test_deepseek_ocr.py); ruff check and ruff format are clean.