uniformize kwargs for SAM #34578

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

src/transformers/models/sam/processing_sam.py

+        *args,  # to be deprecated
+        text: Union[TextInput, PreTokenizedInput, List[TextInput], List[PreTokenizedInput]] = None,
+        audio=None,
+        video=None,


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

src/transformers/models/sam/processing_sam.py

@@ -33,6 +34,23 @@
    import tensorflow as tf


+class SamImagesKwargs(ImagesKwargs):
+    segmentation_maps: Optional[ImageInput]


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

src/transformers/models/sam/processing_sam.py

-        input_boxes=None,
-        return_tensors: Optional[Union[str, TensorType]] = None,
+        images: ImageInput = None,
+        *args,  # to be deprecated


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

src/transformers/models/sam/processing_sam.py

-        segmentation_maps=None,
-        input_points=None,
-        input_labels=None,
-        input_boxes=None,


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

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

src/transformers/models/sam/processing_sam.py

+    @staticmethod
+    def _add_args_for_backward_compatibility(args):
+        """
+        Remove this function once support for args is dropped in __call__
+        """
+        if len(args) > 4:
+            raise ValueError("Too many positional arguments")
+        return dict(zip(("segmentation_maps", "input_points", "input_labels", "input_boxes"), args))
+


This won't be needed as we now have prepare_and_validate_optional_call_args

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

src/transformers/models/sam/processing_sam.py

-        input_boxes=None,
-        return_tensors: Optional[Union[str, TensorType]] = None,
+        images: Optional[ImageInput] = None,
+        *args,  # to be deprecated


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

src/transformers/models/sam/processing_sam.py

+            SamProcessorKwargs,
+            tokenizer_init_kwargs={},
+            **kwargs,
+            **self._add_args_for_backward_compatibility(args),


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

tests/models/sam/test_processor_sam.py

@@ -152,7 +164,9 @@ def test_post_process_masks(self):

 @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

tests/models/sam/test_processor_sam.py

@@ -235,7 +259,9 @@ def test_post_process_masks(self):

 @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

Open

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
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uniformize kwargs for SAM #34578

uniformize kwargs for SAM #34578

tibor-reiss commented Nov 2, 2024

SangbumChoi left a comment

SangbumChoi Nov 3, 2024

tibor-reiss Nov 4, 2024

SangbumChoi Nov 5, 2024

SangbumChoi Nov 3, 2024

tibor-reiss Nov 4, 2024

yonigozlan Nov 27, 2024

SangbumChoi Nov 3, 2024

tibor-reiss Nov 4, 2024

ArthurZucker commented Nov 19, 2024

yonigozlan left a comment

yonigozlan Nov 27, 2024

yonigozlan Nov 27, 2024

yonigozlan Nov 27, 2024

yonigozlan Nov 27, 2024

tibor-reiss commented Nov 29, 2024

tibor-reiss commented Dec 13, 2024

yonigozlan left a comment

yonigozlan left a comment

yonigozlan Dec 19, 2024

tibor-reiss Dec 19, 2024

yonigozlan Dec 19, 2024

yonigozlan commented Dec 19, 2024

HuggingFaceDocBuilderDev commented Dec 19, 2024

ArthurZucker left a comment

-        *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),

uniformize kwargs for SAM #34578

uniformize kwargs for SAM #34578

Conversation

tibor-reiss commented Nov 2, 2024

SangbumChoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurZucker commented Nov 19, 2024

yonigozlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tibor-reiss commented Nov 29, 2024

tibor-reiss commented Dec 13, 2024

yonigozlan left a comment

Choose a reason for hiding this comment

yonigozlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonigozlan commented Dec 19, 2024

HuggingFaceDocBuilderDev commented Dec 19, 2024

ArthurZucker left a comment

Choose a reason for hiding this comment