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.

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

Jump to bottom

[Zero-shot image classification pipeline] Remove tokenizer_kwargs #33174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Zero-shot image classification pipeline] Remove tokenizer_kwargs #33174

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d93bb5
First draft
NielsRogge Aug 28, 2024
abdf5c5
Pass attribute to from_pretrained
NielsRogge Aug 29, 2024
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: 2 additions & 0 deletions src/transformers/models/blip_2/convert_blip_2_original_to_pytorch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ def convert_blip2_checkpoint(
else:
tokenizer = AutoTokenizer.from_pretrained("google/flan-t5-xl")

tokenizer.model_input_names = ["input_ids", "attention_mask"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is pretty hacky. It should at the very least only be done in the BertTokenizer branch to make explicit why this is needed (including an explanatory comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see what's hacky about this, model_input_names is a genuine attribute of PretrainedTokenizer that has been there since BERT/GPT-2 came out (it's not used a lot). As can be seen here, a tokenizer will return token_type_ids if it's present in the model_input_names.

A less "hacky" way would be to pass model_input_names directly in the from_pretrained method, which the Transformers library also supports.

Copy link
Contributor Author

@NielsRogge NielsRogge Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What I'm trying to say is that one can avoid lines like these by appropriately setting the model_input_names attribute of the tokenizer, which is something we can still do for the Blip2ImageTextRetrieval models on the hub as they are just added and not yet in a stable release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, yes, I see. Although both modifying model_input_names and return_token_ids are somewhat hacky: they modify a class attribute which might be depended upon for other behaviours / assumed to have certain properties. Passing in model_input_names to the init is better, as this is more likely to correctly propagate any changes to any other dependant attributes / variables.


if "itm" in model_name:
eos_token_id = None
else:
Expand Down
12 changes: 2 additions & 10 deletions src/transformers/pipelines/zero_shot_image_classification.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,6 @@ def __call__(self, images: Union[str, List[str], "Image", List["Image"]], **kwar
The maximum time in seconds to wait for fetching images from the web. If None, no timeout is set and
the call may block forever.

tokenizer_kwargs (`dict`, *optional*):
Additional dictionary of keyword arguments passed along to the tokenizer.

Comment on lines -100 to -102
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be removed. tokenizer_kwargs is a fairly standard input to _sanitize_parameters as a way to control tokenizer behaviour. It would also be breaking for anyone using this in their pipelines.

We might be able to remove in the tests but it should stay here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm could you clarify? tokenizer_kwargs was added in #29261 which is not yet in a stable release. Hence removing this argument wouldn't break anything

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK, in this case we can remove!

Return:
A list of dictionaries containing one entry per proposed label. Each dictionary contains the
following keys:
Expand All @@ -109,16 +106,14 @@ def __call__(self, images: Union[str, List[str], "Image", List["Image"]], **kwar
"""
return super().__call__(images, **kwargs)

def _sanitize_parameters(self, tokenizer_kwargs=None, **kwargs):
def _sanitize_parameters(self, **kwargs):
preprocess_params = {}
if "candidate_labels" in kwargs:
preprocess_params["candidate_labels"] = kwargs["candidate_labels"]
if "timeout" in kwargs:
preprocess_params["timeout"] = kwargs["timeout"]
if "hypothesis_template" in kwargs:
preprocess_params["hypothesis_template"] = kwargs["hypothesis_template"]
if tokenizer_kwargs is not None:
preprocess_params["tokenizer_kwargs"] = tokenizer_kwargs

return preprocess_params, {}, {}

Expand All @@ -128,18 +123,15 @@ def preprocess(
candidate_labels=None,
hypothesis_template="This is a photo of {}.",
timeout=None,
tokenizer_kwargs=None,
):
if tokenizer_kwargs is None:
tokenizer_kwargs = {}
image = load_image(image, timeout=timeout)
inputs = self.image_processor(images=[image], return_tensors=self.framework)
if self.framework == "pt":
inputs = inputs.to(self.torch_dtype)
inputs["candidate_labels"] = candidate_labels
sequences = [hypothesis_template.format(x) for x in candidate_labels]
padding = "max_length" if self.model.config.model_type == "siglip" else True
text_inputs = self.tokenizer(sequences, return_tensors=self.framework, padding=padding, **tokenizer_kwargs)
text_inputs = self.tokenizer(sequences, return_tensors=self.framework, padding=padding)
inputs["text_inputs"] = [text_inputs]
return inputs

Expand Down
2 changes: 0 additions & 2 deletions tests/pipelines/test_pipelines_zero_shot_image_classification.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,6 @@ def test_blip2_model_pt(self):
output = image_classifier(
image,
candidate_labels=["2 cats", "a plane", "a remote"],
tokenizer_kwargs={"return_token_type_ids": False},
)

self.assertEqual(
Expand All @@ -308,7 +307,6 @@ def test_blip2_model_pt(self):
[image] * 5,
candidate_labels=["2 cats", "a plane", "a remote"],
batch_size=2,
tokenizer_kwargs={"return_token_type_ids": False},
)

self.assertEqual(
Expand Down
Loading