Skip to content

Conversation

yaswanth19
Copy link
Contributor

@yaswanth19 yaswanth19 commented Sep 11, 2025

As per the titles, simply removes image_size arg from Llama4's forward method as it's unused and deprecates vision_feature_layer arg.

@ArthurZucker for review

if pixel_values is not None:
image_features = self.get_image_features(
pixel_values=pixel_values,
vision_feature_layer=vision_feature_layer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vision_feature_layer is also not used in get_image_features and thus not necessary. Should I remove it, since we have kwargs here which should consume that whenever passed thus not breaking BC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep sounds good IMO 😉

@Rocketknight1
Copy link
Member

cc @zucchini-nlp as well

Copy link
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

Looks like a bad copy from Llava, and can be simply removed. I don't expect anyone to be using vision_feature_layer, so let's just remove it since it is inn utility method

@yaswanthg15
Copy link

yaswanthg15 commented Sep 12, 2025

@zucchini-nlp It's also an arg in forward and present in config as well, shall I remove from forward too, removing from config would be breaking. Otherwise def can only remove from utility func.

@zucchini-nlp
Copy link
Member

It's also an arg in forward and present in config as well, shall I remove from forward too, removing from config would be breaking.

Ahh I see, then we can do a tiny deprecation cycle of 1 minor release. Prob ir was also serialized in config in which case it will be still available with config.vision_layer. So we just need to raise warning when users try to set or get unused attributes

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks, not sure about the warning because if we pushed this to the hub, it is probably something users can do nothing about. We don't need a warning as the arg will always be read in **kwargs

@yaswanthg15
Copy link

@ArthurZucker Then you are suggesting that, we remove the arg from config completely (will be read in kwargs for all hub configs) and add the deprecate decorator in modeling file?

self.rope_theta = rope_theta

if not self.vision_feature_layer:
logger.warning("`vision_feature_layer` is deprecated and will be removed in v4.58")
Copy link
Member

Choose a reason for hiding this comment

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

i think we just need warning when users try to get the attribute or set to a different value, letting them know that vision_feature_layer has no effect on model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zucchini-nlp A code suggestion or reference would be better here. I can only think of a niave way to have getter and setter funcs for vision_feature_layer attr and raise a warning like you mentioned (instead of general warning) or is there a util func or decorator which can be used ootb

Copy link
Member

Choose a reason for hiding this comment

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

Smth like this

@property
def sliding_window_pattern(self):
warnings.warn(
"The `sliding_window_pattern` attribute is deprecated and will be removed in v4.55.0.",
FutureWarning,
)
return self._sliding_window_pattern
@sliding_window_pattern.setter
def sliding_window_pattern(self, value):
self._sliding_window_pattern = value

Copy link
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

Thanks, the generation test will be fixed soon and is not related :)

@zucchini-nlp
Copy link
Member

Lets rebase and merge :)

Copy link
Contributor

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

run-slow: llama4

@yaswanth19 yaswanth19 changed the title [Llama4] Remove an unused arg [Llama4] Remove image_sizes arg and deprecate vision_feature_layer Sep 17, 2025
@zucchini-nlp zucchini-nlp enabled auto-merge (squash) September 17, 2025 09:05
@zucchini-nlp zucchini-nlp merged commit ddd4caf into huggingface:main Sep 17, 2025
23 checks passed
@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.

ErfanBaghaei pushed a commit to ErfanBaghaei/transformers that referenced this pull request Sep 25, 2025
huggingface#40832)

* Remove unused arg

* deprecate

* revrt one change

* get set go

* version correction

* fix

* make style

* comment
vijayabhaskar-ev pushed a commit to vijayabhaskar-ev/transformers that referenced this pull request Oct 2, 2025
huggingface#40832)

* Remove unused arg

* deprecate

* revrt one change

* get set go

* version correction

* fix

* make style

* comment
yuchenxie4645 pushed a commit to yuchenxie4645/transformers that referenced this pull request Oct 4, 2025
huggingface#40832)

* Remove unused arg

* deprecate

* revrt one change

* get set go

* version correction

* fix

* make style

* comment
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