Empty Conversation Responses
Author: QuantumEntangledAndy
Pull request: no
Labels: name: wontfix

## Environment info
Mac OS 10.14

My work is in rust, and I have an issue open in guillaume-be/rust-bert#87 however the authour of the repo asked that I also open it here to get HugginFace's opinion as it pertains to code that matches the intention of that in this repo.
     
- `transformers` version: rust-bert 0.10.0
- Platform: Mac OS
- PyTorch version (GPU?): No
- Tensorflow version (GPU?): No
- Using GPU in script?: No
- Using distributed or parallel set-up in script?: Yes

### Who can help
 TextGeneration: @TevenLeScao  (This is in how min_ and max_length are treated during text generation of the conversation model)

## Information

Model I am using: DialgoGPT with the Conversation Moel

The problem arises when using:
* [x] the official example scripts: (give details below)
* [x] my own modified scripts: (give details below)

The problem occurs in the offical example of the rust and the owner of the the rust code assures me that there is exact same behaviour in this code too

The tasks I am working on is:
* [x] my own task or dataset: (give details below)

I am making a small chatbot, I have my own fine tuned model but the same behaviour is obseved with the stock DiagloGPT model

## To reproduce

Steps to reproduce the behavior:

1. Create a conversation model with `min_length` set
2. Talk with the conversation for about 10-12 responses
3. Responses will be zero length despite min_length begin set



## Expected behavior

Min length to be upheld during conversation

## Details

The root cause of this is how cur_len and min/max_len are handled in the code

https://github.com/huggingface/transformers/blob/15a189049e01fbd3bef902848a09f58a5f006c37/src/transformers/generation_utils.py#L86-L88

https://github.com/huggingface/transformers/blob/15a189049e01fbd3bef902848a09f58a5f006c37/src/transformers/generation_utils.py#L533

The cur_len is initalised with the length of current input which contains all previous dialogue with the bot as context

https://github.com/huggingface/transformers/blob/15a189049e01fbd3bef902848a09f58a5f006c37/src/transformers/generation_utils.py#L451

This means that min_length of the new utterence from the bot is already satisfied. It also means that max_length can be exceeded if a long conversation is held.

cur_len should perhaps be initialised differently in the ConverstaionModel

Comments:
Also pinging @patrickvonplaten for info

Hey @QuantumEntangledAndy, I updated the config parameters of all DialoGPT models to `max_length=1000`, see here: https://github.com/huggingface/transformers/issues/7764 -> this problem should now be solved for the DialoGPT models.

I think this is the correct way to tackle this problem - we don't want to change the `generation` code logic here just for Conversational models.

I do not believe this will resolve the issue with min_length. Surly the expected behavior of min_length in a conversation model would be the min length of the new utterence not the length of context + new utterence.

Lets say I set min length to 2, because I want my bot to always say something and I say, "What shall we watch?" This has a length of 4 (five with with eos) this is added into the context and counts towards the length for min_length. The bot therefore has every chance of saying nothing.

What would be the ideal way to deal with this situation?

Sorry I misread your issue a bit here. 
> Surly the expected behavior of min_length in a conversation model would be the min length of the new utterence not the length of context + new utterence

I understand your reasoning here. Are you using the pipelines or the "normal" generate() function?

I think we could tweak the `ConversationPipeline` a bit to better handle the `min_length` parameter. 

 Note that because the `input_ids` get longer with every conversation you have with the bot, `min_length` only works for the very first conversation in pipelines.

If you directly use `generate()` you could just set `min_length=input_ids.shape[-1] + 2` to solve your problem.


I use the ConversationPipeline. I think perhaps the ConversationPipeline should update the min_length on call like this:

```python
min_length=input_ids.shape[-1] + self.min_length_for_response
```

Since we do have such a member in the class although it is set to 32 which seems a little high

https://github.com/huggingface/transformers/blob/0911b6bd86b39d55ddeae42fbecef75a1244ea85/src/transformers/pipelines.py#L2375-L2382

However I think it is high as this is the member that decides how many old responses need to be cleared to make room for new input.

If you want to test this out for yourself and see the blank responses this should work

```python
#! /usr/bin/env python3
"""Small example of conversational pipeline in python."""

from transformers.pipelines import (
    Conversation,
    ConversationalPipeline,
)
from transformers import (
    AutoConfig,
    AutoModelForCausalLM,
    AutoTokenizer,
)

cache_dir = "cached"
model_name_or_path = "microsoft/DialoGPT-medium"
config_name = "microsoft/DialoGPT-medium"
tokenizer_name = "microsoft/DialoGPT-medium"

config = AutoConfig.from_pretrained(
    config_name, cache_dir=cache_dir,
)
tokenizer = AutoTokenizer.from_pretrained(
    tokenizer_name, cache_dir=cache_dir,
)
model = AutoModelForCausalLM.from_pretrained(
    model_name_or_path,
    from_tf=False,
    config=config,
    cache_dir=cache_dir,
)

config.min_length = 2
config.max_length = 1000

print(f"min_length: {config.min_length}")
print(f"max_length: {config.max_length}")

conversation = Conversation()
conversation_manager = ConversationalPipeline(model=model,
                                              tokenizer=tokenizer)

conversation.add_user_input("Is it an action movie?")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("Is it a love movie?")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("What is it about?")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("Would you recommend it?")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("If not what would you recommend?")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("I think you need to think about it more.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("After all action is the best.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("But maybe not.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("What really matters is quality.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("Quality over all other things.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("But not at the expense of tradition.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("For advancement for advancments sake must"
                            " be curtailed.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("Unethical practises must be trimmed.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("In truth nothing is of any good.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("Unless it is traditional.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")

conversation.add_user_input("And sometimes not even then.")
conversation_manager([conversation])
print(f"Response: {conversation.generated_responses[-1]}")
```

> I use the ConversationPipeline. I think perhaps the ConversationPipeline should update the min_length on call like this:
> 
> ```python
> min_length=input_ids.shape[-1] + self.min_length_for_response
> ```
> 
> Since we do have such a member in the class although it is set to 32 which seems a little high
> 
> https://github.com/huggingface/transformers/blob/0911b6bd86b39d55ddeae42fbecef75a1244ea85/src/transformers/pipelines.py#L2375-L2382
> 
> However I think it is high as this is the member that decides how many old responses need to be cleared to make room for new input.

I think I would be fine to add this to the Conversation Pipeline.  Do you want to open a PR and we see how to integrate it? 

Before I do any PR Id like some input on design choices.

- Should I also set max_length?
  
  - If I do set it too I believe it will no longer be necessary to remove old conversations to have room for new content.
  
  - However for very long chats perhaps a chat bot that saves and reloads it's memory this may become computational expensive

  - In light of this would a convenience function that trims memory down to n last inputs be acceptable?

- I am thinking to make min_length an optional parameter to init. It defaults to None, when None is given as input it sets min_length to that of the model at init time. With similar behaviour for max_length

Thank you @QuantumEntangledAndy for sharing the issue here, as I believe it affects both implementation.

If I may, I'd like to add my view on the issue, which I believe is not tied to the `ConversationPipeline`, but rather on how `min_length` and `max_length` and handled for non-"encoder-decoder" architectures.

I would like to question the validity of setting the `cur_len` to the input sequence for pure decoder architectures:
https://github.com/huggingface/transformers/blob/15a189049e01fbd3bef902848a09f58a5f006c37/src/transformers/generation_utils.py#L451.

