Skip to content

Fix Idefics2 padding when the first example has no image - #48753

Merged
zucchini-nlp merged 8 commits into
huggingface:mainfrom
Arnavsharma2:fix/idefics2-text-only-batch-padding
Sep 17, 2026
Merged

zucchini-nlp merged 8 commits into
huggingface:mainfrom
Arnavsharma2:fix/idefics2-text-only-batch-padding

Conversation

@Arnavsharma2

@Arnavsharma2 Arnavsharma2 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

CPU CI GPU run-slow

Idefics2 raises IndexError for [[], [image]] because padding assumes the first batch row contains an image. Use the first available image for channels/device in both backends, preserving image outputs and zero masks for text-only examples.

Related to #31854

Tests

3 failures -> 3 passes

  • python -m pytest -q tests/models/idefics2/test_image_processing_idefics2.py: 25 passed, 3 skipped.
  • python -m pytest -q tests/models/idefics2/test_processing_idefics2.py: 31 passed, 31 skipped.
  • make fix-repo and make typing: passed.

CPU only

AI disclosure

AI helped in the development of tests and code

@Arnavsharma2
Arnavsharma2 marked this pull request as ready for review September 13, 2026 05:20
Copilot AI lite review requested due to automatic review settings September 13, 2026 05:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@zucchini-nlp zucchini-nlp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The support has been added already (in #36682) and there is a test as well, can you double-check if the test is running on Idefics2 and provide a clear reproducer of the "bug"?

@zucchini-nlp

Copy link
Copy Markdown
Member

If it is just a PR trying to solve an old GH issue, better to simply close it! The issue is too old and has been resolved

@Arnavsharma2

Copy link
Copy Markdown
Contributor Author

The support has been added already (in #36682) and there is a test as well, can you double-check if the test is running on Idefics2 and provide a clear reproducer of the "bug"?

Hi!

I double-checked, and [[], [image]] still raises IndexError in both Idefics2 image-processing backends.

I also ran test_processor_text_has_no_visual for Idefics2.

It is collected but skipped because the guard needs both images and videos in the processor signature.

Here's a small reproducer:

import numpy as np
from transformers import (
    Idefics2ImageProcessor,
    Idefics2ImageProcessorPil,
)

image = np.zeros((8, 10, 3), dtype=np.uint8)

for cls in (Idefics2ImageProcessor, Idefics2ImageProcessorPil):
    processor = cls(do_resize=False, do_image_splitting=False)
    try:
        result = processor([[], [image]], return_tensors="pt")
        print(cls.__name__, result.pixel_values.shape)
    except IndexError as error:
        print(cls.__name__, repr(error))

Both of them fail during padding when accessing the first image of the empty first row. My changes change it so that both return shape (2, 1, 3, 8, 10) with a zero pixel-attention mask for the text-only row.

@zucchini-nlp

Copy link
Copy Markdown
Member

the guard needs both images and videos in the processor signature

nooooooo 😿 Oke, in that case would you be open to re-make the test and allow image-only inputs as well? That feature should be working in all VLMs, no matter if video is supported or not

@Arnavsharma2

Copy link
Copy Markdown
Contributor Author

the guard needs both images and videos in the processor signature

nooooooo 😿 Oke, in that case would you be open to re-make the test and allow image-only inputs as well? That feature should be working in all VLMs, no matter if video is supported or not

Sure!

@Arnavsharma2

Copy link
Copy Markdown
Contributor Author

the guard needs both images and videos in the processor signature

nooooooo 😿 Oke, in that case would you be open to re-make the test and allow image-only inputs as well? That feature should be working in all VLMs, no matter if video is supported or not

Just pushed the changes!

@zucchini-nlp

Copy link
Copy Markdown
Member

We'll defi need to fix failing models now, there should be more than only Idefics2

@Arnavsharma2

Copy link
Copy Markdown
Contributor Author

We'll defi need to fix failing models now, there should be more than only Idefics2

Pushed the follow-up!
PI0 was also crashing on empty image rows, so I fixed that and added test coverage

NeoMME already supports mixed document batches, but inputs didn’t match its retrieval format. I modified its test to cover both direct inputs and its supported chat format

GLM-Image requires equal source-image counts across the batch for image generation. I kept that restriction and made a test that ensures it holds up that restriction!

Tests pass: 88 pass 103 skip

@zucchini-nlp zucchini-nlp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! no need for new tests though right?

Comment thread tests/models/pi0/test_processing_pi0.py Resolved
Comment thread tests/test_processing_common.py Resolved

@zucchini-nlp zucchini-nlp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Sep 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 17, 2026
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Sep 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: glm_image, idefics2, lighton_ocr, neomme, pi0

@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 35206572589:2
Result: success | Jobs: 16 | Tests: 190,478 | Failures: 2 | Duration: 13h 18m

@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Sep 17, 2026
Merged via the queue into huggingface:main with commit 2e39c41 Sep 17, 2026
116 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants