[finetune_trainer]  max length cl args redesign
Author: stas00
Pull request: no

Splitting of from https://github.com/huggingface/transformers/pull/9241, it's been proposed to refactor the following 4 cl args of `finetune_trainer.py`:

1.   `--max_source_length`
2.   `--max_target_length`
3.   `--val_max_target_length`
4.  `--test_max_target_length`

https://github.com/huggingface/transformers/blob/f38c4ad302dd34faef1137b13e9636f4408b0462/examples/seq2seq/finetune_trainer.py#L82-L110

There are multiple comments wrt this in https://github.com/huggingface/transformers/pull/9241, especially towards the end of it

Let's redesign it here and then do a single breaking change to this probably in the new year.

To summarize the main suggestions so far were:

1. to perhaps remove `--max_source_length` - but we need use cases to see whether this is safe to do
2. collapse cl args 2-4 into a single `max_length` arg to match `generate`'s API.

@sgugger, @patrickvonplaten, @patil-suraj 

Comments:
After giving this some thought, given the fact there are two different lengths here (input and targets) I would propose keeping `--max_source_length` and `--max_target_length` to avoid any confusion for the user. In the same vein, `run_qa.py` contains `max_seq_length` and `max_answer_length` to clearly differentiate the two.

As for val/test I don't have any strong opinion, apart from the fact they are not used properly in the prediction at the end (only for the preprocessing), so they should be collapsed IMO

This works for me!

I especially would like to see all examples use same cl args for the same functionality.

Works for me too,

And for val/test targets lengths, IMO we can collapse it into one single `max_generate_length` or `eval_max_length` since  most of the users (and the example scripts as well) use the same value for both args

Having only `max_source_length` and `max_target_length` works for me!

regarding the `val_max_target_length` and `test_max_target_length` args

The reason we (I and Sam) decided to add that

- in general, it’s okay to have a bit smaller max target length for training/validation because some documents could be too long than avg length, it’s okay during training if these get truncated
- for the test, we should set the max target length to be as long as the longest text in the test set so it won’t get truncated. The reason is if the text in the test set is truncated then the calculated metrics won’t be accurate.

Also, we should mention in the readme that it's best to use `run_eval.py`  for calculating metrics.  As there is an issue when calculating BLUE score this way as outlined in #9161

> * for the test, we should set the max target length to be as long as the longest text in the test set so it won’t get truncated. The reason is if the text in the test set is truncated then the calculated metrics won’t be accurate.

Why not compute this in the script then? That would avoid having an argument that is half-used.

What I'm hearing is that perhaps there was a concrete situation where the training needed a shorter max length than eval/test. So @sgugger's suggestion will solve your concern that the scoring is done on the full length, but not if for some reason the training stage should use shorter sequences. 

So we have 2 related situations and I'm not sure @sgugger's solution covers the 2nd one. I just don't know whether it's a real use case or a may be.

Please let me know if I haven't explained myself clearly.


Thinking more about it, @patil-suraj, won't fixing up `generate`'s `max_length` to match the longest max length of the test dataset lead to better scores than what they will be otherwise? And thus provide misleading results? 

For example, let's take translation, the best model would do the most correct translation regardless of whether it is allowed to generate much longer sequences. So if we calculate any such max length dynamically to be fair I think there needs to be added some extra length beyond the longest test sequence. Say `max(len(tokenize(test_inputs)))*1.1`? Does it make sense?

So perhaps this is how it should work. 
- `max_target_length` is for training
- for eval/train we derive max length from the input data plus some margin?

What I meant here is that the `test_max_target_length` is also passed to the dataset, and the dataset then truncates the reference targets (translations, summaries) longer than that. So later the generated targets are compared with (possibly) truncated  references which will result in incorrect metrics 

You're absolutely correct - that doesn't sound right.

So before we can discuss the flags we then need to first discuss the algorithm, otherwise we won't get anywhere.

If in order to get the correct metrics we must not truncate the val/test datasets then why are we doing that in the current code?

Perhaps what I suggested at the end of https://github.com/huggingface/transformers/issues/9265#issuecomment-750423347 is a better way to approach it?

Also please don't forget that max_length is used to deal with OOM limitations

This has been resolved.