Make `pipeline` able to load `processor` #32514

qubvel · 2024-08-07T21:01:52Z

What does this PR do?

Add processor to the pipeline and refactor tests a bit to support it. Related to

Fixes # (issue)

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

HuggingFaceDocBuilderDev · 2024-08-07T21:21:10Z

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.

qubvel

The main changes for pipeline code:

Added loading of processor (previously only tokenizer, image_processor and feature_extractor)
Added Pipeline class args to control loading

The main changes

Refactored test signatures to support processor pass
Changed the way how class names are obtained for processors of the testing tiny models in a pipeline. Instead of getting them from JSON file, the MAPPINGs are used.

I highlighted these changes below, please see the comments

cc @ydshieh for initial review

qubvel · 2024-08-09T12:16:49Z

src/transformers/pipelines/__init__.py

+    # Check that pipeline class required loading
+    load_tokenizer = load_tokenizer and pipeline_class._load_tokenizer
+    load_feature_extractor = load_feature_extractor and pipeline_class._load_feature_extractor
+    load_image_processor = load_image_processor and pipeline_class._load_image_processor
+    load_processor = load_processor and pipeline_class._load_processor


For backward compatibility, we can control with Pipeline class if we need to load specific processors/tokenizers. For example, for zero-shot object detection, we will need to load only the processor, and do not need to load image_processor and tokenizer separately. Other legacy pipelines might load only tokenizer and image_processor, even if they have processor class.

qubvel · 2024-08-09T12:18:39Z

src/transformers/pipelines/base.py

+    # Previously, pipelines support only `tokenizer`, `feature_extractor`, and `image_processor`.
+    # As we start adding `processor`, we want to avoid loading processor for some pipelines, that don't required it,
+    # because, for example, use `image_processor` and `tokenizer` separately.
+    # However, we want to enable it for new pipelines. Moreover, this allow us to granularly control loading components
+    # and avoid loading tokenizer/image_processor/feature_extractor twice: once as a separate object
+    # and once in the processor. The following flags a set this way for backward compatibility ans might be overridden
+    # in specific Pipeline class.
+    _load_processor = False
+    _load_image_processor = True
+    _load_feature_extractor = True
+    _load_tokenizer = True


granular control for loading, see comment in the code

qubvel · 2024-08-09T12:21:11Z

tests/models/mt5/test_modeling_mt5.py

+    def is_pipeline_test_to_skip(
+        self,
+        pipeline_test_case_name,
+        config_class,
+        model_architecture,
+        tokenizer_name,
+        image_processor_name,
+        feature_extractor_name,
+        processor_name,
+    ):
+        if tokenizer_name is None:
+            return True
+
+        # `MT5EncoderOnlyModelTest` is not working well with slow tokenizers (for some models) and we don't want to touch the file
+        # `src/transformers/data/processors/squad.py` (where this test fails for this model)
+        if pipeline_test_case_name == "TokenClassificationPipelineTests" and not tokenizer_name.endswith("Fast"):
+            return True
+
+        return False
+


This one is skipped on the main but was not skipped on this branch, so I added a skip rule here

This one is skipped on the main

Could you explain a bit what this means?

Do you mean, in the job run's result, it shows it is skipped (on the main branch)?

Yes, this test was not running on the main branch. One of the problem of current tests: we are testing a set of cases, and if one of the cases (e.g. the first one) falls under skipTest(...) the rest test cases are not running. Probably it's better to return case status, and then aggregate statuses in the main test function running on multiple cases.

A simple example to reproduce, this test will be skipped

import unittest class TestCases(unittest.TestCase): def test_multiple_cases(self): for i in range(10): self.run_test(i) def run_test(self, i): if i == 0: self.skipTest("Skip this test") elif i == 5: raise ValueError("This test failed")

qubvel · 2024-08-09T12:28:15Z

tests/pipelines/test_pipelines_text_generation.py

@@ -413,6 +431,7 @@ def run_pipeline_test(self, text_generator, _):
            "XGLMForCausalLM",
            "GPTNeoXForCausalLM",
            "FuyuForCausalLM",
+            "LlamaForCausalLM",


Llama no longer raises exception for long generation, so I added it to exclude list in the test. This test was skipped on main.

qubvel · 2024-08-09T12:31:34Z

