Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qwen2VL: skip base input_ids-inputs_embeds equivalence check #34535

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

gante
Copy link
Member

@gante gante commented Oct 31, 2024

What does this PR do?

Computing inputs_embeds in Qwen2VL is more complex than simply embedding input_ids (see Qwen2VLForConditionalGeneration), so the basic check doesn't apply :)

Fixes:

py.test tests/models/qwen2_vl/test_modeling_qwen2_vl.py::Qwen2VLModelTest::test_generate_from_inputs_embeds_1_beam_search --flake-finder --flake-runs 100

@gante gante requested a review from ydshieh October 31, 2024 14:37
@@ -1610,7 +1610,7 @@ def test_generate_from_inputs_embeds(self, _, num_beams):
inputs_dict.pop("pixel_values_images", None)
# 2.C - No easy fix, let's skip the check that compares the outputs from `input_ids` and `inputs_embeds`
has_complex_embeds_computation = any(
model_name in model_class.__name__.lower() for model_name in ["moshi"]
model_name in model_class.__name__.lower() for model_name in ["moshi", "qwen2vl"]
Copy link
Member Author

@gante gante Oct 31, 2024

Choose a reason for hiding this comment

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

This flag skips the check that passing inputs_embeds = model.get_input_embeddings()(input_ids) should be equivalent to passing input_ids to generate.

This is not true in Qwen2VL, whose computation of inputs_embeds is more complex

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK for me. Just curious: would this be addressed in the future to make it work

Copy link
Member Author

Choose a reason for hiding this comment

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

Not unless we create a model function to embed inputs, similar to def get_input_embeddings(self).

(I think we should do it eventually, we're starting to have a few models with non-standard inputs embeddings creation!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it :-) 👍

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

@gante gante merged commit 4ca004e into huggingface:main Oct 31, 2024
26 checks passed
@gante gante deleted the qwen2vl_flaky_inputs_embeds branch October 31, 2024 15:42
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
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