Skip to content

Internvl fix#38946

Merged
remi-or merged 4 commits into
huggingface:mainfrom
remi-or:internvl-fix
Jun 26, 2025
Merged

Internvl fix#38946
remi-or merged 4 commits into
huggingface:mainfrom
remi-or:internvl-fix

Conversation

@remi-or

@remi-or remi-or commented Jun 20, 2025

Copy link
Copy Markdown
Collaborator

This PR brings to main a fix that was added in amd-hf-ci-branch here: #38540
The fix aims to correct a failure of torch.compile when compiling a torchvision resize function with an uint8 input. The failing tests were:

tests/models/internvl/test_video_processor_internvl.py::test_can_compile_fast_video_processor
tests/models/qwen2_vl/test_video_processing_qwen2_vl.py::test_can_compile_fast_video_processor

This PR also add some Expectations for AMD machines.

@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.

Comment on lines +283 to +288
# This is a workaround to avoid a bug in torch.compile when dealing with uint8 on AMD MI3XX GPUs
# Tracked in PyTorch issue: https://github.com/pytorch/pytorch/issues/155209
# TODO: remove this once the bug is fixed (detected with torch==2.7.0+git1fee196, torchvision==0.22.0+9eb57cd)
if torch.compiler.is_compiling() and is_rocm_platform():
return self.compile_friendly_resize(image, new_size, interpolation, antialias)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could you share what tests would need this?

Why we have compiling in image processing stage ..?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added the failing tests to the PR description! As for why there is compiling, I am not sure -- probably helps speedup the processing stage

shorter = size.shortest_edge
longer = int(1333 / 800 * shorter)
output_size = get_resize_output_image_size(
output_height, output_width = get_resize_output_image_size(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could you explain why such changes here and below?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The idea here is to use the parent's class resize function rather than torchvision equivalent directly. To do that, we unpack the output_size to then use the output_height and output_width to build a SizeDict which is needed for super().resize

Comment thread tests/models/internvl/test_modeling_internvl.py
@ydshieh

ydshieh commented Jun 20, 2025

Copy link
Copy Markdown
Collaborator

It's better to explain a bit about the changes. (maybe it's explained in the PR for amd-hf-ci-branch).

Also since this is not only test files, I feel it's better for other team members (who work a lot on image stuffs ) to take a look too.

@remi-or

remi-or commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator Author

Yes you are right, my bad! I added more context to the PR description. And as for other team members, I agree, tagging @yonigozlan and @zucchini-nlp as they already reviewed the last PR.

@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.

Since it was discussed already in side branch, lgtm!

Tiny nit: I see many other models calling F.resize instead of super() which aren't fixed in this PR (e.g. pixtral, convnext). Do we need to fix them as well, or are the tests not failing for them?

Comment on lines +247 to +256
stacked_videos = self.resize(
image=stacked_videos,
size=SizeDict(height=resized_height, width=resized_width),
interpolation=interpolation,

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.

Qwen2-vl image processor as well 😄

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Made the change in image_processing_qwen2_vl_fast.py but not in image_processing_qwen2_vl.py because it has a input_data_format argument I am not sure about -- did you mean to change it in both?

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.

No, only the fast processor. Slow processors don't rely on torchvision :)

@remi-or

remi-or commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator Author

We only looked at tests that failed on Mi3XX that passed on the T4 CI, so either (1) the tests did not pass on the T4 (2) they were skipped on MI3XX or (3) they passed for some weird reason (no compilation, etc). I can look into those tests next

@zucchini-nlp

Copy link
Copy Markdown
Member

Yeah, let's check those tests as well. In case the compile issue is not affecting all models equally under same settings, I think we should not need the workaround

Comment thread src/transformers/image_processing_utils_fast.py Outdated

@yonigozlan yonigozlan 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.

LGTM as well! Agreed with @zucchini-nlp, if we can address the issue of using F.resize instead of self.resize (or super().resize) for all fast image processors that would be great, might be a bit out of scope for this PR though

@ydshieh

ydshieh commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator

run-slow: bridgetower, llava_next, llava_onevision, qwen2_vl, qwen2_vl

@ydshieh

ydshieh commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator

let's wait the above CI runs

@github-actions

Copy link
Copy Markdown
Contributor

This comment contains run-slow, running the specified jobs:

models: ['models/bridgetower', 'models/llava_next', 'models/llava_onevision', 'models/qwen2_vl']
quantizations: [] ...

@remi-or

remi-or commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator Author

Yeah, let's check those tests as well. In case the compile issue is not affecting all models equally under same settings, I think we should not need the workaround

Tested with pixtral, and the test test_can_compile_fast_image_processor passes when F.resize output size is smaller than the input size (for instance, here input size is [1, 3, 224, 244] and output size is [24, 24]) but when we pick an output size larger (tried w/ [448, 448]) we get the same failures. So it seems only the up-sample code path is broken, which is why some tests have an issue and some dont. Will update the issue with that information

@ydshieh

ydshieh commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator

run-slow: bridgetower, llava_next, llava_onevision, qwen2_vl, qwen2_vl

@github-actions

Copy link
Copy Markdown
Contributor

This comment contains run-slow, running the specified jobs:

models: ['models/bridgetower', 'models/llava_next', 'models/llava_onevision', 'models/qwen2_vl']
quantizations: [] ...

@zucchini-nlp

Copy link
Copy Markdown
Member

Great @remi-or , thanks for digging!

@ydshieh

ydshieh commented Jun 24, 2025

Copy link
Copy Markdown
Collaborator

slow tests look good. I think we could merge?

@zucchini-nlp

Copy link
Copy Markdown
Member

Yep, I also think we need to replace F.resize with out resize in other models, even though the tests are passing. In actual inference with rocm we aren't guaranteed to have resized shape smaller than the original shape

@remi-or

remi-or commented Jun 24, 2025

Copy link
Copy Markdown
Collaborator Author

Should we take care of it in this PR or should we leave that for a PR with a larger scope?

remi-or and others added 3 commits June 26, 2025 05:35
* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
@remi-or remi-or merged commit 25c44d4 into huggingface:main Jun 26, 2025
20 checks passed
@remi-or remi-or deleted the internvl-fix branch June 26, 2025 11:48
@remi-or remi-or restored the internvl-fix branch June 26, 2025 16:59
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* Image processor compile fix (huggingface#38540)

* Added a compile-friendly versiom of resize to BaseImgProcessorFast

* Changed qwen2 processor to use its parent class .resize

* Style

* underlined issue only happens on AMD w/ comment and bool check

* Fixed some utils functions

* Fixed the same issue for bridgetower

* Fixed the same issue for llava_next

* Repo consistency for llava onevision

* Update src/transformers/image_processing_utils_fast.py

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>

* Added an Expectation to an internvl test

* Made qwen2_vl use the resize method of its parent clas

* Changed to torch.where

---------

Co-authored-by: Mohit Sharma <mohit21sharma.ms@gmail.com>
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.

6 participants