🚨 🚨 🚨 Fix Issue 15003: SentencePiece Tokenizers Not Adding Special Tokens in `convert_tokens_to_string`
Author: beneyal
Pull request: yes

Hello! 👋 

This PR fixes issue #15003 and is also related to issue #11716.

First, I've expanded the `test_sentencepiece_tokenize_and_convert_tokens_to_string` test in `test_tokenization_common.py` which, in addition to checking a round-trip from string to tokens and back, performs a check whether the tokens corresponding to `input_ids` (using `convert_ids_to_tokens`) followed by `convert_tokens_to_string` and then re-tokenized, give the same number of tokens as `input_ids`. This is because not all SentencePIece tokenizers have a `*Fast` flavor, and so I couldn't compare behaviors similar to the `T5Tokenizer` test.

This test failed for the following tokenizers:
- `AlbertTokenizer`
- `BarthezTokenizer`
- `CamembertTokenizer`
- `DebertaV2Tokenizer`
- `FNetTokenizer`
- `LayoutXLMTokenizer`
- `M2M100Tokenizer`
- `MarianTokenizer`
- `MBart50Tokenizer`
- `PegasusTokenizer`
- `Speech2TextTokenizer`


Most of these tokenizers had the same implementation for `convert_tokens_to_string`:

```python
def convert_tokens_to_string(self, tokens):
    return self.sp_model.decode(tokens)
```

All tokenizers who had this implementation, or something that can be reduced to it, are now inheriting `SentencePieceStringConversionMixin` (defined in `tokenization_utils.py`), and the implementation uses the same code as PR #8435. The rest have a variation of this code, and perhaps a future refactor can DRY it a bit.

The only tokenizer that doesn't pass the test, and is therefore excluded from it, is `LayoutXLMTokenizer`, whose `convert_tokens_to_string` method doesn't use the SentencePiece model in its implementation, and so I figured the lack of special tokens is intentional, rather than a byproduct of SentencePiece like the rest.

I think this PR is relevant for @NielsRogge who initially opened the issue on `AlbertTokenizer`, and following the PR guidelines, I think this is also relevant to @LysandreJik and @patrickvonplaten.

This has been a very fun project, and hopefully you will allow me to contribute more in the future 🙂 

Thanks,
Ben

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

_The documentation is not available anymore as the PR was closed or merged._

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.

GitHub Actions, stop marking this PR as stale! 😅 

Hi @SaulLu, how are you? I started a new job so kind of lost track with this PR, is it still relevant? I'd really like to see this PR merged at some point 😁 

Hi @beneyal ! 

Congrats for your new job :confetti_ball: 

Yes it's still relevant: I'm also sorry, I can't find the time to propose the last fixes to merge your PR. I think it would be good to 1) [standardize between tokenizers with only a slow version and the one with a slow and a fast](https://github.com/huggingface/transformers/pull/15775#discussion_r864030843) and also 2)  to find a solution to [avoid hardcoded values in Deberta-v2](https://github.com/huggingface/transformers/pull/15775#discussion_r864034981). 

Sorry, you didn't picked the easier bug to fix :hugs: 

Hi again @SaulLu 🙂 

I found something interesting.

Running this code:

```python
from transformers import AutoTokenizer

tokenizers = [
    (
        "microsoft/deberta-v2-xlarge",
        AutoTokenizer.from_pretrained("microsoft/deberta-v2-xlarge", use_fast=True),
        AutoTokenizer.from_pretrained("microsoft/deberta-v2-xlarge", use_fast=False),
    ),
    (
        "albert-base-v2",
        AutoTokenizer.from_pretrained("albert-base-v2", use_fast=True),
        AutoTokenizer.from_pretrained("albert-base-v2", use_fast=False),
    ),
    (
        "bert-large-uncased",
        AutoTokenizer.from_pretrained("bert-large-uncased", use_fast=True),
        AutoTokenizer.from_pretrained("bert-large-uncased", use_fast=False),
    ),
    (
        "camembert-base",
        AutoTokenizer.from_pretrained("camembert-base", use_fast=True),
        AutoTokenizer.from_pretrained("camembert-base", use_fast=False),
    ),
]

text = "This is some {} to the tokenizer."

print("| Checkpoint Name | Fast Tokenizer | Slow Tokenizer |")

for ckpt, fast, slow in tokenizers:
    fast_tokens = fast.tokenize(text.format(fast.unk_token))
    slow_tokens = slow.tokenize(text.format(slow.unk_token))
    assert (
        fast_tokens == slow_tokens
    ), f"tokens should be identical in the same model. {ckpt = }, {slow_tokens =}, {fast_tokens = }"
    fast_string = fast.convert_tokens_to_string(fast_tokens)
    slow_string = slow.convert_tokens_to_string(fast_tokens)
    print(f"| {ckpt} | {fast_string} | {slow_string} |")
```

Gives us the following table:

| Checkpoint Name | Fast Tokenizer | Slow Tokenizer |
|---|---|---|
| microsoft/deberta-v2-xlarge | This is some[UNK] to the tokenizer. | This is some[UNK]to the tokenizer. |
| albert-base-v2 | this is some\<unk\> to the tokenizer. | this is some\<unk\>to the tokenizer. |
| bert-large-uncased | this is some [UNK] to the tokenizer. | this is some [UNK] to the tokenizer . |
| camembert-base | This is some\<unk\> to the tokenizer. | This is some\<unk\>to the tokenizer. |


