KEMBAR78
Fix processing tests by zucchini-nlp · Pull Request #40379 · huggingface/transformers · GitHub
Skip to content

Conversation

@zucchini-nlp
Copy link
Member

@zucchini-nlp zucchini-nlp commented Aug 22, 2025

What does this PR do?

I broke the CI in #40339 🙈 No idea why the tests weren't triggered before merging

The newly added test is failing for InetrnVL, QwenAudio (and others) because the number of video/audio placeholders doesn't match number of actual video/audios. The fix is to allow to prepare dummy text with different modality placeholder, especially useful when a processor can process more than one modality

@zucchini-nlp zucchini-nlp requested a review from ydshieh August 22, 2025 16:20
@HuggingFaceDocBuilderDev

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.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Would love a bit more detailed in the PR description 🙏 (no need to be a thesis, but something that we can know what/why the previous PR break and a short explanation of the changes in this PR)

@ydshieh
Copy link
Collaborator

ydshieh commented Aug 25, 2025

No idea why the tests weren't triggered before merging

I will check this afternoon, but I need a bit more context (so my comment above 🙏 )


@property
def model_input_names(self):
# InternVL renames video inputs to `pixel_values` before returning
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice to have comment.

Might would be even better to explain a bit more:

  • does this simply "what we are doing here" (and if so, why?)
  • or does this mean because InternVL renames video inputs to pixel_values before returning, so we need to have this change here (otherwise something would go wrong)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, that is the reason to override it, will make it clear

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you

@github-actions
Copy link
Contributor

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

run-slow: aria, chameleon, deepseek_vl_hybrid, gemma3, glm4v, grounding_dino, hunyuan_v1_moe, internvl, llava, mistral3, mllama, pix2struct, qwen2_vl, smolvlm

@zucchini-nlp zucchini-nlp merged commit 3b5b9f6 into huggingface:main Aug 25, 2025
24 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.

3 participants