fix: Text splitting in the BasicTokenizer
Author: connor-henderson
Pull request: yes

# What does this PR do?

Note: The tests are failing because of repo-consistency, but I intentionally haven't made the change across the repo until we confirm what change we want to make. Will remove [Don't merge] tag once done.

This fixes an issue related to the BasicTokenizer.

Initially looked to fix #22166 by updating the _run_split_on_punc method in the BasicTokenizer to split apostrophes without starting a new word. In the issue, it was noted that apostrophes weren't being split properly as `should've` was being converted to `should`, `'`, and `ve` instead of `should` and `'ve`.

However, when adding testing it became apparent there are other cases where the BasicTokenizer is failing too, such as capturing '!!' as separate tokens with id 256 as opposed to one 748 token, which is in the [vocab](https://huggingface.co/openai/clip-vit-base-patch32/resolve/main/vocab.json).

To address these I modified `_run_split_on_punc` in the BasicTokenizer to also split on passed patterns and renamed it `_split_on_punc_or_pattern` and added tests for them. 


<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->


## 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/main/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.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/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. Feel free to tag
members/contributors who may be interested in your PR.

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- text models: @ArthurZucker and @younesbelkada
- vision models: @amyeroberts
- speech models: @sanchit-gandhi
- graph models: @clefourrier

Library:

- flax: @sanchit-gandhi
- generate: @gante
- pipelines: @Narsil
- tensorflow: @gante and @Rocketknight1
- tokenizers: @ArthurZucker
- trainer: @sgugger

Integrations:

- deepspeed: HF Trainer: @stas00, Accelerate: @pacman100
- ray/raytune: @richardliaw, @amogkam

Documentation: @sgugger, @stevhliu and @MKhalusova

HF projects:

- accelerate: [different repo](https://github.com/huggingface/accelerate)
- datasets: [different repo](https://github.com/huggingface/datasets)
- diffusers: [different repo](https://github.com/huggingface/diffusers)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Maintained examples (not research project or legacy):

- Flax: @sanchit-gandhi
- PyTorch: @sgugger
- TensorFlow: @Rocketknight1

 -->
@ArthurZucker

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

I think this was still a fix we were interesting in having for users who don't have `ftfy` installed.

> I think this was still a fix we were interesting in having for users who don't have `ftfy` installed.

Oh ok reopening in that case

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

Thanks for your comments Arthur! Looking back at it to make your changes I realized two things:
- we actually don't need to add the pattern splitting to the BasicTokenizer, we just need to *not* split on punctuation (just need to get out of the way of the byte pair encoding happening later)
- the already written [test_check_encoding_slow_fast](https://github.com/huggingface/transformers/blob/b29fd6971d9cd6ba2a824628effe243f543b8f61/tests/models/clip/test_tokenization_clip.py#L78) test is a bit more comprehensive, so I used that and it passes locally now without ftfy, I'll call out the additional edits I had to make below for this

Will wait for your review again before replicating it in the other BasicTokenizers. 

Also, would you like me to add additional testing? I.e., should tokenizers that use BasicTokenizer each have a test that checks slow and fast match like the comprehensive check like the one mentioned above that's in CLIP, or should they have a simple truncated version of it

Okay! I'll review again, can you make sure `make quality` and `make repo-consistency` both pass? 


Hey @ArthurZucker just checking in, anything else wanted here?

Nope thanks for the ping, it is just that it is a lot of changes on a lot of models (a lot of old models too 😉 ). Getting to it! 

Update: just keeping this PR to the punc splitting param, reasoning below. Lmk if you have other thoughts!

Wrote a [script](https://colab.research.google.com/drive/1tz4yZ_tHsGFvMlhQoihW4kCbCsxAGXco#scrollTo=bIaelmkHsWie) I ran locally to get a directional sense of how much of a difference each of these 3 changes (punctuation split, remove control chars, normalizing) was having to help choose how to address the above. It appears the punc split edit this PR was primarily addressing does help a fair bit, seems to increase compatibility with the CLIP fast tokenizer ~20% to near 100% for the languages tested. The control chars and normalizing edits don’t appear to make much of a difference at all (0% and ~0.1% improvement, respectively). Again this analysis was imperfect but I figure from this the cost-benefit for this PR suggests just keeping it to the punctuation splitting.

Also, I misspoke earlier saying control chars are in the CLIP vocab, they aren’t. Instead the discrepancy between the basic tokenizer and the fast one I was addressing was that the former strips them and the latter marks them as UNK. I don’t believe having this is likely to make much of a difference for inference or tokenization, as the script run suggests, since control chars are rare and UNK tokens don’t provide much info to the model.

Looking at the output for `ar` it seems  NEW + normalize is the best match isn't it ?

I think this proves that `NFC` is indeed a good addition which was previously missing ! 

Thanks a lot for this testing script, this adds a lot of value for future potential changes !

>I think this proves that NFC is indeed a good addition which was previously missing !

Thanks and sounds good, I'll put it back in. I had removed it only because I couldn't test all languages due to the time it would take so I wasn't certain if there could be other issues with it, and the improvements were somewhat modest.

Hey @Narsil just checking in, anything else wanted here?

No we just need a core maintainer's approval.

Sorry I forgot about this PR.

@sgugger for final review.

Noticed the linked issue was marked stale, this PR probably will be soon too. Any other action wanted here? 

I think as the script shows this will significantly improve how well the Basic Tokenizer matches up with the fast one for CLIP, the lingering question was just around whether the NFC normalizing change was approved or whether that part should be removed. @Narsil @sgugger 

Just waited for a clarification on whether @Narsil was fine with merging this or not. :-)

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.

Since @Narsil is not commenting, I'm guessing we can merge :man_shrugging: 

Oh sorry ! Missed this one it's OK !