uniformize kwargs for OneFormer by tibor-reiss · Pull Request #34547 · huggingface/transformers

tibor-reiss · 2024-10-31T21:02:46Z

Adds uniformized processors for OneFormer following #31911.

Small changes to simplify code.

Additional check via test_processor_oneformer.py that the inputs are the same before and after the changes (with fixed random seeds).

@qubvel @molbap

molbap

Thanks for the initiative! Left a couple initial comments

molbap · 2024-11-01T07:40:26Z

Cool! But we should also check that the previous signature works as intended as to not break backwards compatibility - in this case the previous behaviour where arguments were passed positionally should still be supported

I added *args. I had to add it after images, so the signature will only match after another iteration of deprecation. WDYT?

molbap · 2024-11-01T07:42:55Z

since this does more than checking the types, I suggest moving the conversion to list out of this function, and rename it explicitly _validate_input_types for instance.

Implemented.

molbap · 2024-11-01T07:49:59Z

so this tokenizes which should be done with self.tokenizer(..., output_kwargs['text_kwargs']), and it seems that it puts pad tokens where the attention mask is 0 - this should be covered by the tokenizer already, would be a nice refactor

I will look into this...

Atm I don't see how this can be simplified because the function is called with different arguments inside __call__, and it is also called in encode_inputs. Any suggestions?

Agreed that it would be best to use self.tokenizer directly, and pass in the other text kwargs as well. For max_length, you can do the following:

if the max_length kwarg is not set, set it to max_seq_length or task_seq_length in the two different calls.

if it is set, use its value for both task and seq.

I tried this (commit 3c7724d), but I don't see yet how it is simpler/more readable :)
Couple of observations:

the function _preprocess_text is called both with task_seq_length and max_seq_length, so if I add "max_length" to the text_kwargs dict, I would need to add and then replace in __call__.

