Llama tokenizer add_prefix_space
Author: jiqing-feng
Pull request: yes

Hi @sgugger 

This PR enables llama tokenizer supporting `add_prefix_space`.  

Would you please help me review it? Thanks!

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_25278). All of your documentation changes will be reflected on that endpoint.

Hi @sgugger , I have the same request here. My problem is as follows:  

"\nObservation" is a substring of "!\nObservation", but in the encoded version by the `LlamaTokenizerFast` tokenizer, it is not the case anymore. This can be solved if we enable passing the `add_prefix_space` parameter to the tokenizer. 

Here is my code:
```python
from transformers import AutoTokenizer

model_name = 'lmsys/vicuna-13b-v1.3'
tokenizer = AutoTokenizer.from_pretrained(model_name, add_special_tokens=False, padding=True, use_fast=True)
print(tokenizer)
for stop_word in ['\nObservation', '!\nObservation']:
    print(f'++++++++++{stop_word}+++++++++++++')
    tokens = tokenizer.tokenize(stop_word, add_special_tokens=False)
    print(tokens)
    ids = tokenizer.convert_tokens_to_ids(tokens)
    print(ids) 
```

And here is the output:
```bash
You are using the default legacy behaviour of the <class 'transformers.models.llama.tokenization_llama.LlamaTokenizer'>. This means that tokens that come after special tokens will not be properly handled. We recommend you to read the related pull request available at https://github.com/huggingface/transformers/pull/24565, and set the legacy attribute accordingly.
LlamaTokenizerFast(name_or_path='lmsys/vicuna-13b-v1.3', vocab_size=32000, model_max_length=2048, is_fast=True, padding_side='right', truncation_side='right', special_tokens={'bos_token': AddedToken("
", rstrip=False, lstrip=False, single_word=False, normalized=True), 'eos_token': AddedToken("
", rstrip=False, lstrip=False, single_word=False, normalized=True), 'unk_token': AddedToken("<unk>", rstrip=False, lstrip=False, single_word=False, normalized=True), 'pad_token': '<unk>'}, clean_up_tokenization_spaces=False)
++++++++++
Observation+++++++++++++
['▁', '<0x0A>', 'Ob', 'serv', 'ation']
[29871, 13, 6039, 2140, 362]

++++++++++!
Observation+++++++++++++
['▁!', '<0x0A>', 'Ob', 'serv', 'ation']
[1738, 13, 6039, 2140, 362]
```

As you can see, [29871, 13, 6039, 2140, 362] is not a subset of [1738, 13, 6039, 2140, 362] anymore. This is because the LlamaTokenizerFast always adds a prefix space before a word. 



cc @ArthurZucker 

Hey! Not entirely I understand the example you provided @faany, you say that `"\nObservation" is a substring of "!\nObservation",` but I why? 

