Skip to content

Add: num_additional_image_tokens to models #35052

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
zucchini-nlp merged 14 commits into from
Jan 8, 2025
Merged

Add: num_additional_image_tokens to models #35052

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f7b66a9
Add: num_additional_image_tokens to models
jp1924 Dec 3, 2024
17ca285
docs: update docstring for num_additional_image_tokens in configurati…
jp1924 Dec 3, 2024
5093d49
Add num_additional_image_tokens to LlavaNextVideo model and update fe…
jp1924 Dec 3, 2024
264f848
revert
jp1924 Dec 6, 2024
b666b29
Fix: adjust num_image_tokens calculation in LlavaProcessor
jp1924 Dec 6, 2024
e17d95e
Remove num_additional_image_tokens initialization from configuration …
jp1924 Dec 6, 2024
925eaef
Fix test error
jp1924 Dec 6, 2024
0eb8415
revert
jp1924 Dec 6, 2024
301a669
Fix: adjust num_image_tokens calculation in LlavaNextVideoProcessor
jp1924 Dec 8, 2024
8ee4024
fix conflict
jp1924 Dec 9, 2024
139e6af
Merge branch 'main' into img_mismatch_error
jp1924 Dec 9, 2024
114283b
Fix: adjust num_image_tokens calculation in VideoLlavaProcessor
jp1924 Dec 9, 2024
bd3765d
Merge branch 'main' into img_mismatch_error
zucchini-nlp Jan 8, 2025
bb73040
make style
zucchini-nlp Jan 8, 2025
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
1 change: 1 addition & 0 deletions src/transformers/models/llava/modeling_llava.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ def __init__(self, config: LlavaConfig):
self.vocab_size = config.text_config.vocab_size
self.language_model = AutoModelForCausalLM.from_config(config.text_config)
self.pad_token_id = self.config.pad_token_id if self.config.pad_token_id is not None else -1

self.post_init()

def get_input_embeddings(self):
Expand Down
2 changes: 1 addition & 1 deletion src/transformers/models/llava/processing_llava.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def __call__(
width // self.patch_size
) + self.num_additional_image_tokens
if self.vision_feature_select_strategy == "default":
num_image_tokens -= self.num_additional_image_tokens
num_image_tokens -= 1

@qubvel qubvel Jan 6, 2025

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.

L163: we add self.num_additional_image_tokens
L165: we subtract 1

Might that lead to a discrepancy?

@jp1924 jp1924 Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@qubvel
In the existing model code, selected_image_feature[:, 1:] is also hardcoded with 1, so there won't be any discrepancy.

@qubvel qubvel Jan 7, 2025

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.

If I got it right the idea is that "default" behaviour always slice one feature, while "full" does not slice anything. Why do we need self.num_additional_image_tokens then? Would it be more correct to fix the modeling code instead to make it consistent? @zucchini-nlp wdyt?

@zucchini-nlp zucchini-nlp Jan 7, 2025

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.

We add num_additional_image_tokens to account for the ViTs with/without CLS, so currently it is either 0 or 1. Then the modeling code has two options to select the features, either crop 1 token or take the full embeddings. So, the main reason why we added num_additional_image_tokens in the first place was to make processor flexible for different types of ViT and calculate image patch length from patch_size.

@qubvel hmm, not sure I got what you mean by modifying the model code?

@qubvel qubvel Jan 7, 2025

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.

I just trying to unedrstand why do we need both num_additional_image_tokens and vision_feature_select_strategy in processor. As far as I understand num_additional_image_tokens is enough to compute num_image_tokens, or am I missing something?

@qubvel qubvel Jan 7, 2025

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.

Not sure I got the whole picture 🥲 but yeah, introducing two dependent parameters might be confusing, am I right that we should only use it as follows?

  1. num_additional_image_tokens = 1 AND vision_feature_select_strategy = "default"
  2. num_additional_image_tokens = 0 AND vision_feature_select_strategy = "full"

@zucchini-nlp zucchini-nlp Jan 7, 2025

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.

I see, confusing picture here. Yes, currently I think those two are the combinations used but other combinations should not be a problem. For ex, if one wants to keep CLS for any reason and experiment with that

num_additional_image_tokens = 1 AND vision_feature_select_strategy = "full"

@qubvel qubvel Jan 7, 2025

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.

Ok, I hope I got it now 😄 let's ensure we have some docs and comments regarding it, cause it looks not obvious

@qubvel qubvel Jan 7, 2025 •
edited
Loading

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.

Maybe "default" is not the best name for it, cause I expected it to be like "remove cls token if it exists", but it looks like it is "remove first token in any case"

@zucchini-nlp zucchini-nlp Jan 7, 2025

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.

hehe, the naming comes from original implementation. I will update the docstring for more clarity then, in a subsequent PR since this one is about fixing a bug


prompt_strings = []
for sample in text:
Expand Down
2 changes: 1 addition & 1 deletion src/transformers/models/llava_next/processing_llava_next.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def __call__(
orig_height, orig_width = image_size
num_image_tokens = self._get_number_of_features(orig_height, orig_width, height, width)
if self.vision_feature_select_strategy == "default":
num_image_tokens -= self.num_additional_image_tokens
num_image_tokens -= 1
sample = sample.replace(self.image_token, "<placeholder>" * num_image_tokens, 1)
prompt_strings.append(sample)
prompt_strings = [sample.replace("<placeholder>", self.image_token) for sample in prompt_strings]
Expand Down
2 changes: 1 addition & 1 deletion src/transformers/models/llava_next_video/processing_llava_next_video.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ def __call__(
orig_height, orig_width = image_size
num_image_tokens = self._get_number_of_features(orig_height, orig_width, height, width)
if self.vision_feature_select_strategy == "default":
num_image_tokens -= self.num_additional_image_tokens
num_image_tokens -= 1
sample = sample.replace(self.image_token, "<placeholder>" * num_image_tokens, 1)
prompt_strings.append(sample)
text = [sample.replace("<placeholder>", self.image_token) for sample in prompt_strings]
Expand Down
1 change: 1 addition & 0 deletions src/transformers/models/vipllava/modeling_vipllava.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ def __init__(self, config: VipLlavaConfig):
self.vocab_size = config.text_config.vocab_size
self.language_model = AutoModelForCausalLM.from_config(config.text_config)
self.pad_token_id = self.config.pad_token_id if self.config.pad_token_id is not None else -1

self.post_init()

def get_input_embeddings(self):
Expand Down