Move `is_pipeline_test_to_skip` to specific model test classes

[ydshieh]

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.

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[ydshieh]

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)

[ydshieh]

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 :-)

[LysandreJik]

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.

[ydshieh]

@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.

[sgugger]

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.

[ydshieh]

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.

[ydshieh]

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 ?

[sgugger]

That plan works for me!

[ydshieh]

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.)

[ydshieh]

@sgugger Need your feedback :-) for

https://github.com/huggingface/transformers/pull/21999#discussion_r1132834136