tests/test_pipeline_mixin.py

+            # tokenizers are mapped to tuple, e.g. ("XxxTokenizer", None)
+            tokenizer_names = TOKENIZER_MAPPING_NAMES.get(model_type, [None])
+            if len(tokenizer_names) > 1:
+                tokenizer_names = [name for name in tokenizer_names if name is not None]
+
+            # image processors are mapped to tuple, e.g. ("XxxImageProcessor", None)
+            image_processor_names = IMAGE_PROCESSOR_MAPPING_NAMES.get(model_type, [None])
+            if len(image_processor_names) > 1:
+                image_processor_names = [name for name in image_processor_names if name is not None]
+
+            # feature extractors are mapped to instance, e.g. "XxxFeatureExtractor"
+            feature_extractor_names = FEATURE_EXTRACTOR_MAPPING_NAMES.get(model_type, [None])
+            if not isinstance(feature_extractor_names, (list, tuple)):
+                feature_extractor_names = [feature_extractor_names]
+
+            # processors are mapped to instance, e.g. "XxxProcessor"
+            processor_names = PROCESSOR_MAPPING_NAMES.get(model_type, [None])
+            if not isinstance(processor_names, (list, tuple)):
+                processor_names = [processor_names]
+


Previously, tokenizer, image processor and feature extractor class names were taken from JSON file of tiny models, however, there is no *Processor class there. Now MAPPING lists are used to get class names.

I am not in favor of using MAPPING for anything except processor.

The pipeline testing is highly tighten to the tiny model on the Hub, and we should use the actual classes that are used on the Hub repositories (to avoid any surprising).

For processor, it's OK to use MAPPING, but we can also change the json file's structure for the long term.

I did it because in the original tiny-models JSON file we do not have separate lists for image processors and feature extractors, I found they are stored in the same list (even processors are sometimes in this list).

Modified in b6d3bbc

according to @ydshieh suggestion to use mapping only for Processor

qubvel · 2024-08-09T12:36:11Z

tests/test_pipeline_mixin.py

+        # TODO: We should check if a model file is on the Hub repo. instead.
+        try:
+            model = model_architecture.from_pretrained(repo_id, revision=commit)
+        except Exception:
+            logger.warning(
+                f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: Could not find or load "
+                f"the model from `{repo_id}` with `{model_architecture}`."
+            )
+            self.skipTest(f"Could not find or load the model from {repo_id} with {model_architecture}.")
+
+        # -------------------- Load tokenizer --------------------
+


The order of loading is a bit changed in this test, now the model is loaded first to check if it exists, otherwise, the test is skipped.

ydshieh · 2024-08-13T15:00:39Z

Hi @qubvel . IIRC, that PR is more about "make pipeline able to accept loading processor", but no currently existing pipeline code is changed to use processor yet (even if we specify to load it), right?

qubvel · 2024-08-13T15:22:49Z

@ydshieh yes, you are right, the following PR will add processor to the ZeroShotObjectDetection pipeline. I decided to split it to simplify the review a bit.

ydshieh · 2024-08-14T05:36:56Z

Hi @qubvel I have a general question. The process class could load also a single tokenizer, image processor or feature extractor (if not all components are on a Hub repository). For example,

from transformers import AutoProcessor
AutoProcessor.from_pretrained("bert-base-uncased")

will turn out to be a tokenizer instead of an instance of ProcessorMixin.

Therefore, in src/transformers/pipelines/__init__.py, this part

       # Instantiate processor if needed
        if isinstance(processor, (str, tuple)):
            processor = AutoProcessor.from_pretrained(processor, _from_pipeline=task, **hub_kwargs, **model_kwargs)

should have some guard to make sure we really get a ProcessorMixin instance.

WDYT?

I will share my thoughts regarding the testing part this afternoon, somehow related to the same consideration.

ydshieh · 2024-08-14T14:31:31Z

continuation of my previous comment

Once we start to have the code using processor, and if we support it for some currently existing pipelines, it should support:

either not use the processor at all (i.e. currently implementation): for backward compatibility
or completely use the processor only (but need to verify the results are the same)

for testing

Similar consideration to the above. We should test 2 cases:

only load a list of tokenizer, image processor and/or feature extractor: test case 1
only load a single processor: test case 2

ydshieh

Some comments :-)

ydshieh · 2024-08-13T15:49:04Z

tests/models/mt5/test_modeling_mt5.py

+    def is_pipeline_test_to_skip(
+        self,
+        pipeline_test_case_name,
+        config_class,
+        model_architecture,
+        tokenizer_name,
+        image_processor_name,
+        feature_extractor_name,
+        processor_name,
+    ):
+        if tokenizer_name is None:
+            return True
+
+        # `MT5EncoderOnlyModelTest` is not working well with slow tokenizers (for some models) and we don't want to touch the file
+        # `src/transformers/data/processors/squad.py` (where this test fails for this model)
+        if pipeline_test_case_name == "TokenClassificationPipelineTests" and not tokenizer_name.endswith("Fast"):
+            return True
+
+        return False
+


This one is skipped on the main

Could you explain a bit what this means?

ydshieh · 2024-08-13T15:51:51Z

tests/models/mt5/test_modeling_mt5.py

+    def is_pipeline_test_to_skip(
+        self,
+        pipeline_test_case_name,
+        config_class,
+        model_architecture,
+        tokenizer_name,
+        image_processor_name,
+        feature_extractor_name,
+        processor_name,
+    ):
+        if tokenizer_name is None:
+            return True
+
+        # `MT5EncoderOnlyModelTest` is not working well with slow tokenizers (for some models) and we don't want to touch the file
+        # `src/transformers/data/processors/squad.py` (where this test fails for this model)
+        if pipeline_test_case_name == "TokenClassificationPipelineTests" and not tokenizer_name.endswith("Fast"):
+            return True
+
+        return False
+


Do you mean, in the job run's result, it shows it is skipped (on the main branch)?

ydshieh · 2024-08-13T15:59:35Z

tests/test_pipeline_mixin.py

+            # tokenizers are mapped to tuple, e.g. ("XxxTokenizer", None)
+            tokenizer_names = TOKENIZER_MAPPING_NAMES.get(model_type, [None])
+            if len(tokenizer_names) > 1:
+                tokenizer_names = [name for name in tokenizer_names if name is not None]
+
+            # image processors are mapped to tuple, e.g. ("XxxImageProcessor", None)
+            image_processor_names = IMAGE_PROCESSOR_MAPPING_NAMES.get(model_type, [None])
+            if len(image_processor_names) > 1:
+                image_processor_names = [name for name in image_processor_names if name is not None]
+
+            # feature extractors are mapped to instance, e.g. "XxxFeatureExtractor"
+            feature_extractor_names = FEATURE_EXTRACTOR_MAPPING_NAMES.get(model_type, [None])
+            if not isinstance(feature_extractor_names, (list, tuple)):
+                feature_extractor_names = [feature_extractor_names]
+
+            # processors are mapped to instance, e.g. "XxxProcessor"
+            processor_names = PROCESSOR_MAPPING_NAMES.get(model_type, [None])
+            if not isinstance(processor_names, (list, tuple)):
+                processor_names = [processor_names]
+


I am not in favor of using MAPPING for anything except processor.

The pipeline testing is highly tighten to the tiny model on the Hub, and we should use the actual classes that are used on the Hub repositories (to avoid any surprising).

For processor, it's OK to use MAPPING, but we can also change the json file's structure for the long term.

ydshieh · 2024-08-13T16:18:56Z

tests/test_pipeline_mixin.py

                )
-                self.skipTest(f"Could not load the processor from {repo_id} with {processor_name}.")
+                self.skipTest(f"Could not load the tokenizer from {repo_id} with {tokenizer_name}.")


should not skip here.

Addressed in 8e3eb2b

ydshieh · 2024-08-13T16:19:07Z

tests/test_pipeline_mixin.py

+                        f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: "
+                        f"Could not load the {key} from `{repo_id}` with `{name}`."
+                    )
+                    self.skipTest(f"Could not load the {key} from {repo_id} with {name}.")


should not skip here

It is designed the same way on the main now, according to the comment some instances might not be loaded due to optional dependencies

transformers/tests/test_pipeline_mixin.py

Lines 256 to 267 in af638c4

processor = None

if processor_name is not None:

processor_class = getattr(transformers_module, processor_name)

# If the required packages (like `Pillow` or `torchaudio`) are not installed, this will fail.

try:

processor = processor_class.from_pretrained(repo_id, revision=commit)

except Exception:

logger.warning(

f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: Could not load the "

f"processor from `{repo_id}` with `{processor_name}`."

)

self.skipTest(f"Could not load the processor from {repo_id} with {processor_name}.")

Currently on main, this run_pipeline_test only take a single tokenizer_name and a single processor_name, and try to load them. So if a specified processor_name could not be loaded, we can skip it.

However, here in this PR, we try to collect the processors and put them into the dictionary processors. So logically it's better not to skip the test during this collection.

(But in practice, this may not produce any difference)

tests/test_pipeline_mixin.py

ydshieh · 2024-08-14T16:29:33Z

I did it because in the original tiny-models JSON file we do not have separate lists for image processors and feature extractors, I found they are stored in the same list (even processors are sometimes in this list).

Yes, you are correct. It is because I treated them (image processor / feature exactor) equally, and later I decided to make processor join into this pipeline testing

I see better now why you need this change. Could you, at least for now, simply use the information from the summary file processor_classes field, and by looking at training part to decide to assign them to one of the tokenizer_names, feature_extractor_names, processor_names etc.?
``

qubvel · 2024-08-22T13:06:34Z

@ydshieh Thanks for reviewing, I addressed the comments, please have a look!

Once we start to have the code using processor, and if we support it for some currently existing pipelines, it should support:

either not use the processor at all (i.e. currently implementation): for backward compatibility

or completely use the processor only (but need to verify the results are the same)

This is what I tried to achieve with granular control for each pipeline class by adding _load_* attributes
#32514 (comment)

qubvel · 2024-08-27T09:48:32Z

@ydshieh could you please have a look once again?

ydshieh

Just a few nits but overall good!

Let me know if my comments are clear 🙏

ydshieh · 2024-08-27T14:06:14Z

tests/test_pipeline_mixin.py

+                        f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: "
+                        f"Could not load the {key} from `{repo_id}` with `{name}`."
+                    )
+                    self.skipTest(f"Could not load the {key} from {repo_id} with {name}.")


Currently on main, this run_pipeline_test only take a single tokenizer_name and a single processor_name, and try to load them. So if a specified processor_name could not be loaded, we can skip it.

However, here in this PR, we try to collect the processors and put them into the dictionary processors. So logically it's better not to skip the test during this collection.

(But in practice, this may not produce any difference)

ydshieh · 2024-08-27T14:12:06Z

tests/test_pipeline_mixin.py

@@ -236,67 +324,82 @@ def run_pipeline_test(
                A subclass of `PretrainedModel` or `PretrainedModel`.
            tokenizer_name (`str`):
                The name of a subclass of `PreTrainedTokenizerFast` or `PreTrainedTokenizer`.
-            processor_name (`str`):
+            image_processor_or_feature_extractor_name (`str`):


This looks like not the correct docstring?

tests/test_pipeline_mixin.py

ydshieh · 2024-08-27T14:36:55Z

tests/test_pipeline_mixin.py

-            # Adding `None` (if empty) so we can generate tests
-            tokenizer_names = [None] if len(tokenizer_names) == 0 else tokenizer_names
-            processor_names = [None] if len(processor_names) == 0 else processor_names
+            if model_arch_name in tiny_model_summary and "sha" in tiny_model_summary[model_arch_name]:


the first condition is not necessary at this point.

ydshieh · 2024-08-27T14:47:21Z

tests/test_pipeline_mixin.py

+            tokenizer_names = tiny_model_summary[model_arch_name]["tokenizer_classes"]
+
+            # Sort image processors and feature extractors from tiny-models json file
+            image_processor_names = []
+            feature_extractor_names = []


if we don't add None later in this method (while it's empty), then in run_model_pipeline_tests,

test_cases = [ ....]

will be empty if tokenizer_names is empty or image_processor_names is empty or ... (etc).

But we should allow the test case to exist even if only one of them present: for example, a text only model or a vision only model.

yonigozlan · 2024-09-16T18:57:43Z

Hi @qubvel @ydshieh ! Just wondering if anything is blocking this PR from getting merged, as it is needed for the image-text-to-text pipeline. Thanks!

qubvel · 2024-09-17T11:44:46Z

