🔥Rework pipeline testing by removing `PipelineTestCaseMeta` 🚀
Author: ydshieh
Pull request: yes

# What does this PR do?

⚠️ Before merge: We need to add pipeline test classes to `PipelineTesterMixin` and make sure all tests pass. ⚠️

- The pipeline testing now is managed by a new class `PipelineTesterMixin` (similar to `ModelTesterMixin`)
  - no more `metaclass=PipelineTestCaseMeta`
  - In each model testing file (only PyTorch/TensorFlow), we change 
    ```
    class ViTModelTest(ModelTesterMixin, unittest.TestCase)
    ```
    to
    ```
    class ViTModelTest(ModelTesterMixin, PipelineTesterMixin, unittest.TestCase)
    ```
    - There is no need of `TFPipelineTesterMixin`, the framework is automatically detected
  - The directory and files `tests/pipelines/test_pipelines_xxx` and class like `XXXPipelineTests` should exist there:
    - These now contain **only** _non-model-specific_ pipeline tests like `test_small_model_pt`, `test_small_model_tf` etc.
    - Their class attributes `model_mapping` and `tf_model_mapping`, as well as methods `get_test_pipeline` and `run_pipeline_test` are used in `PipelineTesterMixin`:
      - I believe these are better left in current files
      - (if really necessary, a clean-up could be done in a separate PR. I want to keep the scope of current PR minimal) 

## (Preview of the) Rresults

Running the following command
```python
python3 -m pytest --pdb -v tests/models/bert tests/models/vit -k "test_pipeline_text_classification or test_pipeline_image_classification"
```
will give
```bash
tests/models/bert/test_modeling_bert.py::BertModelTest::test_pipeline_image_classification SKIPPED (Test is skipped: No model architecture under framework pt with the configuration class `BertConfig` is found for the task `image-classification`.)                                                                            [ 12%]
tests/models/bert/test_modeling_bert.py::BertModelTest::test_pipeline_text_classification PASSED                                                                                                                                                                                                                                  [ 25%]
tests/models/bert/test_modeling_tf_bert.py::TFBertModelTest::test_pipeline_image_classification SKIPPED (Test is skipped: No model architecture under framework tf with the configuration class `BertConfig` is found for the task `image-classification`.)                                                                       [ 37%]
tests/models/bert/test_modeling_tf_bert.py::TFBertModelTest::test_pipeline_text_classification PASSED                                                                                                                                                                                                                             [ 50%]
tests/models/vit/test_modeling_tf_vit.py::TFViTModelTest::test_pipeline_image_classification PASSED                                                                                                                                                                                                                               [ 62%]
tests/models/vit/test_modeling_tf_vit.py::TFViTModelTest::test_pipeline_text_classification SKIPPED (Test is skipped: No model architecture under framework tf with the configuration class `ViTConfig` is found for the task `text-classification`.)                                                                             [ 75%] 
tests/models/vit/test_modeling_vit.py::ViTModelTest::test_pipeline_image_classification PASSED                                                                                                                                                                                                                                    [ 87%]
tests/models/vit/test_modeling_vit.py::ViTModelTest::test_pipeline_text_classification SKIPPED (Test is skipped: No model architecture under framework pt with the configuration class `ViTConfig` is found for the task `text-classification`.)  
```

Comments:
_The documentation is not available anymore as the PR was closed or merged._

I requested @LysandreJik for review as this is one topic we discussed several times.

Note that tests are then run under "regular" tests of the model, so the pipeline tests as a separate job seem irrelevant after this PR (not necessarily a problem, just stating that we will need to update the tests creation).

> Note that tests are then run under "regular" tests of the model, so the pipeline tests as a separate job seem irrelevant after this PR (not necessarily a problem, just stating that we will need to update the tests creation).

@sgugger No, the current pipeline tests (the remaining test methods) and **their CI jobs should be kept there**, for other test methods in the current existing `XXXPipelineTests`, like this
https://github.com/huggingface/transformers/blob/c26b1503f22feb20d37588d4243bd444d297bc13/tests/pipelines/test_pipelines_image_classification.py#L115

The only pipeline tests being moved to modeling tests is 
https://github.com/huggingface/transformers/blob/c26b1503f22feb20d37588d4243bd444d297bc13/tests/pipelines/test_pipelines_image_classification.py#L60
(which are executed with the dynamically generated tests with the meta class)

The tests like `test_small_model_pt` doesn't use the meta class, and they are bound to a specific model/repo, which is not suitable to put into modeling testing workflow.