I am actually going to add this argument (following #25224 ), as we now control the addition of the SPIECE_UNDERLINE by hand. 


@ArthurZucker maybe a bit more context to my example above: I want my model to stop generating when the string `\nObservation` occurs and  I used a self-defined StoppingCriteria for it. So when the model generates "\nObservation", it should stop generating new texts. But what I found is that my model doesn't stop generating, even though it produces "\nObservation". So I dig a bit deeper into this and found that it produces a `!` in front of `\nObservation` and the token ids of `\nObservation` are not the same as the token ids of `\nObservation` in `!\nObservation`. 

If the token id of SPIECE_UNDERLINE disappears from [29871, 13, 6039, 2140, 362], it will work, because [13, 6039, 2140, 362] is now a sub-list of [1738, 13, 6039, 2140, 362]. 



> Hey! Not entirely I understand the example you provided @Faany, you say that `"\nObservation" is a substring of "!\nObservation",` but I why?
> 
> I am actually going to add this argument (following #25224 ), as we now control the addition of the SPIECE_UNDERLINE by hand.

Hi @ArthurZucker . Thanks for your reply. I use your patch [25224]( https://github.com/huggingface/transformers/pull/25224) and set `legacy=False; add_dummy_prefix=False`, but the issue proposed by @faaany still exists.

```
#########
Observation##########
['▁', '<0x0A>', 'Ob', 'serv', 'ation']
[29871, 13, 6039, 2140, 362]

Observation

#########"
Observation##########
['▁"', '<0x0A>', 'Ob', 'serv', 'ation']
[376, 13, 6039, 2140, 362]
"
Observation
```

We see that `\nObservation` and `"\nObservation` are still recognized as two different tokens. We want the `token_ids` of `\nObservation` to be the sub-list of the `token_ids` of  `"\nObservation`, so I submitted this PR. The issue can be solved with my PR. The results are as follows
```
#########
Observation##########
['<0x0A>', 'Ob', 'serv', 'ation']
[13, 6039, 2140, 362]

Observation

#########"
Observation##########
['"', '<0x0A>', 'Ob', 'serv', 'ation']
[29908, 13, 6039, 2140, 362]
"
Observation
```

The patch does not include `add_dummy_prefix_space` yet, so it's quite normal that the dummy prefix space is sill added. Once it is supported, the patch will behave similarly as this PR 😉

Now the issue that you mention:
>  the token ids of \nObservation are not the same as the token ids of \nObservation in !\nObservation

It's expected no? They are different tokens. What you need to do is actually stop the generation when `\nObserbation` is generated. This means that the stopping criteria should look like this:
```python
from transformers import StoppingCriteria

class VicunaStoppingCriteria(StoppingCriteria):
    def __init__(self, eos_sequence = [13, 6039, 2140, 362]):
        self.eos_sequence = eos_sequence

    def __call__(self, input_ids: torch.LongTensor, scores: torch.FloatTensor, **kwargs) -> bool:
        last_2_ids = input_ids[:,-2:].tolist()
        return self.eos_sequence in last_2_ids
``` 
Because the actual tokenization of `\nObservation` is `[13, 6039, 2140, 362]`:
```python 
>>> tokenizer.sp_model.decode([13, 6039, 2140, 362])
>>> '\nObservation'
```

> The patch does not include `add_dummy_prefix_space` yet, so it's quite normal that the dummy prefix space is sill added. Once it is supported, the patch will behave similarly as this PR 😉
> 
> Now the issue that you mention:
> 
> > the token ids of \nObservation are not the same as the token ids of \nObservation in !\nObservation
> 
> It's expected no? They are different tokens. What you need to do is actually stop the generation when `\nObserbation` is generated. This means that the stopping criteria should look like this:
> 
> ```python
> from transformers import StoppingCriteria
> 
> class VicunaStoppingCriteria(StoppingCriteria):
>     def __init__(self, eos_sequence = [13, 6039, 2140, 362]):
>         self.eos_sequence = eos_sequence
> 
>     def __call__(self, input_ids: torch.LongTensor, scores: torch.FloatTensor, **kwargs) -> bool:
>         last_2_ids = input_ids[:,-2:].tolist()
>         return self.eos_sequence in last_2_ids
> ```
> 
> Because the actual tokenization of `\nObservation` is `[13, 6039, 2140, 362]`:
> 
> ```python
> >>> tokenizer.sp_model.decode([13, 6039, 2140, 362])
> >>> '\nObservation'
> ```

Thanks for the explanation. I see your point, but what if I want to pass the actual stop word in a string, e.g. `\nObservation`, into `VicunaStoppingCriteria` instead of the `eos_sequence` in your example? 

```python
from transformers import StoppingCriteria

class VicunaStoppingCriteria(StoppingCriteria):
    def __init__(self, tokenizer, stop_words = ["\nObservation"]):
        encoded_stop_words = tokenizer(stop_words, add_special_tokens=False, padding=True, return_tensors="pt")
        self.stop_words = encoded_stop_words.input_ids.to('cpu')

    def __call__(self, input_ids: torch.LongTensor, scores: torch.FloatTensor, **kwargs) -> bool:
        last_2_ids = input_ids[:,-2:].tolist()
        return self.stop_words in last_2_ids
```

Then the VicunaStoppingCriteriawould will return False due to the added prefix space.

@ArthurZucker  Maybe the way how I define my `StoppingCriteria` is not recommended in general?  

Yes @faaany, I don't think using a string is the best way to go for two reasons: 
1. The `generate` function does not have access to the tokenizer
2. If you encode `"\nObservation"`, as you noticed, whether there is a prefix space or not will change the encoding. You can mitigate this using the `add_dummy_prefix` indeed, but generally it's better to have the actual ids. 

But I'll make sure to include the `add_prefix_space` argument anyway I think it's important. 

> Yes @faaany, I don't think using a string is the best way to go for two reasons:
> 
> 1. The `generate` function does not have access to the tokenizer
> 2. If you encode `"\nObservation"`, as you noticed, whether there is a prefix space or not will change the encoding. You can mitigate this using the `add_dummy_prefix` indeed, but generally it's better to have the actual ids.
> 
> But I'll make sure to include the `add_prefix_space` argument anyway I think it's important.

Awesome, thank you! Since @jiqing-feng  has tested that the current PR #25224 still doesn't support the argument `add_prefix_space`, can we leave this PR open for the `add_prefix_space` argument? Once your PR gets merged with the support, we will be glad to test it again and we can close this one.  

Hi @ArthurZucker , I saw that you have merged the other PR without the new `add_prefix_space` argument. Would you consider this PR from @jiqing-feng then? Or do you have any other plans or thoughts? 

For now I will not consider this PR as it is very much incomplete, especially for the fast tokenization. 
- If you use a slow tokenizer, the prefix space is added in `prepare_for_tokenization`, which means you can use `tokenizer._tokenizer` to achieve what you are looking for. 
- If you are using a fast tokenizer, you will need to modify the backend tokenizer, and if you test your script, it will not work for now. You need to change the state of the `fast_tokenizer.backen_tokenizer`, to update the `normalizer` and the `decoder`. This is a bit tricky for now, and I would recommend going through another road: use a fast tokenizer and don't lookup strings but rather inputs ids. 

> For now I will not consider this PR as it is very much incomplete, especially for the fast tokenization.
> 
> * If you use a slow tokenizer, the prefix space is added in `prepare_for_tokenization`, which means you can use `tokenizer._tokenizer` to achieve what you are looking for.
> * If you are using a fast tokenizer, you will need to modify the backend tokenizer, and if you test your script, it will not work for now. You need to change the state of the `fast_tokenizer.backen_tokenizer`, to update the `normalizer` and the `decoder`. This is a bit tricky for now, and I would recommend going through another road: use a fast tokenizer and don't lookup strings but rather inputs ids.

Thanks a lot for the explanation! Just to make sure that I understood  you correctly, do you mean that I can use the slow tokenizer instead of fast tokenizer to achieve my goal, e.g. like this:

```python
from transformers import AutoTokenizer

model_name = 'lmsys/vicuna-13b-v1.3'
tokenizer = AutoTokenizer.from_pretrained(model_name, add_special_tokens=False, padding=True, use_fast=False)

for stop_word in ['Observation', '!\nObservation']:
    print(f'++++++++++{stop_word}+++++++++++++')
    tokens = tokenizer._tokenize(stop_word)
    print(tokens)
    ids = tokenizer.convert_tokens_to_ids(tokens)
    print(ids) 
```

But the output I got for the above code is this:

```shell
++++++++++Observation+++++++++++++
['▁Observ', 'ation']
[21651, 362]
++++++++++!
Observation+++++++++++++
['▁!', '<0x0A>', 'Ob', 'serv', 'ation']
[1738, 13, 6039, 2140, 362]
```

I mean that the way you wanted to use the fast tokenizer would not have worked unless you convert it again (so the `tokenization_llama_fast` is wrong yes. 
For the snippet did you set `legacy = False`? 

> I mean that the way you wanted to use the fast tokenizer would not have worked unless you convert it again (so the `tokenization_llama_fast` is wrong yes. For the snippet did you set `legacy = False`?

Ahh, thanks for the hint, it worked with `legacy=False`!!

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.