uniformize kwargs for SAM by tibor-reiss · Pull Request #34578 · huggingface/transformers

tibor-reiss · 2024-11-02T09:40:28Z

Adds uniformized processors for SAM following #31911.

SangbumChoi

I appreciate this PR since I am working on SAM2. It seems nice :)

SangbumChoi · 2024-11-03T23:56:23Z

Can we also define the input time in audio and video?

I don't understand what you mean here, could you please be more specific?

The signature of __call__ is given, it should not be extended as far as I understand. However, it can be added as part of VideosKwargs and/or AudioKwargs. Can you point me where this attribute/variable is defined?

*time -> *type, I think it is okay since this does not requires audio and video

SangbumChoi · 2024-11-03T23:57:15Z

I think it this segmentation_maps is not required for the inference of SAM.

This is needed for backwards compatibility - even if it is not used. E.g. take the following call: processor(images, None, input_points). If I would remove it from _add_args_for_backward_compatibility, such calls would break.
If I understood correctly, #31911 is about uniform kwargs, and then at a later step the API would be cleaned up further. In SAM's case, _add_args_for_backward_compatibility will be removed, and then segmentation_maps can be removed as well.

We actually have an existing way to handle such positional args, you can take a look at udop processor for example :)

SangbumChoi · 2024-11-03T23:58:06Z

Is there reason for the existance of this *args?

Yes, see comment above. Purely for bc and should be removed at a later stage.

ArthurZucker · 2024-11-19T11:40:02Z

cc @yonigozlan can you take a look!

yonigozlan

Thanks a lot for working on this!
Looks great to me overall, only thing is to use the existing methods to handle positional args for backward compatibility.

On a separate note, and I know this precedes this PR, but it looks to me that there is only an image processor in this processor? Why was all this logic created as a processor at all and not just added to an image processor?

If anyone has any explanation for that, I’d really appreciate it. :)

yonigozlan · 2024-11-27T01:44:58Z

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

yonigozlan · 2024-11-27T01:45:48Z

This won't be needed as we now have prepare_and_validate_optional_call_args

yonigozlan · 2024-11-27T01:48:40Z

Suggested change

*args, # to be deprecated

# The following is to capture `segmentation_maps`, `input_points`, `input_labels` and `input_boxes` arguments that may be passed as a positional argument.

# See transformers.processing_utils.ProcessorMixin.prepare_and_validate_optional_call_args for more details,

# or this conversation for more context: https://github.com/huggingface/transformers/pull/32544#discussion_r1720208116

# This behavior is only needed for backward compatibility and will be removed in future versions.

#

*args,

yonigozlan · 2024-11-27T01:49:30Z

Suggested change

**self._add_args_for_backward_compatibility(args),

**self.prepare_and_validate_optional_call_args(*args),

tibor-reiss · 2024-11-29T06:40:54Z

@yonigozlan Thanks for the review, I have implemented your suggestions. Could you check again please?

tibor-reiss · 2024-12-13T21:44:19Z

Friendly reminder @yonigozlan

yonigozlan

Looks good to me, thanks for iterating!
Last step is to add the ProcessorTesterMixin to SamProcessorTest in test_processor_sam.py, and make sure all the tests pass. Since the processor really only process images, there will probably be quite a few tests to override.
And also you'' have to rebase on main!

yonigozlan

Left two small comments and after these modifications LGTM! Thanks for working on this

yonigozlan · 2024-12-19T17:11:05Z

 @require_vision
 @require_tf
-class TFSamProcessorTest(unittest.TestCase):
+class TFSamProcessorTest(ProcessorTesterMixin, unittest.TestCase):


I'd say you only need ProcessorTesterMixin for SamProcessorTest, so you can remove this here and the skipped tests as well

Makes sense. However, I had to then add back the prepare_image_inputs methods. With this, I updated the comment.

yonigozlan · 2024-12-19T17:11:32Z

 @require_vision
 @require_torchvision
-class SamProcessorEquivalenceTest(unittest.TestCase):
+class SamProcessorEquivalenceTest(ProcessorTesterMixin, unittest.TestCase):


yonigozlan · 2024-12-19T17:16:07Z

Pinging @ArthurZucker for a core maintainer review

HuggingFaceDocBuilderDev · 2024-12-19T17:41:27Z

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 @yonigozlan for the reviews! 🤗

tibor-reiss force-pushed the fix-31811-sam branch from 5c2efda to 197e079 Compare November 2, 2024 09:47

qubvel added Vision Multimodal Processing labels Nov 2, 2024

qubvel mentioned this pull request Nov 2, 2024

Uniform kwargs for processors #31911

Closed

40 tasks

SangbumChoi reviewed Nov 3, 2024

View reviewed changes

molbap self-requested a review November 5, 2024 12:49

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

yonigozlan reviewed Nov 27, 2024

View reviewed changes

yonigozlan mentioned this pull request Nov 27, 2024

uniformize kwargs for OneFormer #34547

Open

tibor-reiss requested a review from yonigozlan November 29, 2024 06:40

yonigozlan reviewed Dec 16, 2024

View reviewed changes

tibor-reiss added 5 commits December 19, 2024 13:10

Make kwargs uniform for SAM

c3b52af

Remove unused attribute

ff3c3e9

Make point_pad_value part of image_kwargs

f9c3fe1

Update annotations

3da5ddc

Code review - use existing methods

96dd98f

tibor-reiss force-pushed the fix-31811-sam branch from 67d3d6b to 3dc8b92 Compare December 19, 2024 12:43

Use ProcessorTesterMixin

d92cd4b

tibor-reiss force-pushed the fix-31811-sam branch from 3dc8b92 to d92cd4b Compare December 19, 2024 12:45

tibor-reiss requested a review from yonigozlan December 19, 2024 13:04

yonigozlan reviewed Dec 19, 2024

View reviewed changes

yonigozlan requested a review from ArthurZucker December 19, 2024 17:16

Do not add ProcessorTesterMixin everywhere

6f24d6c

tibor-reiss force-pushed the fix-31811-sam branch from a7c143d to 6f24d6c Compare December 19, 2024 19:02

ArthurZucker approved these changes Dec 23, 2024

View reviewed changes

ArthurZucker merged commit e10be82 into huggingface:main Dec 23, 2024

-        *args,  # to be deprecated
+                # The following is to capture `segmentation_maps`, `input_points`, `input_labels` and `input_boxes` arguments that may be passed as a positional argument.
+        # See transformers.processing_utils.ProcessorMixin.prepare_and_validate_optional_call_args for more details,
+        # or this conversation for more context: https://github.com/huggingface/transformers/pull/32544#discussion_r1720208116
+        # This behavior is only needed for backward compatibility and will be removed in future versions.
+        #
+        *args,

	**self._add_args_for_backward_compatibility(args),
	*self.prepare_and_validate_optional_call_args(args),

Uh oh!

Conversation

tibor-reiss commented Nov 2, 2024

Uh oh!

SangbumChoi 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibor-reiss commented Nov 29, 2024

Uh oh!

tibor-reiss commented Dec 13, 2024

Uh oh!

yonigozlan left a comment

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!

yonigozlan commented Dec 19, 2024

Uh oh!

HuggingFaceDocBuilderDev commented Dec 19, 2024

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants