[trainer] remove `--model_parallel`
Author: stas00
Pull request: yes

Per @sgugger's request removing `--model_parallel` in trainer, as it was never tested or made to work with the trainer.

We will get back to it in the future.

This PR doesn't introduce breaking changes, since  `--model_parallel` never worked (well other than in my MP PRs that have been parked for now, since they are very inefficient and we are looking for a better approach, rather than waste time on sorting those out).

@LysandreJik, @sgugger 

Comments:
Thanks for putting it back. Since we're in a PR on this test alone, can we "fix" it to ignore the `args.model_parallel` argument? This argument will be removed/renamed (I'd prefer the first option as it's not useful) since peoples are confusing it with something that will enable `DataParallel`. The test can be replaced by `model.is_parallelizable and model.parallel` I believe, with the current API.

2 things:

1. you must be referring to `self.model_parallel`? But it will be always `False` unless  `model.parallelize()` is called! 

   So while you can rename the argument, you can't remove it, the user needs to activate this explicitly and the trainer then must activate MP with `model.parallelize()`
   
   Wrt `DataParallel`. Why are we turning it on automatically in first place? Why not make it manual and call it `--data_parallel` - no more confusion. Loud and clear:
   
   - `--model_parallel`
   - `--data_parallel`


2. As we discovered last night current trainer doesn't work at all with --model_parallel - see https://github.com/huggingface/transformers/pull/9211#discussion_r553172405 there is no activation of that parallel mode - nobody calls `model.parallelize()` so it's very broken

I change this code last night to;
```
        if self.args.model_parallel:
            if model.is_parallelizable:
                model.parallelize()
            else:
                raise ValueError(
                    f"{model.__class__.__name__} implementation currently doesn't support model parallelism, therefore --model_parallel cl arg cannot be used"
                )
```

and it doesn't work when I try:

```
rm -r output_dir; CUDA_VISIBLE_DEVICES=0,1 PYTHONPATH=../../src USE_TF=0 ./finetune_trainer.py --model_name_or_path t5-small --output_dir output_dir --adam_eps 1e-06 --data_dir wmt_en_ro --do_eval --do_train --evaluation_strategy=steps --fp16 --freeze_embeds --label_smoothing 0.1 --learning_rate 3e-5 --logging_first_step --logging_steps 1000 --max_source_length 128 --max_target_length 128 --num_train_epochs 1 --overwrite_output_dir --per_device_eval_batch_size 4 --per_device_train_batch_size 4 --predict_with_generate --eval_steps 25000 --save_steps 25000 --sortish_sampler --task translation_en_to_ro --test_max_target_length 128 --val_max_target_length 128 --warmup_steps 1 --n_train 2 --n_val 2 --n_test 2 --do_predict --model_parallel
```

It doesn't look it ever worked...

i.e. MP works when setup up manually but doesn't work in trainer.

p.s. I tagged you on that discussion - not sure if you saw it.

> i.e. MP works when setup up manually but doesn't work in trainer.
> As we discovered last night current trainer doesn't work at all with --model_parallel - see #9211 (comment) there is no activation of that parallel mode - nobody calls model.parallelize() so it's very broken

That's not a discovery on my side, that is exactly why I keep saying that the argument `--model_parallel` should be removed. It doesn't actually do anything and is confusing for the user. The call to `model.parallelize()` can always be done outside of `Trainer` IMO, which is why the test can be changed as suggested. We can think of integrating it inside the Trainer later, when the API is stable and actually used, for now I don't see the point of adding this.

> Wrt DataParallel. Why are we turning it on automatically in first place? Why not make it manual and call it --data_parallel

That would be a big breaking change in the API, and beginners actually want to have the parallelism work out of the box when they have several GPUs, so I don't see why change something that works.

> The call to model.parallelize() can always be done outside of Trainer IMO, which is why the test can be changed as suggested. 

It doesn't work



>     Wrt DataParallel. Why are we turning it on automatically in first place? Why not make it manual and call it --data_parallel
> 
> That would be a big breaking change in the API, and beginners actually want to have the parallelism work out of the box when they have several GPUs, so I don't see why change something that works.

OK, then the flag should be there with the default On? Surely a user should be able not to run DP and it's not possible at the moment.

OK, so I did remove `--model_parallel` - no problem in `trainer.py` since I used `model.is_parallelizable and model.parallel` instead - and I now understand that the point is that the user has to activate `model.parallelize()` themselves before passing the `model` to the trainer - i.e. no examples scripts will support MP at the moment.

The problem is `training_args.py` - how do I deal with:

```
        if not self.model_parallel:
            train_batch_size = per_device_batch_size * max(1, self.n_gpu)
        else:
            train_batch_size = per_device_batch_size
```

`self` is args here, and there is no `trainer` object. Suggestions?

But I guess I need to first figure out how to make MP work in trainer at all, I doesn't look it was ever tried or tested. As it fails for me.

FWIW, `--model_parallel` works just fine with my Bart MP PR: https://github.com/huggingface/transformers/pull/9384#issuecomment-756300194 in case someone needs it.

I suspect t5 MP wasn't tested/made to work with `generate` tools (beam search, etc.) - **edit** It works now in this PR https://github.com/huggingface/transformers/pull/9323 - but super slow in beam search! 

OK, I committed the bulk of it, and @sgugger will push some magic to deal with `training_args.py`

tests should be failing I think until he does that. 

So now I can see I can jokingly blame my initial mistake on @sgugger since he wanted it removed all along and so I unconsciously did it during rebasing and he unconsciously saw this as the right thing to do during the review ;) It's all Freud's fault anyway ;)

I added a wrapped first, but it looked out of place so I introduced and documented a new attribute: `self.is_model_parallel` - hope it's loud and clear.

@sgugger, I must be doing something wrong - that docstring section of `Important attributes` that I started in model_wrapped PR gets wrapped all funny - so I tried to add bullets and then it gets all messed up, as it bunches it all up into one paragraph. If I add new lines then `make docs` fails. Your magic touch is needed. Thank you.

and here is why I  removed `init=False` in https://github.com/huggingface/transformers/pull/9451/commits/a7a39216e99aae60238962ec3d6c96ecf23da42b

The tests were failing with:
```
TypeError: __init__() got an unexpected keyword argument '_n_gpu'
```
https://circle-production-customer-artifacts.s3.amazonaws.com/picard/forks/5bdabdd888af1f000130874a/278[…]cc8b6d6c390aab800d0cc1350f731a19529ac82f48


Thank you for fixing the docs, @sgugger!