I would argue for setting the `cur_len` to 1 (or 0?) for pure-decoder architectures as well for a few reasons:
- I would believe that user looking to generate a sequence would be generally looking to set how many tokens they would like to generate, and not the length of the context + generated inputs. It would be great if you could share some use-cases where that is typically not the case.
- This definition of `cur_len` leads to somewhat "hacky" workarounds when the context needs to be extended by a prefix (for example, XLNet). Setting `min_length` and `max_length` to refer to generated content would make it independent of any context pre-processing
- The current behaviour differs between "encoder-decoder" and "decoders", and I am not entirely sure why. A definition based on the length generated would bring the behaviour of both together.
- Lastly, and in my opinion more importantly, the current solution does not work for batched generation. Let's say the input is made of 2 sentences, with initial length of 3 and 8. For batched generation, the input prompt will be padded as follows (`0` indicates a padded token, `x` an input token):
```
[ x x x x x x x x]
[ 0 0 0 0 0 x x x] 
```
the `cur_len` will be set to 8 (`input_ids.shape[-1]`). Let's assume the `min_len` is 12. The model could generate the following sequence (`g` indicates a generated token):
```
[ x x x x x x x x g g g g]
[ 0 0 0 0 0 x x x g g g g] 
```
This shows that while the first sequence respects the `min_len` of 12, the effective length of the second sequence is below the minimum value. Using `min_length` and `max_length` to refer to generated content would lead to valid constraints on all of the sequences in the batch, regardless of padding. For example, with the previous example if `min_length` is 6, both sequence would have at a minimum 6 generated token. In summary, I believe the current handling of `min_length` and `max_length` make them misleading as soon as inputs are passed as batches - but working on generated sequences would prevent that.

I would be more in favor of initializing`cur_len` to 1 (0) for decoders as well - I would be interested in your thoughts on that.

Hmmm I think I see what you mean with batch generation.

I am wondering what the use case of treating non encoder-decoders differently is. If for some reason we cannot reasonably change the definition perhaps we should consider adding different types of new lengths.

- minmax document length
  - This is current definition of min/max
- minmax generated length
  - This is the effective length the model generates.
  - This of course would mean new config variables. But we could default them to none and then ignore that condition in this case.

Hey @guillaume-be,

I fully understand your point of view and I also tend to agree that the handling of `min_length` and `max_length` as described by you and I tend to agree with you (actually @sshleifer suggested this change a while back as well).

I guess the disadvantages of changing `max_length's` logic (making it "max added tokens" vs. "max total tokens") is the following:

- For models like GPT2, users might want to generate multiple articles < 100 words including the prefix. The output of decoder models is always prefix + generated text so it makes sense to me that `max_length` is the maximum length of the output. It's also safer regarding the `max_embedding_positions` provided by the model  - by setting `max_length=512`, there will never be an error independent of the prefix. I guess all arguments are based on the advantages one might have from knowing that `max_length` is independent of the input.

That's the main argument.
- Changing the logic now would break backward compatibility quite heavily. Beam search is highly dependent on max_length *e.g.* - not sure whether we want that.

Let's put @LysandreJik and @yjernite and @sshleifer  in cc as well to see their opinions on changing the `max_length` logic.

We could make it an option like `use_relative` lengths. It defaults to `None` meaning auto (the current behavour) but can be set to `True` of `False` in order to explicitly override the current auto logic. Ultimately I think the auto logic is not perfect and there will always be situations where it choices incorrectly, making it configurable will at least allow the library user to choose the appropiate choice.

My opinion is that I have never seen "users might want to generate multiple articles < 100 words including the prefix", and I have seen, "Why does prefix count?" a number of times and had the same misunderstanding the first few times I used it. So I think we should change the behavior to not count `decoder_start_token_id` or tokens in `input_ids`. 
Another argument that might land: you would expect a function called `generate` to `generate` N tokens. Not generate N tokens where N varies based on your inputs.

The `use_relative` compromise is my second favorite option.
The status quo is my least favorite.

Was there any progress on making these changes?

This issue has been automatically marked as stale and been closed because it has not had recent activity. Thank you for your contributions.

If you think this still needs to be addressed please comment on this thread.