Skip to content

Qwen2VL: skip base input_ids-inputs_embeds equivalence check #34535

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.

Sign up for GitHub

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

Merged
gante merged 1 commit into from
Oct 31, 2024
Merged

Qwen2VL: skip base input_ids-inputs_embeds equivalence check #34535

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/generation/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Contributor Author

@gante gante Oct 31, 2024 •
edited
Loading

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

@ydshieh ydshieh Oct 31, 2024

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

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

@ydshieh ydshieh Oct 31, 2024

Choose a reason for hiding this comment

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

got it :-) 👍

)
# 3 - `inputs_dict` doesn't contain `attention_mask`. When `attention_mask` is not passed to generate,
# we infer it from `input_ids`. The last test case will fail if there is a pad token in the original input.
Expand Down