So we can see that DeBERTa v2, Albert and CamemBERT have the same behavior w.r.t UNK:
* Fast tokenizer has no space between a non-special token and UNK
* Slow tokenizer has no spaces on both sides of the UNK

That's weird, because then we would expect DeBERTa v2's `convert_tokens_to_string` code (which I made identical to Albert's) to pass the tests we wrote.

So I made a slight change to the test (`test_tokenization_deberta_v2.py:40`):

```python
# tokenizer = DebertaV2Tokenizer(SAMPLE_VOCAB, unk_token="<unk>")
tokenizer = DebertaV2Tokenizer.from_pretrained(
    "https://huggingface.co/microsoft/deberta-v2-xlarge/resolve/main/spm.model"
)
```

Now the `convert_tokens_to_string` tests pass, and the only failing tests are ones that rely on the size of the test vocabulary and some casing (`<pad>` vs `<PAD>`). Add this to the fact the only DeBERTa v2's test needed to specify a different UNK token, I believe the logic is fine, but something is amiss with the test vocab. What do you think?

Have a lovely weekend!

Hello @SaulLu 🙂 Just a little ping in case you didn't see my written novel of a comment above 😅 Thank you again for your time and have a lovely day!

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.

Hi @SaulLu, GitHub Actions wants to close this issue again 😅 Hope everything is well!

I haven't worked on this since June 11th because I'm waiting for your insights 🙂 

Have a great week!

Hi @beneyal , 

I wanted to leave you a message to let you know that I haven't had time so far to dive into your PR (and I, unfortunately, have no idea what the right answer is by reading your findings) and that unfortunately, I'm going to be off next week.

I appreciate a lot all the efforts you made for this fix, I really hope we will find a satisfactory solution! 

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.

Hi @SaulLu 🙂 Any news? The bot wants to close the issue again 😅 

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.

Hi @SaulLu, how are you? 🙂 

Anything new regarding this PR? 🙏 

This PR is still in progress 🙂 

The Albert changes resolved the lack of `[MASK]` for me. Plans to get this merged?

@sgugger Wonderful to have you on board! 🚀 

I can't comment on the Deberta snippet, so I'll try here: we basically want the code for Deberta V2 be the same as the code for the other tokenizers like Albert, i.e., without the special `seen_unk` hack?

I also changed the title as requested 🙂 

Yes, that would be great! And if you could fix the merge conflicts/rebase on main that would also be awesome!

@sgugger I have a rather silly question: I see in GitHub there are two conflicts: `tests/deberta_v2/test_tokenization_deberta_v2.py` and `tests/layoutxlm/test_tokenization_layoutxlm.py`. When merging `upstream/main` to my branch, there are no conflicts. Regarding rebasing, I've never done it so I'm not familiar with that functionality and what do I rebase into what. Could you please help me with this? 😅 

If you have merged your branch with upstream then just a push should be enough to resolve the conflicts.

To rebase just do this on your branch:
```
git rebase origin/main
```
then you'll need to force-push
```
git push -u origin branch_name --force
```

@sgugger Done 🙂 Whisper's test failed because an HTTP timeout, but the rest seem fine. Deberta V2 is now excluded from the sentencepiece tests, but has the same logic as Albert.

@sgugger Done, thanks for the suggestions!

Thanks again and sorry for the long wait on this PR!

Hi @beneyal First, thank you for working on this. I have a question as we have a CI failure. With the following code snippet, 
```python
from transformers import T5Tokenizer, T5ForConditionalGeneration

tokenizer = T5Tokenizer.from_pretrained("t5-small")
model = T5ForConditionalGeneration.from_pretrained("t5-small")

input_ids = tokenizer("The <extra_id_0> walks in <extra_id_1> park", return_tensors="pt").input_ids

sequence_ids = model.generate(input_ids)
sequences = tokenizer.batch_decode(sequence_ids)
print(sequences)
```
we have different results:

**Before this PR**
```python
['<pad> <extra_id_0> park offers<extra_id_1> the<extra_id_2> park.
']
```

**After this PR**
```python
['<pad><extra_id_0> park offers<extra_id_1> the<extra_id_2> park.
']
```
We no longer have the space between `<pad>` and `<extra_id_0>`. Is this expected? 🙏 Thank you.

@ydshieh Glad to do it!

Yes, this is intended behavior, as it mimics the fast tokenizer:

```python
from transformers import T5Tokenizer, T5TokenizerFast, T5ForConditionalGeneration

tokenizer = T5Tokenizer.from_pretrained("t5-small")
tokenizer_fast = T5TokenizerFast.from_pretrained("t5-small")

model = T5ForConditionalGeneration.from_pretrained("t5-small")

input_ids = tokenizer("The <extra_id_0> walks in <extra_id_1> park", return_tensors="pt").input_ids
inputs_ids_fast = tokenizer_fast("The <extra_id_0> walks in <extra_id_1> park", return_tensors="pt").input_ids

assert input_ids.tolist() == inputs_ids_fast.tolist()

sequence_ids = model.generate(input_ids)

sequences = tokenizer.batch_decode(sequence_ids)
sequences_fast = tokenizer_fast.batch_decode(sequence_ids)

print(sequences, sequences == sequences_fast)  # outputs True
```