Seq2seq generation with prefix
Author: yjernite
Pull request: yes
Labels: name: wontfix

This PR introduces two small changes in the way model.generate() works.

Previously, the function took in an input_ids argument which had different behaviors in the seq2seq and language model settings: in language modeling, input_ids could be used to provide a prefix for the generation, while in seq2seq, input_ids represented the encoder input and the generation prefix was automatically initialized to a batch with one time step willed with the [BOS] token.

Conceptually, this feels a little awkward, as a language model and the decoder of a seq2seq model should really behave similarly (the latter just has added conditioning). And more importantly, there was no way to provide both the encoder input_ids and a generation prefix in the seq2seq model.

I've added a prefix_ids argument to fix that. The model will still default to using input_ids as a prefix in the language model setting so as not to break current use cases, but otherwise the model works with prefix_ids and initializes it similarly for the LM and seq2seq settings.

The second smaller change is the initialization of the past variable in generate_beam_search and generate_no_beam_search: it is now initialized to the form it will have in later generation steps, so we can dispense with the firs step tests in the prepare_inputs_for_generation functions in modeling_t5.py and modeling_bart.py

(Next time I'll do two separate PR's as suggested by @sshleifer :) )


Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=h1) Report
> Merging [#3739](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/7a7fdf71f80452fcae064bd016f06e9a0f0f19ed&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `95.74%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/3739/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #3739      +/-   ##
==========================================
- Coverage   78.27%   78.26%   -0.02%     
==========================================
  Files         104      104              
  Lines       17835    17843       +8     
==========================================
+ Hits        13960    13964       +4     
- Misses       3875     3879       +4     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3739/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ191dGlscy5weQ==) | `91.81% <95.45%> (-0.09%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3739/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl91dGlscy5weQ==) | `92.89% <95.65%> (-0.08%)` | :arrow_down: |
| [src/transformers/modeling\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/3739/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19iYXJ0LnB5) | `96.48% <100.00%> (-0.02%)` | :arrow_down: |
| [src/transformers/modeling\_t5.py](https://codecov.io/gh/huggingface/transformers/pull/3739/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190NS5weQ==) | `82.77% <100.00%> (-0.44%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=footer). Last update [7a7fdf7...0de2191](https://codecov.io/gh/huggingface/transformers/pull/3739?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


In general, I think we have to be careful with a distinction between the different special token ids. 
I can see why `decoder_input_token_id` looks weird at first glance, but in #3225 and #3140, we decided to add it to keep Bart's good performance on summarization. 

I don't really see the need to overwrite `input_ids` with `prefix_ids` - do we have to do this? 
I would be ok with adding an optional `decoder_input_ids` that would be used for encoder-decoder models only. 

There are quite a few hidden hacks in `generation()` (like the `force_token_id` fn) that look quite strange. If we replace / delete them, we should always check that the hard-coded integration tests don't fail (running the tests with `Run_SLOW=1` as mentioned above.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.