GPT2 -- build_inputs_with_special_tokens lacking BOS and EOS tokens.
Author: moinnadeem
Pull request: no
Labels: name: wontfix

# 🐛 Bug

## Information

Model I am using (Bert, XLNet ...): GPT-2

Language I am using the model on (English, Chinese ...): English

The problem arises when using:
* [ ] the official example scripts: (give details below)
* [x] my own modified scripts: (give details below)

The tasks I am working on is:
* [ ] an official GLUE/SQUaD task: (give the name)
* [x] my own task or dataset: (give details below)

## To reproduce

Steps to reproduce the behavior:

Script:
```
from transformers import GPT2Tokenizer

tokenizer = GPT2Tokenizer.from_pretrained("gpt2")
encoded_dict = tokenizer.encode_plus(text="Hello I am Moin", add_special_tokens=True, \
    max_length=512, truncation_strategy="longest_first", pad_to_max_length=False, \
    return_tensors=None, return_token_type_ids=True, return_attention_mask=True, \
    return_overflowing_tokens=False, return_special_tokens_mask=False)

print(tokenizer.bos_token_id)
print(encoded_dict['input_ids'])
```

You should see that the `input_ids` do not include the `bos_token_id`. Shouldn't `encode_plus` be doing this?

<!-- If you have code snippets, error messages, stack traces please provide them here as well.
     Important! Use code tags to correctly format your code. See https://help.github.com/en/github/writing-on-github/creating-and-highlighting-code-blocks#syntax-highlighting
     Do not use screenshots, as they are hard to read and (more importantly) don't allow others to copy-and-paste your code.-->

## Expected behavior
The 
 token would appear, since I included to `add_special_tokens`.

## Environment info
<!-- You can run the command `python transformers-cli env` and copy-and-paste its output below.
     Don't forget to fill out the missing fields in that output! -->
     
- `transformers` version:
- Platform: Linux-4.15.0-54-generic-x86_64-with-debian-buster-sid
- Python version: 3.7.2
- PyTorch version (GPU?): 1.3.1 (False)
- Tensorflow version (GPU?): not installed (NA)
- Using GPU in script?: No
- Using distributed or parallel set-up in script?: No


Comments:
Hi @moinnadeem, 

Thanks for posting this! 
As it is implemented in the moment, you are right, GPT2 Tokenizer does not add the BOS in the beginning nor the EOS token in the end. 
You can see e.g. that the XLNet tokenizer has a method that adds special tokens to the encoded input string (see https://github.com/huggingface/transformers/blob/4e4403c9b44324671cb795df2ef30e70fe3b606e/src/transformers/tokenization_xlnet.py#L241), whereas the GPT2 tokenizer does not have such a function and thus uses the default one which does not add any special tokens. 

As far as I can see this could be a feature request, where a `build_inputs_with_special_tokens()` would be added to `tokenization_gpt2.py`. 

The expected behavior could be:
input_string -> BOS + encoded(input_string) + EOS in the case of GPT2. 

Feel free to open a PR to include this feature :-) In the meantime you can obviously just manually add the BOS and EOS token before encoding. 

@mfuntowicz do you think such a PR would make sense? 

I don't think this has been fixed, right?

It's not really a bug because the default behavior of GPT2 is to just not add bos or eos tokens. GPT2 is mainly used to generate text so it would not make a lot of sense to add a EOS of a input prompt. If one wants he could just manually add `gpt2_tokenizer.eos_token` to the input and the eos_token_id will be added

> It's not really a bug because the default behavior of GPT2 is to just not add bos or eos tokens. GPT2 is mainly used to generate text so it would not make a lot of sense to add a EOS of a input prompt. If one wants he could just manually add `gpt2_tokenizer.eos_token` to the input and the eos_token_id will be added

I think in the original GPT2 model, there *are* special tokens for bos and eos, both of which are `
`, right? So if I want to finetune it, we should do the same thing -- add both bos and eos to the corpus for finetune, right?

@zhujl1991 - yes this is correct. 
We also set bos and eos token to `
` for GPT2 as you can verify as follows:

```python
from transformers import GPT2Tokenizer
tok = GPT2Tokenizer.from_pretrained("gpt2")
print(tok.eos_token)
print(tok.bos_token)
```

However, I don't think we plan on adding these tokens automatically when tokenizing an input string because the main use case for GPT2 is open-domain text generation where these tokens should not be added.
I agree that they could /should be added for fine-tuning. 

So I'm not sure if we want to add any special "fine-tune" behavior to the GPT2Tokenizer. @LysandreJik - what do you think?



> @zhujl1991 - yes this is correct.
> We also set bos and eos token to `
` for GPT2 as you can verify as follows:
> 
> ```python
> from transformers import GPT2Tokenizer
> tok = GPT2Tokenizer.from_pretrained("gpt2")
> print(tok.eos_token)
> print(tok.bos_token)
> ```
> 
> However, I don't think we plan on adding these tokens automatically when tokenizing an input string because the main use case for GPT2 is open-domain text generation where these tokens should not be added.
> I agree that they could /should be added for fine-tuning.
> 
> So I'm not sure if we want to add any special "fine-tune" behavior to the GPT2Tokenizer. @LysandreJik - what do you think?

