[trainer] --model_parallel hasn't been implemented for most models
Author: stas00
Pull request: yes
Labels: name: Model Parallel

Apparently we unleashed `--model_parallel` in trainer w/o checking if the model supports MP (most don't). This PR:

* [x] checks whether the model supports MP and asserts otherwise
* [x] fixes the cl arg help to note that the flag will only work if the model supports MP 

As we are gradually starting to build MP-support a cleaner solution will be made in the future, but for now this is good enough to prevent misleading false expectations as reported in https://github.com/huggingface/transformers/issues/9336

(Also for the future, I'm not sure whether it'd be better to check `model.config.architectures`, which would be more precise than checking `model_type` since it's the `architectures` that may or may not support MP within the same `model_type` - but that's a different discussion).

Fixes: #9336

@patrickvonplaten, @sgugger 


Comments:
@alexorona proposed to have the `model_parallel` method in `PreTrainedModel`, https://github.com/huggingface/transformers/pull/9323#issuecomment-752352280 which then would break this code as it'd be then present in all models.

I see this PR as a quick band-aid since we released the new cl arg w/o checking that it always works. And then we will surely improve it as we generalize MP and not leave it this way. This is definitely not how it'll remain in the long run.

So should we merge this one as a hot-fix?

-------------

An absolute yes to `PreTrainedModel.parallelizable` accessor - default `False`, then a `True` override for each specific model head that implements it - better than checking arch which doesn't guarantee that it'll have all heads parallelizable. 

And also what do you think about tests? Currently we hardcore a list of parallelizable models:

https://github.com/huggingface/transformers/blob/086718ac6e20ca2e2cfa3aa0f6da9dc7ee34f6c6/tests/test_modeling_t5.py#L491

should it remain this way or should we automatically derive those from the model by iterating over `all_model_classes`:

https://github.com/huggingface/transformers/blob/086718ac6e20ca2e2cfa3aa0f6da9dc7ee34f6c6/tests/test_modeling_t5.py#L489

and automatically deriving which are parallelizable. Less code to write in the future.


I'd rather merge as a hotfix the proper check and then worry about the tests in a follow up PR (I think we should have a combination of a flag (like for pruning) and checking the models having the attributes there).

It no longer will be hot, but yes, I will code that ;) thank you for the feedback, @sgugger 

> I think we should have a combination of a flag (like for pruning) and checking the models having the attributes there).

I'm not sure what you mean here. An example would be helpful to understand what you propose.

The class `ModelTesterMixin` has a few attributes that control what common tests to apply. I just realized while reading it that it already has the `test_model_parallel` flag so this part is done already. All that is left is just to infer the models to test from the presence of the right attribute :-)

OK, I added `model.is_parallelizable` property - let me know if this looks good, or whether you prefer not using a property. 

if you prefer w/o `is_` or not have it a property please let me know.

> I'm fine with this design but it differs from what we were talking about, so we should check the others are fine with it too before merging.

Yes, of course.

that's why it is no longer a hotfix, but it seems to be fine - only one user has filed an issue about using a non-working `--model_parallel` so far.

So since the only change I proposed is from `parallelizable` to `is_parallelizable`, do you still think we ought to re-validate with @LysandreJik?

Yes, let's wait for him to review this tomorrow morning (he's on European time for the next month or so).