Repository navigation
Fix HunYuan-VL image assignment during batched generation - #49557
Conversation
|
Thank you for your contribution 🤗! CI Security Gate — waiting for maintainer approvalCI is approved automatically only for authors who already have a contribution merged in this repository. A maintainer will review this PR and approve CI to run on it. |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
A few tiny comments and if the new test is passing for all VLMs image/video - lgtm
| image_outputs = self.vision_tower(pixel_values, grid_thw=image_grid_thw, **kwargs) | ||
| # Include row separators and image boundary tokens in each split. | ||
| merged_h, merged_w = (image_grid_thw[:, 1:] // self.config.vision_config.spatial_merge_size).unbind(-1) | ||
| split_sizes = (merged_h * (merged_w + 1) + 2).tolist() |
There was a problem hiding this comment.
i am seeing +2 if cat_extra_tokenin processor, can you check that all released ckpt actually add boundary tokens
zucchini-nlp
added
the
for patch
|
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. |
|
run-slow: llava_next_video, idefics2, idefics3, smolvlm, hunyuan_vl |
|
okek, now it is fine, i will be off tmrw and wrap up/merge on Thursday before release |
|
Thanks for the fixes! I checked HunyuanOCR too, it includes the begin/end tokens, so the +2 is correct. All good from my side, thanks! |
|
run-slow: chameleon, gemma4, gemma4_unified, hunyuan_vl, idefics2, idefics3, inkling, llava_next_video, nemotron_h_omni, smolvlm |
CI (Nvidia + AMD)Nvidia CICommit Info
✅ No failing test specific to this PR 🎉 👏 ! AMD CICommit Info
✅ No failing test specific to this PR 🎉 👏 ! |
|
oke the test is failing because we didn't handle idefics with multi-image where images could be padded-unpadded 😭 |
ac66f04 to
cf36cf6
Compare
|
run-slow: idefics2, idefics3, smolvlm |
|
@Arnav66692 i am trying to merge, pls dont push. I pushed the correct fix we need |
|
Sorry for pushing while you were trying to merge. Thanks for fixing this and for all your help reviewing it. I’ll hold off on any further pushes. |
|
run-slow: idefics2, idefics3, smolvlm, modernbert |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: chameleon, gemma4, gemma4_unified, hunyuan_vl, idefics2, idefics3, inkling, llava_next_video, modernvbert, nemotron_h_omni, smolvlm |
CI recapDashboard: View test results in Grafana |
|
Hi @zucchini-nlp, thanks for the review and for the fixes you pushed. The PR seems to be approved now and all checks are green. Whenever you have a moment, could it be merged? Happy to make any last changes if needed. Again thank you for your help in all of this. :) |
|
i forgot to enable the auto-? |
|
Hi @zucchini-nlp, sorry, small correction to my last comment. The checks on the PR look good to go, but it keeps getting kicked out of the merge queue. I looked at the failed runs and it doesn't seem related to the HunYuan change. Every time it's the same two muse_spark tests in tests_processors (test_apply_chat_template_audio and test_apply_chat_template_multi_audio), and they fail with a 401 Unauthorized from the hf-internal-testing/muse-spark-integration-test bucket. #49428 is getting removed from the queue for the same two tests. Also the tensor parallel and tokenization failures on the last run look like runner issues, not actual test failures. Could we re-queue it when you get a chance and please do let me know if there's anything I should change on my side, really sorry for having this be such a hassle 😭 |
|
@ArthurZucker @vasqu one of them will force merge and add it to today's patch, dw |
* fix: return per-image image features for HunYuan-VL batched generation * test: clarify image feature checks and skip unrelated models * gotta fix the test, `num_images` isn't the actual batch size * fix models except nemotron * oops, meant this! * fix * oh no, flatten was wrong dim * fix repo as well * fix repo didnt copy it * i hare modular --------- Co-authored-by: raushan <raushan@huggingface.co>
4 participants
What does this PR do?
HunYuan-VL was returning all image features in one tensor, so some beams got the wrong image during batched generation. This PR splits the features per image to fix that. I also moved the shared pooler_output check into its own test and added a HunYuan test that compares first-step logits from beam search with greedy decoding. No new runtime dependencies are needed.
Validation:
Fixes #49499.
Code Agent Policy
The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. These often are low-quality, or fix extremely minor issues that occur rarely or never in practice.
As a result, we're instituting a rule that first-time contributors should not use code agents to submit PRs or issues.
We'd also ask autonomous "OpenClaw"-like agents not to open any PRs or issues.
Issues/PRs from first-time contributors that violate this rule will probably just be closed without review, and we
might block you, especially if you open more than one or appear to be deliberately ignoring this. We especially do not
want new contributors to jump in on random issues to contribute an agent-written fix. This creates lots of noise
for reviewers and other users and will almost certainly get you blocked.
For more information, please read
CONTRIBUTING.md.I confirm that this PR description and code is not written by an LLM or code agent
Before submitting
No, this is a bug fix
Pull Request checks?
Yes I have read them
to it if that's the case.
Yes I discussed in #49499 with @zucchini-nlp and he gave approval to let me make a PR request on this issue.
Not needed for this fix since get_image_features now returns pooler_output as a tuple with one tensor per image, but that output shape isn’t described in the docs.
Yes, I added the shared image-feature check and HunYuan beam-search regression.
Who can review?
@zucchini-nlp