fix CLIP fast tokenizer and change some properties of the slow version
Author: SaulLu
Pull request: yes

# What does this PR do?

As discussed in Issue #12648, the fast version of the CLIP tokenizer does not give at all the same tokenization as the slow 
version. 

From my point of view, the tokenization difference is really important and I think it would be important to fix this difference quickly (and as proposed in the issue, the **best is maybe to just remove - for the moment - the fast version of this tokenizer**).

## The PR in details

This PR proposes several changes to reduce this difference as much as possible. There are however several subtleties that I need to highlight and would love to hear your feedback on:
1. From my point of view, the difference between the slow and the fast version of the CLIP tokenizer comes from the fact that the original tokenizer applies the BPE algorithm on an already very pre-tokenized text ([line](https://github.com/openai/CLIP/blob/main/clip/simple_tokenizer.py#L124) of the original code where this pre-tokenization happen). The original tokenizer uses a lot of pre-tokenization rules and especially split on spaces and leave them out. In the current version of the CLIP fast tokenizer there is only a ByteLevel pre-tokenization which keeps the spaces which are transformed into Ġ by the ByteLevel pre-tokenizer ([code](https://github.com/huggingface/transformers/blob/master/src/transformers/convert_slow_tokenizer.py#L838)). I propose to discuss in more detail the solution I propose in [this comment](https://github.com/huggingface/transformers/pull/15067#discussion_r780993421). 
2. Then, the original tokenizer, uses quite powerful tools to [clean the text](https://github.com/openai/CLIP/blob/main/clip/simple_tokenizer.py#L50-L53): 1) the [`ftfy.fix_text`](https://ftfy.readthedocs.io/en/latest/explain.html#ftfy.fix_text) method and the 2) the [`html.unescape` method](https://docs.python.org/3/library/html.html#html.unescape) . You will find below a copy of the docstrings of these 2 methods. These methods are therefore very similar to normalization because they will greatly modify the text that will be given as input to the model. I think that here we come across the same type of "problem" that we could have with vision for example: **at what point do we consider that it is pre-processing and that it goes beyond the scope of transformers?** Personally, I think that replicating exactly what these 2 methods do is out of the scope of transformers and that we just need to identify if there are normalization steps in these methods that would be considered as very frequent and that should therefore be applied in the normalizer. I identified 3 types of normalization and wrote a test that tests the equality of tokenization of complicated / specious texts between the slow and fast version of the tokenizer . I refer you to comments [2](https://github.com/huggingface/transformers/pull/15067/files#r780360824) and [3](https://github.com/huggingface/transformers/pull/15067/files#r780361274) where I discuss respectively these points in more details. 
3. Then, I have to talk to you about the fact that the components available today in the tokenizers library do not allow to make a tokenizer fast perfectly adapted to the CLIP one. More specifically, there are two components of the backend_tokenizer that would ideally need to chain 2 sub-operations and today we can't do that (we can only apply one). It is possible that in a future version of tokenizers these problems will be solved, but it will require in any case to wait for a next version (see the 2 open issues respectively for the 2 components: [issue 1](https://github.com/huggingface/tokenizers/issues/872#issuecomment-1007239999) and [issue 2](https://github.com/huggingface/tokenizers/issues/873#issuecomment-1007236641)). This issue concerns: 
    - **the `decoder`**. For the decoder, the CLIP tokenizer would need to chain `ByteLevel` decoder and `BPEDecoder`. Nevertheless, even if it is not possible to do it in rust today, **I have a solution that results in a 100% identical behavior**. There may be a small loss of efficiency (but it should still be more efficient than going through the slow tokenizer) and it will only concern the decode function. My hack consists in including a `ByteLevel` component in the `backend_tokenizer` then monkey patch the `self.backend_tokenizer.decode` method to perform the operation that should have been done by the `BPEDecoder` decoder. See this [comment 4](https://github.com/huggingface/transformers/pull/15067/files#r780369218) for the hack. 
    - **the `post_processor`**. For the post_processor, the CLIP tokenizer would need to chain `ByteLevel` post-processor (for the offsets) and the `TemplateProcessing` to add special tokens to the output (one template for a single sentence and another template for a pair of sentences). Today, it is not possible to chain these 2 processors but there is a processor `RobertaProcessing` that contains the `ByteLevel` processing and that adds special tokens as desired for the template with a sentence. The difficulty here posed by the post-processor is due to the post-processing of a pair of sentences: something that the original CLIP model does not define. What the slow tokenizer proposed to do if you gave it a pair of sentences as input is to simply join the 2 sentences with the template `"bos_token" tokens_for_sentence_1 tokens_for_sentence_2 "eos_token" `. Unfortunately, roberta's template is `"bos_token" tokens_for_sentence_1 "eos_token" "eos_token" tokens_for_sentence_2 "eos_token"` (and the `token_type_ids` don't match either). So, **I made some modifications so that the slow and fast tokenizer follow the same template as Roberta for tokenizing a pair of sentences. This is the change that annoys me the most in this PR because it modifies the behavior of the slow tokenizer: I wanted to propose it anyway because I'm not sure to see the use case for tokenization of sentence pairs for CLIP.** 
4. I also have other modifications in the slow version of the tokenizer because I found that some arguments in the init could not really be used. These are the following arguments: 
    - **do_lower_case**: [This line](https://github.com/huggingface/transformers/blob/v4.15.0/src/transformers/models/clip/tokenization_clip.py#L179) and [this line](https://github.com/huggingface/transformers/blob/v4.15.0/src/transformers/models/clip/tokenization_clip.py#L316) hardcode the lowercasing of the text. 
    - **add_prefix_space**: I don't think this tokenizer "has been trained to treat spaces as parts of tokens". Currently adding `add_prefix_space=True` to the slow tokenizer does not change the tokenization. This is because the space is added at the beginning of the sentence before applying the pre-tokenization using the regex `r"""<\|startoftext\|>|<\|endoftext\|>|'s|'t|'re|'ve|'m|'ll|'d|[\p{L}]+|[\p{N}]|[^\s\p{L}\p{N}]+"""`. So I removed all references to `add_prefix_space` and forced this argument to be `False` in the tokenizer fast components. 
5. Regarding the tests, I think it is important to run all the CLIP tokenization tests with the `ftfy` dependency installed. Without the `ftfy` dependency installed, the slow tokenizer uses the `BasicTokenizer` of BERT which gives a different tokenization. So I tagged some tests as requiring with the ftfy dependency and added a new r`un_tests_tokenization_CLIP` in CircleCI. **Do you agree with this addition? Would you have a better way to do it?** 
6. Without the `ftfy` dependency installed, the slow tokenizer uses the `BasicTokenizer` of BERT which gives a different tokenization. This leads to another question: **shouldn't we add the same normalizations to this default tokenizer as the normalizations chosen for the fast tokenizer?** (if yes, I think that we can do it in a next PR)
7. Then, I made 3 other modification: 
    - I removed the `pad_token_id` property hardcoded to 0. I removed the custom test for CLIP `test_pretokenized_inputs` (it was not executed before) after this change. **Was there a reason for hardcoding it to token 0? Isn't it better to leave it equal to the value of the pad_token id set in the init?** If I have missed something and it is necessary to hardcode this value, I can absolutely revert the changes 
    - I modified the vocabulary and the merges in the tests because before the merge list was using tokens that were not in the vocabulary (and that prevented to initialize a tokenizer fast) 
    - I have remove `errors` from the `CLIPTokenizerFast` docstring because I don't think that it's an argument that can be used for this object.

## Documentation extracts

### ftfy.fix_text method
> Given Unicode text as input, fix inconsistencies and glitches in it, such as mojibake (text that was decoded in the wrong encoding).

### html.unescape
> html.unescape(s)
> Convert all named and numeric character references (e.g. `&gt;`, `&#62;`, `&#x3e;`) in the string s to the corresponding Unicode characters. This function uses the rules defined by the HTML 5 standard for both valid and invalid character references, and the list of HTML 5 named character references.



## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [x] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/master/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/master/docs#writing-source-documentation).
- [x] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. 

It would be really great if @patil-suraj , @LysandreJik and @sgugger you could share your opinion :hugs: :

- @patil-suraj in particular because I propose to modify the slow version of the tokenizer
- @LysandreJik and @sgugger in particular :
       -  to know your opinion about the post-processor template with sentence pairs (as for me this is my least favorite point in my PR)
       -  to know your opinion about the way to test with `ftfy` dependency
        - and if you have time, I'd be really happy to know your opinion about all the other points

Thanks a lot in advance :hugs: 


Comments:
@sgugger , your comment makes a lot of sense! Indeed, now that you share the idea of the `tokenizer_vxxx.json` file, Lysandre showed me this a while ago. Have you already implemented this kind of solution for another part of the library (to try to propose something similar)?

Also, have you ever had a similar case of an object not behaving as expected? Do you have any ideas how to warn users if we keep the old `CLIPTokenizerFast` object (if we create several objects, maybe it's just to leave a warning log in the first object)?

@n1t0 will know if it was already used for some tokenizers or not.

> - to know your opinion about the post-processor template with sentence pairs (as for me this is my least favorite point in my PR)

It's fine for me as I'm not convinced it's used for sentence pairs either, but maybe @patil-suraj or @NielsRogge can confirm

> - to know your opinion about the way to test with ftfy dependency

For the tests we could add it to the custom tokenizer run which is created for that purpose:

https://github.com/huggingface/transformers/blob/4df69506a8250d4bd298d457090b321b26b0c77f/.circleci/config.yml#L538-L565

> - and if you have time, I'd be really happy to know your opinion about all the other points

I think what you propose is sensible. This will result in a breaking change, which is fine if it doesn't affect previous versions as it's really a bugfix, imo. Sylvain's proposal of using the versioning system could work, indeed. From what I'm seeing there aren't many repos with a fast tokenizer script, but it's very possible that users have fast tokenizer files stored locally, would these users benefit from the fix?

Given how broken it is I'm actually wondering if removing the fast tokenizer isn't the best way forward, actually. It's the first time we would ever do something like that, but it would not break previous versions and it would ensure that all future versions will continue working correctly. If we decide to go this way, we could actually have an automatic way of returning a slow CLIP tokenizer from the fast tokenizer instantiation, since they're able to save themselves with slow files which can then be re-used by the slow tokenizer we would be returning. Very inelegant and unoptimized as extra serialization + deserialization + promise of returning a fast tokenizer while returning a slow one without many feautres of the fast tokenizer (are they really useful for a CLIP tokenizer though? thinking of offsets, for example).

@LysandreJik , Thanks a lot for your feedback! 

> For the tests we could add it to the custom tokenizer run which is created for that purpose:

Well noted, I moved it (corresponding [commit](https://github.com/huggingface/transformers/pull/15067/commits/c9eebb7882fbee9c3cb1cae3a28bb90274a7ea03)).

> Given how broken it is I'm actually wondering if removing the fast tokenizer isn't the best way forward, actually. It's the first time we would ever do something like that, but it would not break previous versions and it would ensure that all future versions will continue working correctly. If we decide to go this way, we could actually have an automatic way of returning a slow CLIP tokenizer from the fast tokenizer instantiation, since they're able to save themselves with slow files which can then be re-used by the slow tokenizer we would be returning. Very inelegant and unoptimized as extra serialization + deserialization + promise of returning a fast tokenizer while returning a slow one without many feautres of the fast tokenizer (are they really useful for a CLIP tokenizer though? thinking of offsets, for example).

I prefer to make sure I don't miss anything here. Actually you think we should replace `CLIPTokenizerFast` with a bridge that would build a `CLIPTokenizer` instead - not the tokenizer proposed in this PR. Do I have this right?


I am building a CLIP tokenizer for a rare language. But this fix is causing issues. Here is the script,

https://gist.github.com/humanely/7e66004744c629c317df492e2a86b555
```
c=CLIPTokenizerFast.from_pretrained("cliptok", from_slow=False      )

c.decode(c.encode(c.decode(c.encode("ॐ नमः शिवाय"))))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/homebrew/anaconda3/lib/python3.11/site-packages/transformers/tokenization_utils_base.py", line 3811, in decode
    return self._decode(
           ^^^^^^^^^^^^^
  File "/opt/homebrew/anaconda3/lib/python3.11/site-packages/transformers/tokenization_utils_fast.py", line 625, in _decode
    text = self._tokenizer.decode(token_ids, skip_special_tokens=skip_special_tokens)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/anaconda3/lib/python3.11/site-packages/transformers/models/clip/tokenization_clip_fast.py", line 102, in new_decode_method
    text = text.replace(self.backend_tokenizer.model.end_of_word_suffix, " ").strip()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: replace() argument 1 must be str, not None
```

When I replace `"end_of_word_suffix": null,` with `"end_of_word_suffix": ""` the output is spaced out.
With `"end_of_word_suffix": "</w>",` the decoded output is truncated.

Whereas, the decoder works fine with Autotokenizer. Any suggestion about how the CLIP tokenizer model should be built instead of how I did? TIA

Hi @humanely! This is a question best placed in our [forums](https://discuss.huggingface.co/).