Fix GPT2DoubleHeadsModel to work with model.generate()
Author: LSinev
Pull request: yes
Labels: name: model card

`token_type_ids` were not passed through `model_specific_kwargs` nor cut properly with `use_cache`/`past` usage (maybe this should be corrected through bigger PR for all models available for caching).

Comments:
I do not know what happened on your PR, any way you can restore or rebase/force-push for the diff to be readable?

force-pushed

@julien-c Rebased onto latest commit in master (https://github.com/huggingface/transformers/commit/f72fe1f31aca235c7f675680832cc364efe4088e)

Don't think the change to `past[-1]` is applicable to all models. We should enforce the use of `return_dict=True` in TF as well to simplify this behaviour

> Don't think the change to `past[-1]` is applicable to all models.

@patrickvonplaten removed this commit.

# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/6601?src=pr&el=h1) Report
> Merging [#6601](https://codecov.io/gh/huggingface/transformers/pull/6601?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/26d5475d4b6644528956df3020dbaa436b443706?el=desc) will **increase** coverage by `5.22%`.
> The diff coverage is `80.00%`.

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

```diff
@@            Coverage Diff             @@
##           master    #6601      +/-   ##
==========================================
+ Coverage   75.91%   81.13%   +5.22%     
==========================================
  Files         195      168      -27     
  Lines       39827    32288    -7539     
==========================================
- Hits        30233    26198    -4035     
+ Misses       9594     6090    -3504     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/6601?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_gpt2.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19ncHQyLnB5) | `86.66% <80.00%> (-1.32%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_electra.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9lbGVjdHJhLnB5) | `25.13% <0.00%> (-71.56%)` | :arrow_down: |
| [src/transformers/tokenization\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fYmFydC5weQ==) | `42.10% <0.00%> (-57.90%)` | :arrow_down: |
| [src/transformers/tokenization\_camembert.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fY2FtZW1iZXJ0LnB5) | `37.03% <0.00%> (-53.13%)` | :arrow_down: |
| [src/transformers/tokenization\_mbart.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fbWJhcnQucHk=) | `36.50% <0.00%> (-39.33%)` | :arrow_down: |
| [src/transformers/tokenization\_funnel.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fZnVubmVsLnB5) | `62.79% <0.00%> (-37.21%)` | :arrow_down: |
| [src/transformers/modeling\_lxmert.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19seG1lcnQucHk=) | `70.01% <0.00%> (-20.72%)` | :arrow_down: |
| [src/transformers/trainer\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90cmFpbmVyX3V0aWxzLnB5) | `60.29% <0.00%> (-20.48%)` | :arrow_down: |
| [src/transformers/modeling\_mobilebert.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19tb2JpbGViZXJ0LnB5) | `79.21% <0.00%> (-10.22%)` | :arrow_down: |
| [src/transformers/modeling\_openai.py](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19vcGVuYWkucHk=) | `72.25% <0.00%> (-10.00%)` | :arrow_down: |
| ... and [157 more](https://codecov.io/gh/huggingface/transformers/pull/6601/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/6601?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/6601?src=pr&el=footer). Last update [26d5475...8ffb4c0](https://codecov.io/gh/huggingface/transformers/pull/6601?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Hey @LSinev, 

I think the `token_type_ids` would have to be increased by 1 at each generation step. With the current generation implementing this logic is quite tedious - I think we should put this PR on hold until generation is properly refactored. Will try to get to this within the next 3,4 weeks. Hope that's fine!


@LSinev I think this implementation is what I need! (haven't tried it)
For my purpose, I would have a `context` (`token_type_ids`=0), a `prompt token` (`token_type_ids`=1) followed by prediction (`token_type_ids`=1).
With this PR, seems like I can pass in `context` +  `prompt token` with `token_type_ids` and the last `token_type_ids` will be reused in the generated, which is what I want.

> I think the `token_type_ids` would have to be increased by 1 at each generation step

 @patrickvonplaten Do you mean `position_ids`?
 I just got an idea. What if we allow users to pass in a function for `prepare_inputs_for_generation`, so users can easily customize the behavior? 🤔 💥

> like

That would work with `past`, but would break if you set `use_cache=False` because `token_type_ids` length's is not increased. I can see that this feature is required. However, I want to wait until the refactoring is done before adding more functionality to the general `generate()` method. 

Adding a function to `prepare_input_ids` is a bit tooo hacky for me as well (we would need to add a function arg to `generate()`)

Was just curious what was the status of this PR? It seems all the checks have passed

> > like
> 
> That would work with `past`, but would break if you set `use_cache=False` because `token_type_ids` length's is not increased. I can see that this feature is required. However, I want to wait until the refactoring is done before adding more functionality to the general `generate()` method.
> 
> Adding a function to `prepare_input_ids` is a bit tooo hacky for me as well (we would need to add a function arg to `generate()` which I don't really like

Sorry to only get back you guys now. So having refactored the `generate()` method, I think we can should add an `if token_type_ids in model_kwargs` here: https://github.com/huggingface/transformers/blob/70708cca1a2f3e01d9d72a3aaa7ab078dfef639e/src/transformers/generation_utils.py#L184 to deal with this case. The way it's implemented now does not really work as explained in the message above.

Maybe I should add changes to `GPT2ForSequenceClassification` too? Or maybe `GPT2PreTrainedModel` should have widest `prepare_inputs_for_generation` for all inherited models?

Tests added. Not a good showcase (because the model is not finetuned to use `token_type_ids` in the way like https://github.com/huggingface/transfer-learning-conv-ai), but at least it tests that `token_type_ids` are used (and expanded if `num_return_sequences` passed) and affect output. Not sure if slow tests are tested online at PR submission, though.

Tests are great! It's fine if it's not a good showcase - as long as we make sure the functionality works and will work in the future this is great! 

Great job @LSinev  and thanks for bearing with me throughout this PR :-)