Ok, I'm briefly going to share what we discussed internally:

- I am not too big of a fan of `skips` being ok permanently like this PR would make tests to be `like BertModel::test_pipeline_audio_classification`. If Bert isn't supposed to do audio-classification, we shouldn't have a test for it.
- I don't like moving tests from `tests/pipelines/` to `tests/models/` it makes the testing harder to reason about imo. For instance, I broke tests on purpose and still had a green test: https://app.circleci.com/jobs/github/huggingface/transformers/694859. Either the runner should run those tests, or the tests should be brought back to `tests/pipelines`.
   Another argument is that the `PipelineMixin` actually needs to read `tests/pipelines`. This is putting some coupling between `test/models` and `test/pipelines`. The dependency should be reversed IMO (the pipelines depends on the models, not the other way around).
- There's a ton of skipping happening in the tests, which we could maybe take the opportunity to remove to get hard errors `AutoTokenizer.from_pretrained("hf-internal-testing/")`  not working should be an auto error (if the tokenizer is supposed to exist, which we should be able to know from the task under test). We can do things progressively since the migration exhibited actual missing tests (which is really good !) But we should focus on removing them as much as possible so we can build some trust that the testing is correct. (It's really easy to miss some skipped too much tests, where it's much hard to miss hard failures)

Overall I think the core of the test changes, are about moving away from custom hard-to-reason-about logic in how we create the tiny models to make them all easily accessible on the hub. Not as much about `TestMeta` vs `TestMixin` 

@Narsil 

Could you tell us (me) which test(s) - the test names) you tried to break intentionally but sill get green CI?

> I broke tests on purpose and still had a green test: https://app.circleci.com/jobs/github/huggingface/transformers/694859. Either the runner should run those tests, or the tests should be brought back to tests/pipelines.

> I don't like moving tests from tests/pipelines/ to tests/models/ it makes the testing harder to reason about imo.

I don't like having a non-canonical model be tested in the pipeline. It also makes the testing harder to reason about. It's completely fine if the pipeline tests GPT-2, but having a change in some random generative model impact the test_pipeline_text_generation, and then put some custom logic there to skip it based on the model name is not a good design either.

> having a change in some random generative model impact the test_pipeline_text_generation, 

If some behavior is modified in such a way that it breaks existing code, them IMO it is justified to break in the pipelines. Pipelines are users of models, if there is a breaking change in a model, and it's seen by the pipeline tests, imho it means the tests are doing their job correctly.

> and then put some custom logic there to skip it based on the model name is not a good design either.

That's the core issue. I'm not advocating for this either.

I'm curious about your solution to remove code like [this](https://github.com/huggingface/transformers/blob/b31cee67277d25fdb3dbd4619a4aa59d71a29b56/tests/pipelines/test_pipelines_common.py#L96) without moving the tester to a model common tester then. Also the TestMeta has been criticized as too hard to use by almost all model contributors. The core of the problem is not just the tiny model creation.

> I'm curious about your solution to remove code like [this](https://github.com/huggingface/transformers/blob/b31cee67277d25fdb3dbd4619a4aa59d71a29b56/tests/pipelines/test_pipelines_common.py#L96) without moving the tester to a model common tester then.

This is exactly my point.  If RocBert cannot work with FillMask, it most likely means that `RocBertForMaskedLM` is not respecting an invariant of `ForMaskedLM` the pipeline is relying on.
Actually it seems to be working ok now, so maybe this condition is not needed for `fill-mask`. https://huggingface.co/weiweishi/roc-bert-base-zh

What the pipeline does ?
```
model_inputs = tokenizer(somestring_with_mask)
model_outputs = model(**model_inputs)
outputs = somefunction(model_outputs["logits"], model_outputs["input_ids"])
```

> TestMeta has been criticized as too hard to use by almost all model contributors.

Logic was indeed pretty hard to follow, but I think the issue is the complexity of the logic, not the fact that it's meta vs something else.
With the addition of tiny tests, most tests should become equivalent to 

```python
pipe = pipeline(model="hf-internal-testing/my-tiny-modelForXX")
out = pipe("some string with [MASK]")
self.assertEqual(out, [{"class": "bear"}])
```
at least in principle.
If they could be spelled out in test code I would find that awesome (since explicit tests are always easier to understand)