in order to do something similar to what the attention-mask is doing (padding with 0s), I would need to change self.tokenizer.pad_token_id (and then change it back to what was before, e.g. 49407 (endoftext), because I can't pass in pad_token_id to the tokenizer. I am most probably missing here something, so suggestions welcome.

molbap · 2024-11-01T07:51:41Z

so the way it should be is that in OneFormerProcessorKwargs, these defaults should be passed to the relevant dictionary, which you can then get back. say

_defaults = {"max_seq_length" = 77, "task_seq_length" = 77}

and you can inform the types with a Kwargs typed class

class OneFormerTextKwargs(TextKwargs, total=False): max_seq_length: Optional[int] task_seq_length: Optional[int]

then your types are correctly informed as well as your defaults and you can use both.

This was a bigger refactor. Let me know what you think...

ArthurZucker · 2024-11-19T11:38:51Z

Hey! @molbap is off as he needs to rest, cc @yonigozlan can you review this? 🤗

yonigozlan

Thanks for working on this!
I mainly have comments about the handling of positional kwargs (like in the SAM PR), and about avoiding the wrapping of text processing inside _preprocess_text

yonigozlan · 2024-11-27T16:22:31Z

Agreed that it would be best to use self.tokenizer directly, and pass in the other text kwargs as well. For max_length, you can do the following:

if the max_length kwarg is not set, set it to max_seq_length or task_seq_length in the two different calls.

if it is set, use its value for both task and seq.

yonigozlan · 2024-11-27T16:27:42Z

With this refactoring, it looks to me like encode_inputs could be made the same as call while preserving backward compatibility (if the args are handled correctly), so maybe we could just use call here?

~~Good catch! Done.~~

I tried this in commit 027003465, took some time until I realized that there is a big difference in the two calls:

encode_inputs is calling OneFormerImageProcessor.encode_inputs

__call__ is calling OneFormerImageProcessor.__call__, which will eventually call also OneFormerImageProcessor.encode_inputs, but only after quite some checks and massaging.

So atm I don't think there is a simple and elegant way to replace and would just leave as is. WDYT?

Oh I see, never mind then

yonigozlan · 2024-11-27T16:29:01Z

Yes, see SAM PR comment #34578 (comment)

yonigozlan · 2024-11-27T16:30:13Z

not needed #34578 (comment)

yonigozlan · 2024-11-27T16:31:26Z

These should be added to the optional_call_args attribute (see udop processor)

yonigozlan

There's a breaking change in the init that needs to be fixed. Otherwise it is looking good!
Just like for SAM, you'll also have to add the ProcessorTesterMixin to the processor test class, make sure all the tests pass and override the, if needed, and rebase on main.

yonigozlan · 2024-12-16T15:53:35Z

Oh I see, never mind then

yonigozlan · 2024-12-16T16:00:32Z

Sorry I hadn't caught that before, but removing max_seq_length and task_seq_length from the init looks like a breaking change. We should add them back, and use self.max_seq_length and self.task_seq_length in the processing when the "max_length" kwarg is not defined. No need to add max_seq_length and task_seq_length to OneFormerTextKwargs as they weren't accepted before.

yonigozlan · 2024-12-16T16:03:00Z

I would say the opposite:

Suggested change

max_length=max_length if max_length is not None else text_kwargs.get("max_length", 77),

max_length=text_kwargs.get("max_length") if text_kwargs.get("max_length") is not None else max_length,

So that the max_length kwarg overrides the default kwarg when it is specified

yonigozlan · 2024-12-16T16:03:23Z

No need for that (see comments after)

yonigozlan · 2024-12-16T16:05:25Z

No need to redefine the defaults kwargs here in the get, as if they are not specified, they will necessarily be "max_length" and True respectively, you can just have:

Suggested change

padding=text_kwargs.get("padding", "max_length"),

truncation=text_kwargs.get("truncation", True),

padding=text_kwargs.get("padding"),

truncation=text_kwargs.get("truncation"),

I believe this is needed because encode_inputs does not pass in any dict, so text_kwargs will become {}, without default.

Oh yes I see

what is weird for me here is taht we don't use all the textkwargs. Why not just pass **text_kwargs? They can be typed as well (maybe just tokenizer kwargs) and you just update them with the args passed!

yonigozlan · 2024-12-16T16:06:07Z

Suggested change

max_length=output_kwargs["text_kwargs"]["task_seq_length"],

max_length=self.task_seq_length,

yonigozlan · 2024-12-16T16:06:35Z

Suggested change

max_length=output_kwargs["text_kwargs"]["max_seq_length"],

max_length=self.max_seq_length,

tibor-reiss · 2024-12-19T19:01:20Z

@yonigozlan I had to repeat some of the tests, e.g. test_structured_kwargs_nested, due to the mandatory task_inputs. It was either this, or making a default for task_inputs, e.g. 'semantic'. I went with the former, because it might be surprising for the users that the 'semantic' is picked as default - currently it raises a ValueError if not specified.

yonigozlan

LGTM after putting the kwargs back in the init! Thanks for iterating on this :)

yonigozlan · 2024-12-20T19:36:44Z

+        image_processor=None,
+        tokenizer=None,
+        max_seq_length: int = 77,
+        task_seq_length: int = 77,


Nit removing the **kwargs here would also be a breaking change, even if it's not used anywhere.

yonigozlan · 2024-12-20T19:40:58Z

@yonigozlan I had to repeat some of the tests, e.g. test_structured_kwargs_nested, due to the mandatory task_inputs. It was either this, or making a default for task_inputs, e.g. 'semantic'. I went with the former, because it might be surprising for the users that the 'semantic' is picked as default - currently it raises a ValueError if not specified.

Yes I think that's the best way to do it :)

HuggingFaceDocBuilderDev · 2024-12-23T18:39:16Z

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.

ArthurZucker

Thanks, not sure I get why encode inputs does not pass kwargs, while call does !

ArthurZucker · 2025-01-06T16:41:56Z