The behavior of "set add_special_tokens to True but no special tokens are added while there are special tokens in the tokenizer" looks like a bug to me anyway. If the user doesn't want to add special tokens when tokenizing, e.g., as you said, when generating text, the user should set add_special_tokens to False.

I see what you mean @zhujl1991 -> Thinking about backwards compatibility and that by default `add_special_tokens` is set to `True`, I still do not think that we should add this feature to the `__call__` or `encode_plus` functions for GPT2. On the other hand such a functionality would be very useful for training/fine-tuning.

I see three options:

1) overwrite the __call__ method in GPT2 to have add_special_tokens=`False` by default and append BOS and EOS if set to `True` => I don't like this option as it's quite hacky and would still not be 100% backward compatible

2) Add a new method `prepare_for_training` where the input is prepared for fine-tuning / training as you said.

3) Don't do anything about it and let the user overwrite such a method himself. 

I would be fine with option 2), but also don't think it's that important of a feature (option 3))....let's see what @LysandreJik @sgugger, @thomwolf  and @sshleifer think

IMO this is something that should be written by the user for their specific needs (option 3). We can document more that the tokenizers are pre-set for the most common tasks the corresponding models are used for, to avoid any user being too surprised.

I feel that if we add a method, it will cover some use cases but not all and it will either be overly too complex or only used by a small percentage of the users.

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.


Ran into this too – this seems like a bug to me, or at the least not intuitive behaviour.

If there's a tokeniser that has an EOS token, and I encode with `add_special_tokens=True`, I'd expect it to include the eos token at the end of sentence. 

+1 on this.

The main issue here is really how opaque and unintuitive this has been for me. Here's my thought process:

```
from transformers import GPT2TokenizerFast
gpt2_tok = GPT2TokenizerFast.from_pretrained("gpt2")
gpt2_tok("Mary had a little lamb")["input_ids"]
# prints [24119, 550, 257, 1310, 19343]
```
Mh, weird, no special tokens? I've used HF before and I thought the default was to add them?

Then I went and looked up the function, and indeed the default is to have them on. Bah, whatevs. Let's explicitly pass it as on:

```
gpt2_tok("Mary had a little lamb", add_special_tokens=True)["input_ids"]
# No difference :D
```
This part is what got me massively confused. I think it's entirely fine to change the default behavior for GPT-2 if the majority of the users don't care/want those tokens, but it would be more intuitive to change the default to add_special_tokens=False, and actually add the special tokens when the option is passed explicitly! :)


I had some thoughts over this question too.
In the end, I realized that the model has been trained using "full paragraphs/articles of text", which means that spaces and new line symbols were part of the training. The 
 token was added between  paragraphs/articles.
So the 
  should only be added at the beginning and end of text paragraphs/articles for fine-tuning, but it seems to be a detail since in fact, it is just a kind of text formatting.
For text generation, usually you think of the "end of text" as a punctuation mark or newline character not  as the  
 token which denotes the end of a paragraph/article.
So I think that the code is perfectly right.


@patrickvonplaten 

Hi, I also believe that BOS should be prepended before an input sentence (w1, w2, ...) for two reasons:

1. Without BOS, the model cannot calculate the probability of generating the first token, i.e. P(w1|BOS).
2. BOS also affects the probability of generating the following words, e.g. P(w2|w1) != P(w2|w1, BOS).

For the second point, see the following example:

```
import torch
from transformers import GPT2Tokenizer, GPT2LMHeadModel

tokenizer = GPT2Tokenizer.from_pretrained('gpt2')
model = GPT2LMHeadModel.from_pretrained('gpt2')
inputs = tokenizer("
This", return_tensors="pt")
# inputs: {'input_ids': tensor([[50256,  1212]]), 'attention_mask': tensor([[1, 1]])}
outputs = model(**inputs, labels=inputs["input_ids"])
tokenizer.convert_ids_to_tokens(outputs.logits[0][1].topk(20)[1])
# ['Ġis', 'Ġarticle', 'Ġpost', 'Ġweek', 'Ġpage', 'Ġstory', 'Ġyear', 'Ġwas', 'Ġmonth', 'Ġsite', 'Ġbook', 'Ġpast', 'Ġitem', 'Ġproject', 'Ġblog', 'Ġstudy', 'Ġsection', 'Ġmorning', 'Ġvideo', 'Ġgame']

inputs = tokenizer("This", return_tensors="pt")
# {'input_ids': tensor([[1212]]), 'attention_mask': tensor([[1]])}
outputs = model(**inputs, labels=inputs["input_ids"])
tokenizer.convert_ids_to_tokens(outputs.logits[0][0].topk(20)[1])
# ['Ġis', ',', '.', 'Ċ', "'s", 'Ġwas', 'Ġto', 'Ġand', 'Ġthe', 'Ġin', 'Ġhas', 'Ġof', 'Ġwill', 'Ġa', ':', 'Ġare', 'Ġcan', 'Ġ(', '-', 'Ġfor']
```
Comparing these two generations, the prediction with "
" seems more accurate (e.g. Without BOS, some punctuations are predicted as the next word of "This").

