Move `is_pipeline_test_to_skip` to specific model test classes
Author: ydshieh
Pull request: yes

# What does this PR do?

As promised!

So far, it's incomplete - just for you to check this way is OK. If so, I will move all of them around.

It's normal to have some test failures at this moment.

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

> I don't get the use case for `is_pipeline_test_to_skip`, we have the `pipeline_model_mapping`, why not just remove the model from there?

The reasons are:

- the mapping `pipeline_model_mapping` is (IMO) what should be tested in **theory**, i.e. what model classes of a specific model type are for some pipeline tasks
  - **it's not the place to control what to skip or not**
    - if we do skip tests by using this mapping, we lose the important information of `why this test is skipped`. We will only see some model class is not in the mapping, but not `skip this test as ...[whatever more precise reason]`
  - define this mapping as what should be tested in theory allows:
    - to generate the mapping in a systematic way **(less error prone)**
    - to define a repo check (so **less chance to miss a pipeline test**)
- But **most importantly**, the skip conditions sometimes go to the level of what tokenizer/process classes are used. It's not just about the model class
  - For example, a model class might be tested with a faster tokenizer, but not a slow tokenizer (due to some issue) 

> Ok then, let's validate with @LysandreJik as well though!

I have talked to him offline when you were off. But yeah, let's make it official :-)

I think this would add another layer of unneeded complexity; do you think this will touch a lot of the models? In any case if you're both ok with this, I'm fine with merging it, but let's aim for as simple a change as possible that makes contributing a new pipeline/model as simple as possible. 

@LysandreJik 

This would almost **never** affect the **model** contribution experience:
- when a model is added, the tiny model creation **would not run by the contributor** (at least for this moment, as it's way to complex)
- no tiny model checkpoint on the Hub for the newly added model -> no pipeline test will be (actually) run for that model
- no pipeline test being run -> no need to skip any test by adding/changing `is_pipeline_test_to_skip`.

==> It's us (if not me) to create and upload the tiny models. Once done, if there is anything not working, **it's us to skip them**.
(The complexity is absorbed by the hidden process of tiny model creation that is not run by the contributor)

Regarding **adding a new pipeline**: if an existing tiny model work (i.e. it could run other existing pipeline tasks), the chance that it works for the new task (if it is suitable for that task) is high. **So the chance to changing existing `is_pipeline_test_to_skip` is low**.

Note that since it's a test modeling file and contributors add new models by copying those, those `is_pipeline_test_to_skip` will be copied over new models automatically. So it will be part of the contributor experience (though probably unnoticed) and we will get lots of those without really noticing (since the PRs that add new models are very big). This can be alleviated in some way if the add-new-model-like command takes special care to remove the `is_pipeline_test_to_skip` from the new test file, but this is again more complexity.

Thank you @sgugger , very nice point! Let me play with `add-new-model-like command` and see how the current `pipeline_model_mapping` and `is_pipeline_test_to_skip` will be treated by this command.


I tried it, both `pipeline_model_mapping` and `is_pipeline_test_to_skip` will be copied.

If `pipeline_model_mapping` is used to also control which tests should be skip or not, it's also dangerous that this attribute being copied (especially automatically) to another model test files: as we are very likely to miss more and more tests that should be tested (a test that fails for an existing model have the chance to work on a new similar model - and should be tested at once to determine if we need to skip it).

Also as mentioned earlier:
  - manually edit `pipeline_model_mapping` have more disadvantages than good.
  - having `pipeline_model_mapping` edited by a contributor won't actually make the pipeline tests to run - we still need to create and upload the tiny models to `hf-internal-testing` 

**I am going to make changes to `add_new_model_like` to not copy these 2 attributes**. It makes the script a bit more complex, but it won't bother the users - as long as we all agree and know that these 2 attributes for pipeline testing are not for contributors to add/change (at least not before we can have a much easier and safer process to create/upload tiny models). 

Is this OK for you @sgugger ?

That plan works for me!

As discussed offline, I changed the approach to use string only.

Would still like @sgugger to elaborate a bit more:

> Using ast would be a first for the repo and would make contributing harder 

Do you mean for the contributors (either external or internal?) who want (or might need) to modify `src/transformers/commands/add_new_model_like.py`? If so, I agree better not to use `ast` here. If you mean the usage only, I don't think using `ast` is a real problem - if they don't need to look the internals.

I would also like to mention, for automatically adding `pipeline_model_mapping` to a test file (from the auto mapping, prepared in `XXXPipelineTest` classes, we will need more access to the test files. And string approach would make it more complex (well `ast` is complex, but at least it also avoid a lot of things). Furthermore, if we want to add a new repo check on `pipeline_model_mapping`, the same consideration applies.

So let's have a talk later - at least for the above 2 scripts that I might have to implement.

(well, after a 2nd thought, I understand using `ast` might bring new burden to the reviewers.)

@sgugger Need your feedback :-) for

https://github.com/huggingface/transformers/pull/21999#discussion_r1132834136