what is weird for me here is taht we don't use all the textkwargs. Why not just pass **text_kwargs? They can be typed as well (maybe just tokenizer kwargs) and you just update them with the args passed!

ArthurZucker · 2025-01-06T16:50:08Z

+            text_inputs = [
+                self._preprocess_text(
+                    texts,
+                    max_length=self.max_seq_length,
+                    text_kwargs=output_kwargs["text_kwargs"],
+                ).unsqueeze(0)
+                for texts in encoded_inputs.text_inputs


let's od a for loop it's more redable. Also this is now almost exactly the same as encode_inputs why don't we call that funciton instead / merge both?

Added the for loop

tibor-reiss · 2025-01-14T08:45:34Z

Hi @ArthurZucker,

Thanks, not sure I get why encode inputs does not pass kwargs, while call does !
I don't understand this comment. encode_inputs has kwargs, and is passed e.g. here: encoded_inputs = self.image_processor.encode_inputs(images, task_inputs, segmentation_maps, **kwargs). Could you please clarify?

Regarding _preprocess_text and text_kwargs: I rewrote it a little bit differently, let me know if it's more readable. The difficulty comes from several things, namely:

encode_inputs is similar, but not the same as __call__, see also uniformize kwargs for OneFormer #34547 (comment)
max_length can overwrite self.max_seq_length and self.task_seq_length
backwards compatibility
I think once we remove encode_inputs, the whole code gets simpler and can be again refactored.

ArthurZucker · 2025-01-27T12:30:56Z

+        if not all(task in ["semantic", "instance", "panoptic"] for task in task_inputs):
+            raise ValueError("task_inputs must be semantic, instance, or panoptic.")


typing with choice should allow us not to have to do this

ArthurZucker · 2025-01-27T12:31:32Z

+        if not isinstance(task_inputs, List) or not task_inputs:
+            raise TypeError("task_inputs should be a string or a list of strings.")


typing is enough IMO we should not do this

ArthurZucker · 2025-01-27T12:32:06Z

+        if task_inputs is None:
+            raise ValueError("You have to specify the task_inputs. Found None.")
+        elif images is None:
+            raise ValueError("You have to specify the images. Found None.")


this would be the only thing left and we can do something like if task_inputs ^ images: you have to specifiy one of task_inputs or images

Both are needed, so I have put it into one line. wdyt?

ArthurZucker · 2025-01-27T12:35:13Z

-        if hasattr(encoded_inputs, "text_inputs"):
-            texts_list = encoded_inputs.text_inputs
+        task_token_inputs = []
+        for task in task_inputs:


if task_inputs is None this will fail

Yes, would fail. But it is already caught in _validate_input_types before.

ArthurZucker · 2025-01-27T12:37:46Z

-
+            for texts in encoded_inputs.text_inputs:
+                text_inputs.append(
+                    self._preprocess_text(


might be worth updating _preprocess_text because the tokenizer supports directly passing batches of text.
Also, task_inputs are written all the time so there is no case when this is not run no?

Sorry, but I don't understand neither part of the comment here.

This part is only run if text_inputs is part of encoded_inputs - see the if two lines before. Not connected to task_inputs.

Once above is clarified, could you please specify in more detail how to update _preprocess_text? I have the feeling that this will again become simpler once we drop encode_inputs...

_preprocess_text could only be called once, as the tokenizer.__call__ fonction supports list of list of text

tibor-reiss · 2025-04-27T07:13:04Z

Rebased

qubvel · 2025-05-05T18:28:46Z

Hey @tibor-reiss, please run make modified_only_fixup to fix style issues and make CI green, thank you!

qubvel · 2025-05-08T09:47:57Z

run-slow: oneformer

github-actions · 2025-05-08T09:49:21Z

This comment contains run-slow, running the specified jobs: This comment contains run-slow, running the specified jobs:

models: ['models/oneformer']
quantizations: [] ...

tibor-reiss force-pushed the fix-31811-oneformer branch 2 times, most recently from cd15539 to fad4111 Compare October 31, 2024 21:12

molbap reviewed Nov 1, 2024

View reviewed changes

qubvel added Vision Multimodal Processing labels Nov 1, 2024

qubvel mentioned this pull request Nov 2, 2024

Uniform kwargs for processors #31911

Closed

40 tasks

tibor-reiss requested a review from molbap November 19, 2024 10:18

ArthurZucker requested a review from yonigozlan November 19, 2024 11:38

yonigozlan reviewed Nov 27, 2024

View reviewed changes

tibor-reiss force-pushed the fix-31811-oneformer branch 2 times, most recently from 40cf888 to cbddaf4 Compare November 29, 2024 09:58

tibor-reiss requested a review from yonigozlan December 13, 2024 21:43

yonigozlan reviewed Dec 16, 2024

View reviewed changes

tibor-reiss force-pushed the fix-31811-oneformer branch 2 times, most recently from 9ccaa40 to b579834 Compare December 19, 2024 18:51

tibor-reiss requested a review from yonigozlan December 19, 2024 18:58

yonigozlan reviewed Dec 20, 2024

View reviewed changes

yonigozlan requested a review from ArthurZucker December 20, 2024 19:40

yonigozlan approved these changes Dec 23, 2024

View reviewed changes

ArthurZucker approved these changes Jan 6, 2025

View reviewed changes

tibor-reiss requested review from Rocketknight1 and qubvel as code owners January 14, 2025 08:38

tibor-reiss requested a review from ArthurZucker January 14, 2025 08:46

ArthurZucker reviewed Jan 27, 2025

View reviewed changes

tibor-reiss added 16 commits April 27, 2025 08:42

Nitpick

fe9f0a4

Make kwargs uniform for OneFormer

53bf005

Add support for backwards compatibility

b855650

Better validation

1a61dbd

Refactor max_seq_length and task_seq_length

036b152

Linting

ce50fae

Code reivew - use existing methods

79e4c22

Order matters - task_inputs has to be first

896c278

Pass in text_kwargs

410c686

Add back max_seq_length and task_seq_length

ebd9da0

Add ProcessorTesterMixin

21a593d

Fix tests

0e33655

Put back kwargs in init

e26e9fd

Kwargs

c867029

Add for loop

155a008

Simplify error handling by relying on typing

77601ed

tibor-reiss force-pushed the fix-31811-oneformer branch from 0f7b044 to 77601ed Compare April 27, 2025 07:12

tibor-reiss requested a review from ArthurZucker April 27, 2025 07:13

Lint

8bd1e54

ArthurZucker removed their request for review July 7, 2025 11:57

	max_length=max_length if max_length is not None else text_kwargs.get("max_length", 77),
	max_length=text_kwargs.get("max_length") if text_kwargs.get("max_length") is not None else max_length,

-            padding=text_kwargs.get("padding", "max_length"),
-            truncation=text_kwargs.get("truncation", True),
+            padding=text_kwargs.get("padding"),
+            truncation=text_kwargs.get("truncation"),

	max_length=output_kwargs["text_kwargs"]["task_seq_length"],
	max_length=self.task_seq_length,

	max_length=output_kwargs["text_kwargs"]["max_seq_length"],
	max_length=self.max_seq_length,

		if not all(task in ["semantic", "instance", "panoptic"] for task in task_inputs):
		raise ValueError("task_inputs must be semantic, instance, or panoptic.")

		if not isinstance(task_inputs, List) or not task_inputs:
		raise TypeError("task_inputs should be a string or a list of strings.")

Uh oh!

Conversation

tibor-reiss commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

molbap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibor-reiss Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurZucker commented Nov 19, 2024

Uh oh!

yonigozlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibor-reiss Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonigozlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibor-reiss commented Dec 19, 2024

Uh oh!

yonigozlan left a comment

tibor-reiss commented Oct 31, 2024 •

edited

Loading

tibor-reiss Nov 1, 2024 •

edited

Loading

tibor-reiss Nov 29, 2024 •

edited

Loading