Due to the lack of documentation, I am not entirely sure if the "
" token is actually used as a BOS token during training, but the following example suggests it may be the case.

```
inputs = tokenizer("
", return_tensors="pt")
outputs = model(**inputs, labels=inputs["input_ids"])
tokenizer.convert_ids_to_tokens(outputs.logits[0][0].topk(20)[1])

# ['Ċ', 'The', '"', 'A', 'I', 'In', '.', 'It', 'S', 'This', 'B', '-', 'C', 'We', '1', 'T', "'", 'P', '(', 'G']
```

Even if you opt not to prepend BOS, I believe these things should be clarified more in the documentation.

To add confirmation that `
` is also a BOS token, the official repo uses it for inference as well: https://github.com/openai/gpt-2/blob/a74da5d99abaaba920de8131d64da2862a8f213b/src/generate_unconditional_samples.py#L60

Would be nice to add some documentation on this in the GPT2Tokenizer [docs](https://huggingface.co/docs/transformers/model_doc/gpt2#transformers.GPT2Tokenizer).

(Self-note for future PR) 

@patrickvonplaten 
> However, I don't think we plan on adding these tokens automatically when tokenizing an input string because the main use case for GPT2 is open-domain text generation where these tokens should not be added.

Hi.
I'd like to ask you a question.
Could you explain how the model want to stop generation if there's no EOS token?

> @patrickvonplaten
> 
> > However, I don't think we plan on adding these tokens automatically when tokenizing an input string because the main use case for GPT2 is open-domain text generation where these tokens should not be added.
> 
> Hi. I'd like to ask you a question. Could you explain how the model want to stop generation if there's no EOS token?

I'm trying to train the model with EOS tokens at the end. Let's see if that works...

Shouldn't the EOS tokens be set by default when we use `DataCollatorForLanguageModeling(..., mlm=False)`? It makes sense to me that they should. If not, **at least** [this documentation](https://huggingface.co/docs/transformers/tasks/language_modeling) should be changed and the EOS token should be added at the end of each raw text.



> > @patrickvonplaten
> > > However, I don't think we plan on adding these tokens automatically when tokenizing an input string because the main use case for GPT2 is open-domain text generation where these tokens should not be added.
> > 
> > 
> > Hi. I'd like to ask you a question. Could you explain how the model want to stop generation if there's no EOS token?
> 
> I'm trying to train the model with EOS tokens at the end. Let's see if that works...
> 
> Shouldn't the EOS tokens be set by default when we use `DataCollatorForLanguageModeling(..., mlm=False)`? It makes sense to me that they should. If not, **at least** [this documentation](https://huggingface.co/docs/transformers/tasks/language_modeling) should be changed and the EOS token should be added at the end of each raw text.

I agree with you. I really don't understand how CLM train without EOS token.

> I'm trying to train the model with EOS tokens at the end. Let's see if that works...

this worked for me, but I really had to make sure that the EOS token was always at the end of each sequence

> this worked for me, but I really had to make sure that the EOS token was always at the end of each sequence

Do you mean the inference is working? How the model decide to stop generate if there's no EOS token or there're multiple EOS tokens when they concat sequences as mentioned in [this documentation](https://huggingface.co/learn/nlp-course/chapter7/6#preparing-the-dataset)?


> > this worked for me, but I really had to make sure that the EOS token was always at the end of each sequence
> 
> Do you mean the inference is working? How the model decide to stop generate if there's no EOS token or there're multiple EOS tokens when they concat sequences as mentioned in [this documentation](https://huggingface.co/learn/nlp-course/chapter7/6#preparing-the-dataset)?

Tokenizers used in causal models don't append the EOS token by default, while the ones in encoder-decoder (like T5) do.
```
t5_tokenizer("My name is Sarah and I live in London")
Out[7]: {'input_ids': [499, 564, 19, 8077, 11, 27, 619, 16, 1524, 1], 'attention_mask': [1, 1, 1, 1, 1, 1, 1, 1, 1, 1]}
t5_tokenizer.eos_token_id
Out[8]: 1
gpt2_tokenizer("My name is Sarah and I live in London")
Out[9]: {'input_ids': [3666, 1438, 318, 10490, 290, 314, 2107, 287, 3576], 'attention_mask': [1, 1, 1, 1, 1, 1, 1, 1, 1]}
gpt2_tokenizer.eos_token_id
Out[10]: 50256
```
To make the model generate EOS tokens at inference time, I had to tokenize my texts and then add the EOS token at the end, like: `tokenized_texts = tokenizer([t + tokenizer.eos_token for t in texts])`

If the tokenizer doesn't even have an EOS token, then you may have to create a new one, or rely on some heuristics to stop the generation.

Expect to see more GPT2-related fine-tuning cases in transformers doc.