Hi @yonigozlan, just a lack of time 🙂 I will address comments this week, and hopefully it can be merged then!

LysandreJik · 2024-09-17T18:48:43Z

cc @Rocketknight1 as well (please always ping Matt for pipelines 🤗)

qubvel added 2 commits August 7, 2024 20:48

Refactor get_test_pipeline

7c53ae2

Fixup

cf5bbb6

qubvel marked this pull request as draft August 7, 2024 21:02

qubvel added 18 commits August 7, 2024 21:21

Fixing tests

b5643e4

Add processor loading in tests

bc9cbd6

Restructure processors loading

998ffb3

Add processor to the pipeline

f658d5e

Move model loading on tom of the test

699194f

Update get_test_pipeline

99822ef

Fixup

b28d962

Add class-based flags for loading processors

6995bc8

Change is_pipeline_test_to_skip signature

bb30380

Skip t5 failing test for slow tokenizer

d658e77

Fixup

e6f3912

Fix copies for T5

75e7e43

Fix typo

5b096e3

Add try/except for tokenizer loading (kosmos-2 case)

aa2dcd2

Fixup

a62cf86

Llama not fails for long generation

6d438d0

Revert processor pass in text-generation test

5502f3b

Fix docs

ad7cd28

qubvel commented Aug 9, 2024

View reviewed changes

qubvel changed the title ~~Tests refactoring for processor in pipeline~~ Add processor to the pipeline Aug 9, 2024

qubvel marked this pull request as ready for review August 9, 2024 13:00

ydshieh self-requested a review August 13, 2024 12:41

ydshieh self-assigned this Aug 13, 2024

qubvel changed the title ~~Add processor to the pipeline~~ Make pipeline able to load processor Aug 13, 2024

ydshieh reviewed Aug 14, 2024

View reviewed changes

ydshieh mentioned this pull request Aug 14, 2024

Unused variable in test_pipelines_text_generation.py function #32397

Open

qubvel added 3 commits August 22, 2024 11:47

Switch back to json file for image processors and feature extractors

b6d3bbc

Add processor type check

2f56147

Remove except for tokenizers

8e3eb2b

ydshieh reviewed Aug 27, 2024

View reviewed changes

amyeroberts mentioned this pull request Aug 29, 2024

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

Open

ydshieh assigned Rocketknight1 Sep 19, 2024

	processor = None
	if processor_name is not None:
	processor_class = getattr(transformers_module, processor_name)
	# If the required packages (like `Pillow` or `torchaudio`) are not installed, this will fail.
	try:
	processor = processor_class.from_pretrained(repo_id, revision=commit)
	except Exception:
	logger.warning(
	f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: Could not load the "
	f"processor from `{repo_id}` with `{processor_name}`."
	)
	self.skipTest(f"Could not load the processor from {repo_id} with {processor_name}.")

Make pipeline able to load processor #32514

Are you sure you want to change the base?

Make pipeline able to load processor #32514

Conversation

qubvel commented Aug 7, 2024 • edited Loading

What does this PR do?

Before submitting

Who can review?

HuggingFaceDocBuilderDev commented Aug 7, 2024

qubvel left a comment

Choose a reason for hiding this comment

qubvel Aug 9, 2024 • edited Loading

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

qubvel Aug 14, 2024 • edited Loading

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

qubvel Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydshieh commented Aug 13, 2024 • edited Loading

qubvel commented Aug 13, 2024 • edited Loading

ydshieh commented Aug 14, 2024

ydshieh commented Aug 14, 2024

continuation of my previous comment

for testing

ydshieh 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

ydshieh commented Aug 14, 2024

qubvel commented Aug 22, 2024

qubvel commented Aug 27, 2024

ydshieh 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

yonigozlan commented Sep 16, 2024

qubvel commented Sep 17, 2024

LysandreJik commented Sep 17, 2024

Make `pipeline` able to load `processor` #32514

Make `pipeline` able to load `processor` #32514

qubvel commented Aug 7, 2024 •

edited

Loading

qubvel Aug 9, 2024 •

edited

Loading

qubvel Aug 14, 2024 •

edited

Loading

qubvel Aug 22, 2024 •

edited

Loading

ydshieh commented Aug 13, 2024 •

edited

Loading

qubvel commented Aug 13, 2024 •

edited

Loading