ByT5: problem with tokenizer.decode()
Author: piegu
Pull request: no

## Environment info

- transformers version: 4.11.0
- Platform:  Google Colab
- Python version: 3.7.12
- Using GPU in script?: NO
- Using distributed or parallel set-up in script?: NO

### Who can help

ByT5: @patrickvonplaten 
Documentation: @sgugger 

## Information

Model I am using: `google/byt5-small` (the problem is the same with `google/byt5-base`).

## To reproduce

See this [notebook ](https://colab.research.google.com/drive/1ZS_zPF_ShLU0SKVLt5zYNHqoPOenBkEN?usp=sharing&authuser=1#scrollTo=PiKc6U3atGoh) that shows the problem when using `google/byt5-small` from the model hub of Hugging Face and the `tokenizer.decode()` method, when the `transformers `version is 4.11.0.

The problem does not appear with the `transformers `version 4.9.2 for example.

```
from transformers import T5ForConditionalGeneration, ByT5Tokenizer

model_checkpoint = 'google/byt5-small'
model = T5ForConditionalGeneration.from_pretrained(model_checkpoint)
tokenizer = ByT5Tokenizer.from_pretrained(model_checkpoint)

texts = ["Life is like a box of chocolates.", "Today is Monday."]

for text in texts:
  inputs = tokenizer(text, padding="longest", return_tensors="pt")
  output = model.generate(**inputs)
  print(tokenizer.decode(output[0], skip_special_tokens=True, clean_up_tokenization_spaces=True))
```

Error:

```
---------------------------------------------------------------------------
UnicodeDecodeError                        Traceback (most recent call last)
<ipython-input-8-6f8451a23561> in <module>()
      6       output[0],
      7       skip_special_tokens=True,
----> 8       clean_up_tokenization_spaces=True
      9       )
     10   )

2 frames
/usr/local/lib/python3.7/dist-packages/transformers/models/byt5/tokenization_byt5.py in convert_tokens_to_string(self, tokens)
    238                 tok_string = bytes([ord(token)])
    239             bstring += tok_string
--> 240         string = bstring.decode("utf-8")
    241         return string
    242 

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte
```

## Expected behavior

2 strings as outputs of the ByT5.


Comments:
Hey :)

for faster debugging this can be break-downed to:

```python
from transformers import T5ForConditionalGeneration, ByT5Tokenizer

model_checkpoint = 'google/byt5-small'
tokenizer = ByT5Tokenizer.from_pretrained(model_checkpoint)

print(tokenizer.decode([258], skip_special_tokens=True, clean_up_tokenization_spaces=True))
```

The "official" ByT5 tokenizer is used from `seqio` and their implementation would return:

```python
from seqio import ByteVocabulary

tokenizer = ByteVocabulary()

tokenizer._decode([258])

# Returns:
# ''

# Better test:
tokenizer._decode([258]) == ''
# Return True
```

But as seen in the `ByteVocabulary()` implementation:

https://github.com/google/seqio/blob/main/seqio/vocabularies.py#L399

they use `errors="ignore"` as attribute of the `.decode()` function. Maybe this kind of error handling should also be applied here:

https://github.com/huggingface/transformers/blob/83d3dc0f6f8ae03e01aa5acacf88e79b2c1ecd06/src/transformers/models/byt5/tokenization_byt5.py#L240

:thinking: 

Update: this seems to be intended:

https://github.com/huggingface/transformers/commit/5c7789d4167064f7464b8801c7488a9a2878480a

Pinging @Narsil :)

Hello @stefan-it.

Thank you very much for taking the time to verify the problem.

Now I understand that `string = bstring.decode(" utf-8 ", error =" ignore ")` has been replaced by `string = bstring.decode(" utf-8 ")` by @Narsil (see [5c7789d](https://github.com/huggingface/transformers/commit/5c7789d4167064f7464b8801c7488a9a2878480a)) but because of this, it is not possible anymore:
- to use for example `model.generate()` with a `ByT5` model (because it will fail)
- and it is not possible to finetune a `ByT5` model (because when evaluating metrics it will use `tokenizer.decode()` that will fail).

We must find a solution. Do you have a proposal?

@Narsil - could you take a look once you're back?

Hi @piegu @stefan-it @patrickvonplaten ,

Do you know what you would expect to see instead ?

IMHO, failing here is perfectly correct as there is no correct way to represent byte (255) on its own.

If ByT5 generates invalid bytes, then the part that is supposed to recover a string should fail just like regular Python IMO, that's a model fail (it failed to generate bytes that correspond to a real string). Ignoring the error will just hide the problem under the rug and not really solve the problem. If you really don't care about failed generated bytes, then having a way to opt-in a different way of decoding even with malformed bytes makes sense. For the library, I am not sure it's a desired behavior by default as really we're ignoring a real model error.

If I take a less "simple" error where the model would generate `b'\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff'` then if we had errors=ignore by default, this would be `''` which would be very surprising to say the least as the model actually generated 128 tokens here...

Proposed modifications to make things easier for users:

- Add a new method specific to bytes tokenizers that would return raw `bytes` instead of `str`. This would not fail, and it would really be a user responsability to use it. (`decode_bytes` ?). Here it would return `b'\xff chocolates chocol'` leaving the user in charge to do something meaningful with it.
- Add a way to override to `errors=ignore` somehow directly from `decode`. It would add non trivial complexity to tokenizer code as `decode` is a generic method though.
- Add a way to override the `errors=ignore` through some attribute of the tokenizer. A bit better as it does not spill into generic code, but probably less discoverable (would need to add this to the doc in a very clear way).
- Switching back to `errors=ignore` but I really think it's a mistake in that case (just like having `errors=ignore` within Python itself would be pretty bad).

My personal take on this is that the first solution seems the better, but happy to hear counterarguments or if I overlooked something.

Hi @Narsil.

I would start with your question `Do you know what you would expect to see instead ?`
My answer: just a model (ByT5 here) with an output that can be always decoded in order to use the model in production and to finetune it with new data.... as this is the case for all models of the HF model hub.

In fact, I read and understood your arguments about not using `errors=ignore` in the `.decode()` method (`bstring.decode("utf-8")` instead of `bstring.decode("utf-8", errors="ignore")`) but the problem is in fact about ByT5 outputs, not about its `.decode()` method.

In my opinion, the true question is: **Why the model ByT5 in the Transformers library outputs tokens that can not be decoded?**

When I use the model BERT or T5 to generate outputs, I do not have the generation of such tokens (for example, I do not have the generation of a token id that is out of the tokenizer ids list).

And if you have a loot at the HF model hub, there are the Google ByT5 models and the finetuned ones:
- **Google ByT5 models**: how did Google train their ByT5 models? Indeed, at the end of each epoch, it is necessary to use  a method `.decode()` in order to obtain the generated texts and thus compare them to the targeted ones. Google used `errors=ignore`?
- **ByT5 finetuned models**: which version of Transformers the ByT5 models finetuned in the HF model hub used? Certainly the version 4.9.2 as this [notebook](https://colab.research.google.com/drive/1syXmhEQ5s7C59zU8RtHVru0wAvMXTSQ8) but not the actual one. What does it mean about the **quality** of these finetuned ByT5 models (which were finetuned with `errors=ignore` ) and how to finetune now a ByT5 model with the actual Transformers version?

What do you think? Should we focus on the method `.decode()` or on debugging ByT5?

> Hi @Narsil.
> 
> I would start with your question `Do you know what you would expect to see instead ?` My answer: just a model (ByT5 here) with an output that can be always decoded in order to use the model in production and to finetune it with new data.... as this is the case for all models of the HF model hub.

ByT5, unlike ALL other models (afaik), uses raw bytes, so it has no guarantee whatsoever to output a `string`.
It will however always produce `bytes`. (hence `decode_bytes` proposal).

> 
> In fact, I read and understood your arguments about not using `errors=ignore` in the `.decode()` method (`bstring.decode("utf-8")` instead of `bstring.decode("utf-8", errors="ignore")`) but the problem is in fact about ByT5 outputs, not about its `.decode()` method.
> 
> In my opinion, the true question is: **Why the model ByT5 in the Transformers library outputs tokens that can not be decoded?**

I have no idea, but it's expected that if it can produce non string data, it will (at some point at least). 

> 
> When I use the model BERT or T5 to generate outputs, I do not have the generation of such tokens (for example, I do not have the generation of a token id that is out of the tokenizer ids list).
> 
> And if you have a loot at the HF model hub, there are the Google ByT5 models and the finetuned ones:
> 
>     * **Google ByT5 models**: how did Google train their ByT5 models? Indeed, at the end of each epoch, it is necessary to use  a method `.decode()` in order to obtain the generated texts and thus compare them to the targeted ones. Google used `errors=ignore`?

Probably differently. All `string` can be casted to `bytes` (but not the other way around). So checking two `bytes` objects was probably the way it was done as this is always possible. Take the generated output, convert to bytes, take expected string, convert to bytes and compare the.

> 
>     * **ByT5 finetuned models**: which version of Transformers the ByT5 models finetuned in the HF model hub used? Certainly the version 4.9.2 as this [notebook](https://colab.research.google.com/drive/1syXmhEQ5s7C59zU8RtHVru0wAvMXTSQ8) but not the actual one. What does it mean about the **quality** of these finetuned ByT5 models (which were finetuned with `errors=ignore` ) and how to finetune now a ByT5 model with the actual Transformers version?
> 
> 
> What do you think? Should we focus on the method `.decode()` or on debugging ByT5?

I think we should accept that `ByT5` is different from other models, propose `decode_bytes` method and let users try to do things with it.

We could also break standard API and `decode` would return `bytes` instead of `string` but that would break many things, the automated tests at the very least.

To weigh in on this discussion, I wanted to reiterate the points raised by @piegu:
>  it is not possible anymore:
>
>  * to use for example `model.generate()` with a `ByT5` model (because it will fail)
>  * and it is not possible to finetune a `ByT5` model (because when evaluating metrics it will use `tokenizer.decode()` that will fail).

This means that it would always be required to overwrite the `evaluate` function when using `Seq2SeqTrainer` in combination with `predict_with_generate`, unless the `decode_bytes` option is directly addressed in the `Trainer`/`generate` implementation as well (creating additional overhead).

Since it is required to pass a `Tokenizer` in any case, I would prefer the option to choose directly through the tokenizer whether to ignore errors or not. I agree that it would have to be quite visible, but even for the T5 repository's implementation, this behavior is not very obvious ([reference issue](https://github.com/google-research/byt5/issues/11)), but implemented with ignoring the errors by default.

As to this point: 

> So checking two bytes objects was probably the way it was done as this is always possible. Take the generated output, convert to bytes, take expected string, convert to bytes and compare the.

I don't see any indication of the evaluation on `bytes` objects instead of `string`, as there seem to be no modifications on top of the vanilla T5 modeling from their own repository.

Thanks a lot for the nice repro @stefan-it! 
To be honest, I think we should just add `errors="ignore"` for the following reasons:

- One of the philosophies of `transformers` is to stay as close as possible to the original code
- If google added `errors="ignore"`, it was probably intended and is therefore not a bug IMO
- We broke backwards compatibility between 4.9 and 4.11 

What do you think @Narsil ? 

Also cc @LysandreJik here 

If google did it, then let's do it.

I have fine-tuned a model for orthographic modernization in Spanish with a prior version of transformers. In many cases, the predicted token is an accented letter such as `"ó"`. When these tokens appear in the list `tokens` of the function `.convert_tokens_to_string()`, the addition of the `errors="ignore"` makes the conversion miss one token, producing ill-formed words that were correctly predicted by the model. The reason, I believe, is that sometimes predicted tokens, as they come as strings, can be already encoded into more than 1 byte, and forcing them to be one byte triggers a `UnicodeDecodeError` as expected, which seems to be fixed by the `errors="ignore"` addition. However, I think the problem comes from the [line above](https://github.com/patrickvonplaten/transformers/blob/cba5330dc6825b6168fe043cb4b7a6f9694f78b6/src/transformers/models/byt5/tokenization_byt5.py#L238):

```python
tok_string = bytes([ord(token)])
```

If token was `"ó"`, that line of code produces `b'\xf3'`, which cannot be converted into a string and gets caught by the ignoring of encoding errors. But if I change that line to the next (which is equivalent to [`seqio`'s implementation](https://github.com/google/seqio/blob/main/seqio/vocabularies.py#L389))

```python
tok_string = bytes(token, encoding="utf8")
```

Now the result is `b'\xc3\xb3'`, which can be safely decoded back into a string

```python
>>> b'\xc3\xb3'.decode("utf8")
'ó'
```

I have created a separate issue [here](https://github.com/huggingface/transformers/issues/14461), but have not sent a PR because this change makes tests fail. I also wonder if models trained with `errors="ignore"` are somehow invalid. It could also be that my model is wrong somehow? I don't know how it's possible that in the list of tokens there are strings that decode to more than 1 byte.

Hi @versae 

Interesting.

I think we should stay away from `"ó"` since as you mentionned it has multiple encodings/representation.

The way I understand it, you are referring to a token (so an number between 0 and 255) . That's what tokens should be (even if in the linked function they are strings, they should be 1-1 mapping of numbers).

So the model generated, Token [243] in your case, which is `unicode codepoint` (`ó`). Let's keep calling it 243 for simplicity.

243 is a continuation byte, and not valid utf-8 on its own, you need another byte to have something valid. Your proposed changed would work, but because you're using the unicode codepoint as real unicode, where in fact it's just a number (represented as unicode codpoint, but that's because of previous internals of the base class it inherits from not by design). So I don't think the fix you're proposing is valid.

If you want the model to output "ó" it should instead output 2 different tokens, [195(c3), 179(b3)] and that should yield the desired result. Ultimately this generation should depend on how this model was trained, which I can't say for sure. But ideally, when outputting 243, it should never output EOS afterwards and keep on generating. If you are doing an early stopping then, you are indeed having an invalid byte sequence, and we should either ignore or crash, but we shouldn't change the meaning (output is likely to be garbage if we did it).

Does that make sense ?

Note: I tried to keep things simple and keep everything in 0-255 range, but ByT5 has 3 tokens (0, 1, 2) which are special, so the 0-255 range is shifted to 3-258 actually.


Hi @Narsil, thanks for the detailed explanation. I trained the model using Tensorflow on Oct 15th, which according to PyPI that should mean transformers was in its 4.11.3 version. I used `source,target` pairs such as (simplifying) `"precision","precisión"` so the model could learn modern Spanish orthography.

I understand the issue with the token 243. You are absolutely right. If I print the token ids generated you can see id 246 (246 - 3 = 243), which correspond to "ó". Why would the model learn to predict a token id like that?

```python
In [1]: from transformers import pipeline

In [2]: modernize = pipeline("text2text-generation", "versae/modernisa-pre")

In [3]: modernize("presuncion")
_convert_id_to_token: index, token 115 p
_convert_id_to_token: index, token 117 r
_convert_id_to_token: index, token 104 e
_convert_id_to_token: index, token 118 s
_convert_id_to_token: index, token 120 u
_convert_id_to_token: index, token 113 n
_convert_id_to_token: index, token 102 c
_convert_id_to_token: index, token 108 i
_convert_id_to_token: index, token 246 ó
_convert_id_to_token: index, token 113 n
convert_ids_to_tokens ['p', 'r', 'e', 's', 'u', 'n', 'c', 'i', 'ó', 'n']
convert_tokens_to_string: tokens ['p', 'r', 'e', 's', 'u', 'n', 'c', 'i', 'ó', 'n']
Out[3]: [{'generated_text': 'presuncin'}]
```

It happens with any accented letter. How could the model correctly predict 243 if it's never seen such a token in training? And how is it even possible that for accented letters the correct token is predicted although it is not possible to decode it in UTF-8? It seems I'll have to re-train I guess. Sorry for derailing the original issue topic, happy to take it somewhere else.

Cheers.

@versae 
Is it possible that you fed the models with unicode codepoints during training and not utf-8 encoded bytes ?
This looks like it, but I can't be sure. Since I think most accented spanish letters are still below 255 you might not have encountered any issue and been able to train your model just fine.

Just to make sure I tested, that the `byt5` tokenizer would encode `presunción` with the correct encoding:

```python
tokenizer = AutoTokenizer.from_pretrained('google/byt5-small')
okenizer.encode('presunción')
>>> [115, 117, 104, 118, 120, 113, 102, 108, 198, 182, 113, 1]
>>> # (195 + 179) so it works
```

If that's the case, then the good news is you don't necessarily need to retrain the model but maybe you need to override this function to change with your fix. Something along the way of :

```python
  def convert_tokens_to_string(self, tokens):
      """Converts a sequence of tokens (string) in a single string."""
      bstring = b""
      for token in tokens:
          if token in self.special_tokens_decoder:
              tok_string = self.special_tokens_decoder[token].encode("utf-8")
          elif token in self.added_tokens_decoder:
              tok_string = self.special_tokens_decoder[token].encode("utf-8")
          elif token in self.special_tokens_encoder:
              tok_string = token.encode("utf-8")
          elif token in self.added_tokens_encoder:
              tok_string = token.encode("utf-8")
          else:
              tok_string = token.encode("utf-8")
          bstring += tok_string
      string = bstring.decode("utf-8", errors="ignore")
      return string

tokenizer.convert_tokens_to_string = convert_tokens_to_string
```

Keep in mind:

1- This is a dirty hack
2- It might not be the core of the issue (it could be mistrained model, or some other error at training time). If it's not the core issue, this fix might just be hiding the true culprit and leading to more errors downstream.
3- You have now effectively broken your tokenizer since it won't encode the same things it decodes

But it should do the job for your specific purpose.

If you could also provide a link/script to how you trained it might provide more insights into what went wrong.

Thank you so much, @Narsil.

I trained models using the Trainer on [this dataset](https://huggingface.co/datasets/versae/modernisa) but their performance was really bad. Then I tested a small library called [SimpleT5](https://github.com/Shivanandroy/simpleT5/blob/main/simplet5/simplet5.py) that conveniently wraps Huggingface and PyTorch Lighting for T5 fine-tuning in like 3 lines of code and it worked like a charm. Or that's what I thought.

I'll use the hack for now and probably train new models to see if I'm able to get not terrible results using the Trainer.

@versae 

For information, we wanted to make sure everything is the same as the original and recompared everything to make sure.
FYI, there was a period in august where transformers had such a bug on `transformers`: https://github.com/huggingface/transformers/pull/13119 Might be linked to the issue you're having (if someone trained with the bug, then it could explain behavior you're seeing).

Thanks, @Narsil.

It seems to be the case! I re-trained the models and they work perfectly fine now, and with good BLEU and CER scores :)