For completeness' sake, `feature-extraction` is a good example where the opposite is True. `XXXModel` do not have any invariant to uphold, because the inputs are a bunch of tensors with no specific shape or name (since we moved away from pure NLP), so it's hard for the pipeline to build upon it (and that's why this pipeline is broken for all non-nlp models). 

> TestMeta has been criticized as too hard to use by almost all model contributors. 

Part of the reason of its existence was to ensure forward compatibility (so if you implement ForXX, you have a working pipeline).
We could by all means remove this. But @LysandreJik added it because we needed some kind of way of detecting non functional pipelines early rather than later (because fixing the model once it's been published almost always requires a breaking change).

Maybe if we had lower level tests on the `ModelForXX` it could become more explicit what's missing ? Like you need to be able to do:

```python
input_ids = torch.zeros(3, 10).long()
output = model(input_ids=input_ids)
assertExists(outputs.logits)
```
of some kind ?

And just as a last note. In the `ModelTester` I have seen countless times (mostly in tokenizer part of test since it's the ones I look at the most) contributors override the required test, and just skipping it because it was too hard to implement.
If you're allowed to do that, it's definitely more convenient, but you're also almost certainly introducing bugs somewhere else. 

https://github.com/huggingface/transformers/blob/main/tests/models/gpt2/test_tokenization_gpt2.py#L128
https://github.com/huggingface/transformers/blob/main/tests/models/gpt2/test_tokenization_gpt2.py#L250
https://github.com/huggingface/transformers/blob/main/tests/models/clip/test_tokenization_clip.py#L183
https://github.com/huggingface/transformers/blob/main/tests/models/layoutlmv3/test_tokenization_layoutlmv3.py#L264
https://github.com/huggingface/transformers/blob/main/tests/models/layoutlmv3/test_tokenization_layoutlmv3.py#L817
https://github.com/huggingface/transformers/blob/main/tests/models/layoutlmv3/test_tokenization_layoutlmv3.py#L1715

And all of these bugs are silent, since the test exists and are green, yet do not test anything actually.


Final note: My stance is about what we're going to loose and why I'm against it. Now, I am not a core maintainer, and contributor happiness is not something I can easily put into my balance to do a good tradeoff in this particular decision (nor core maintainer's happiness). This is definitely an important factor in the decision to be made.
Most of the failing tests I have seen where I have reviewed the code was an actual broken invariant, rarely a pipeline issue (and when it was it was definitely off for the better in the pipeline by forcing to use cleaner code), so I view these tests as providing value in preventing bugs. Maybe the proposed change is still the way to go, maybe there are other alternatives. 

I also view some of these tests as painful because I sometimes have a hard time fixing them. But 99% of the time, I view these tests as helping rather than hurting in the long run.

@Narsil Just 2 points to your comments

> This is exactly my point. If RocBert cannot work with FillMask, it most likely means that RocBertForMaskedLM is not respecting an invariant of ForMaskedLM the pipeline is relying on.

Sometimes the issue is not really at pipeline level. It could be a simple **tiny** model's tokenizer or processor issue, like the vocabulary size is not correctly set or sth similar (as you know, the creation of tiny models has a lot of potential pitfall). And not all these kinds of thing are easy to figure out. Moreover, we still have the slow tokenizer not being tested before (and now it is tested, but a few failures are currently skipped - which will be checked in next steps).

> And all of these bugs are silent, since the test exists and are green, yet do not test anything actually.

@SaulLu and me had a short conversation that we should instead use `self.skipTest` rather than silently return. This is something we can improve.

Thank you for the feedbacks, @LysandreJik. Here is my comments, question, and final thought after reading the feedback. 

## Updates: 2023/02/13

I have moved forward with some of @LysandreJik's suggestion, so the comments after this **Update** section is no longer relevant (unless you still want to read it).

What have been implemented:

- Adding a class attribute to `pipieline_model_mapping`, which should be specified with an explicit mapping of task/model for pipeline testing.
https://github.com/huggingface/transformers/blob/636e90c721f682f9df637915e6b3ee0ca278ff34/tests/models/bert/test_modeling_bert.py#L448-L461
- Rely on a (newly added to `transformers`) json file regarding the information of tiny models on the Hub.
https://github.com/huggingface/transformers/blob/636e90c721f682f9df637915e6b3ee0ca278ff34/tests/test_pipeline_common.py#L52-L54
  - So we don't need to access this summary file on the Hub
  - The advantage of this file is we know explicitly which (tiny) model/tokenizer/processor are available on the Hub, so we don't need to `try ...from_pretrained(...) ... except ... skipTest`.
  - This list needs to be updated regularly (mostly for newly added model code)
- Add a new _magic_ method to disable irrelevant tests
https://github.com/huggingface/transformers/blob/636e90c721f682f9df637915e6b3ee0ca278ff34/tests/test_pipeline_common.py#L74-L82
  - This is done by setting some pipeline test methods to `None` at model test class declaration time
https://github.com/huggingface/transformers/blob/636e90c721f682f9df637915e6b3ee0ca278ff34/tests/models/bert/test_modeling_bert.py#L461
  - This is to avoid some tests to be even collected: for example `test_pipeline_audio_classification` for `bert` is really irrelevant.
  - So such tests won't be run and then being skipped at runtime.
  - We also have a cleaner test reports.

## Comments

> a very explicit way to show which models should be supported by pipelines and which should not.

- This mappings are (eventually) derived from our exposed `XXX_MAPPING`. If we have issues at pipeline testing level (regarding architectures for specific tasks), it also implies that:
    - our mappings has some issues
        - (these are publicly exposed objects, but I am not sure if these are widely used)
   - Auto might be affected 
   - pipeline classes (under `src/transformers/pipelines`) are potentially affected
   - the tiny model creation are (potentially) affected (use a lot of mappings)

    Therefore, from this viewpoint, **we should instead put focus on the correctness of these mappings** (with any extensive suite of tests we need/want), and rely on these mappings (which is already the cases in many places, even not all in testing suite), instead of trying to develop extra mappings (for example, the suggested dict attribute) under each model test class.

    **If we do identify any missing case, a fix/update on the mappings `XXX_MAPPING` has much more benefits on `transformers`**, while a fix on a particular `dict. attribute of a model test class` just benefit that model test case.

    Having new mappings also increase the chance of having inconsistent mappings (for example, the one givens by `XXX_MAPPING` (when restrict to a configuration class) and the one given by `pipelines_to_test`.

> remove the magic identification of tokenizers/processors linked to an external file on the Hub
- That repo. is necessary when we decided to use tiny models on the Hub but while we are still using metaclass. As you can see at this line
https://github.com/huggingface/transformers/blob/ab81397c8cbe0ba74c75c7dfa4a8787ce6916ce7/tests/test_pipeline_common.py#L34
during the dynamic test generation, all the 8 processes (from `pytest -8`) will try to collect the whole set of tests. Without this file, all the 8 processes tries to access hub repos. for all the involved model architectures, which leads to connection errors, and then not all processes get the same number of tests being collected, and leads to a super bad error message and state (during test collection), even before the test suite could be run.

    Now, without metaclass, the tests are collected without the need to access the Hub. The access to the Hub  only happens during the tests being actually run (distributed in the 8 processes). **We no longer have the issue, and we can remove the usage of this external file on the Hub.** So I will remove it and made the necessary changes.

> and gain readability, as well as ease of contribution.

- When contributor on (new) pipeline testing, they will mainly deal with this method
https://github.com/huggingface/transformers/blob/ab81397c8cbe0ba74c75c7dfa4a8787ce6916ce7/tests/test_pipeline_common.py#L154
I am a bit suspicious about if they will need to deal with or look at the logic `run_task_tests`, but you core maintainers and @Narsil have more experience regarding the user experience on pipeline testing (so far with metaclass).

## Questions

> I would remove the magic creation of models

- Could you point me where is **the the magic creation of models** you mean? The model is obtained by
https://github.com/huggingface/transformers/blob/ab81397c8cbe0ba74c75c7dfa4a8787ce6916ce7/tests/test_pipeline_common.py#L188
from the tiny models that are created offline and uploaded to the Hub.

## Final thought

I provide my viewpoints that are a bit different, I do see the arguments in the suggestions are valid from other angles.
I am open to the suggestion of being more explicit regarding which pipelines to be tests for a specific model class, if you still think that has more benefits after reading this long comment :-). 


## Extra words for pipeline testing - regarding tiny models on the Hub

I am not sure if I communicate it clear enough, but we need some improvements on the usage of tiny models on the Hub for pipeline testing

- We will need to run the creation more frequently, either in a regular basis (automatically), or manually when a new model is added to the library
  - we won't be able to always get the checkpoints, or the tokenizers/processors
  - even we can create all files, it's still possible we will encounter some issues when using them in pipeline testing

So at this moment, when a contributor work on a model, say `NewModel`, and add something like in `NewModelTest`
```python
pipelines_to_test = {
       'sequence-classification': NewModelForSequenceClassification,
       'token-classification': NewModelForTokenClassification,
   }
```
it's still possible the tiny model checkpoint(s) are not on the Hub yet. Force the users to run the tiny model creation would be too hard, and even running them, they won't be able to upload to `https://huggingface.co/hf-internal-testing`.

So it seems to me we can only run and upload those tiny checkpoints afterward.



> either in a regular basis (automatically)

This seems the best approach imo, since it would mean our tools allow for it to happen. When it works, usually it's just a matter of choosing a small config (n_layers smaller + hidden_smaller + vocab_size smaller for NLP for instance). 
But you're right it's not always possible easily (I tend to think it's a modeling issue)

Hi @LysandreJik 

I have added all tasks to `PipelineTesterMixin`, updated all model test classes to inherit from `PipelineTesterMixin` and defined their `self.pipieline_model_mapping`.
- In each modeling test file, I only add `PipelineTesterMixin` to only one test class
  - to avoid the tests being duplicated in classes like  `BartModelTest` and `BartStandaloneDecoderModelTest`
- For transparency, I use an automatic way to obtain and add such mappings from current `...PipelineTests` classes to the modeling test classes
  ```python
  class TextGenerationPipelineTests(unittest.TestCase):
      model_mapping = MODEL_FOR_CAUSAL_LM_MAPPING
      tf_model_mapping = TF_MODEL_FOR_CAUSAL_LM_MAPPING
  ```

If everything is fine for you, I am going to merge 🔥 

(`test_tf` is successfully completed - circleci has some issue to report back here)

Hello @sgugger . Last week, I was about to merge this PR, but I felt something might be wrong and found out that

**tests_fetcher.py won't work well along with this PR**

In short, if I change a file named `src/transformers/pipelines/text_classification.py` in a commit, and run

```bash
python utils/tests_fetcher.py --diff_with_last_commit
```
it won't trigger those modeling test files that contain the text classification (added via `pipeline_model_mapping` in this PR).
See the bash output shown at the end.

I haven't yet look the code in `tests_fetcher.py`, **but I am wondering**:
- if you have ideas of how we should adopt `tests_fetcher.py` to this new situation?
- and if you think it might be easier for you to take over this PR and make the necessary change regarding this final part of test fetching?
  - (I am not asking you to do it. I can work on it with your help/idea - it might take longer for me however) 


### bash output

```bash
### MODIFIED FILES ###
- src/transformers/pipelines/text_classification.py

### IMPACTED FILES ###
- src/transformers/__init__.py
- src/transformers/commands/run.py
- src/transformers/commands/serving.py
- src/transformers/commands/train.py
- src/transformers/commands/transformers_cli.py
- src/transformers/convert_graph_to_onnx.py
- src/transformers/pipelines/__init__.py
- src/transformers/pipelines/text_classification.py

### TEST TO RUN ###
- tests/onnx/test_onnx.py
- tests/pipelines/test_pipelines_text_classification.py
- tests/utils/test_cli.py
```

For now, I think the easiest way is to add a special return statement [here](https://github.com/huggingface/transformers/blob/2d506ea4c4980a4cab43c2940d9836ddfd629524/utils/tests_fetcher.py#L432) where you put something like `tests/models/modeling_*.py` on top of the pipeline test to force-trigger all common model tests in those instances. I'll rework it when I dive into the test_fetcher v2 (normally later this week!)

@sgugger Thank you for the suggestion (as well as the good willing to work on test fetcher v2).

**I have to modify a bit more than what you suggested**. **The wild card** `_*` such as `pipelines/test_pipelines_*.py` (already on `main`) and `models/.../test_modeling_*.py` **won't work** as this statement
https://github.com/huggingface/transformers/blob/6ca844582c2ec12309e054484a419d3d467d0f8c/utils/tests_fetcher.py#L581
will remove them. I have run the command with a change to `pipelines/__init__.py` against `main`: **we do get all pipeline test files being collected, but this is not due to the wild card, but the dependency between the modules**

I therefore change them to explicit test files using `glob` to avoid further usage of wild card (to avoid potential missing test files to run - despite the above example is fine).

[Here](https://github.com/huggingface/transformers/pull/21516/commits/23c875f919176240e4b03b010a89f4d14562a56d) is the change in the last commit regarding test fetcher.


Oh good catch! Change LGTM!