fix tokenize(): potential bug of splitting pretrained tokens with newly added tokens
Author: askerlee
Pull request: yes
Labels: name: wontfix

In the tokenizer base class, `split_on_token()` attempts to split input text by each of the added tokens. Because it uses `text.split(tok)`, it may accidentally split a token in the pretrained vocabulary at the middle.

For example a new token "ht" is added to the vocabulary. Then "light" will be split into `["lig", ""]`. But as "light" is a token in the pretrained vocabulary, it probably should be left intact to be processed by `self._tokenize()`.

Hence in this pull request, `text.split()` is replaced with `re.split()`, which will split only at word boundaries (`[^A-Za-z0-9_]` in regular expression). This behavior can be enabled by specifying a new `tokenize()` argument: `additional_tokens_as_full_words_only=True` (default: False). If it's specified in `tokenizer.encode(text, ...)`, it will still take effect, as this argument will be passed down to `tokenize()`.

On languages that have no or different word boundaries as above (such as Chinese or Japanese), this behavior may produce undesirable results, and the user can revert to the old `text.split()` by not specifying `additional_tokens_as_full_words_only` (it will take the default value `False`).

An explanation of the argument `additional_tokens_as_full_words_only` has been added to the docstring of `tokenize()`. A test function `test_add_partial_tokens_tokenizer()` has been added to `tokenization_bert_test.py`.


Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=h1) Report
> Merging [#1219](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/80faf22b4ac194061a08fde09ad8b202118c151e?src=pr&el=desc) will **increase** coverage by `7.67%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/1219/graphs/tree.svg?width=650&token=9qOlN6Hb1c&height=150&src=pr)](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
+ Coverage   73.24%   80.91%   +7.67%     
==========================================
  Files          87       46      -41     
  Lines       14989     7903    -7086     
==========================================
- Hits        10979     6395    -4584     
+ Misses       4010     1508    -2502
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...torch\_transformers/tests/tokenization\_bert\_test.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-cHl0b3JjaF90cmFuc2Zvcm1lcnMvdGVzdHMvdG9rZW5pemF0aW9uX2JlcnRfdGVzdC5weQ==) | `98.98% <100%> (ø)` | |
| [pytorch\_transformers/tokenization\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-cHl0b3JjaF90cmFuc2Zvcm1lcnMvdG9rZW5pemF0aW9uX3V0aWxzLnB5) | `80.62% <100%> (ø)` | |
| [src/transformers/tokenization\_t5.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdDUucHk=) | | |
| [src/transformers/tokenization\_ctrl.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fY3RybC5weQ==) | | |
| [src/transformers/configuration\_roberta.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX3JvYmVydGEucHk=) | | |
| [src/transformers/tokenization\_distilbert.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fZGlzdGlsYmVydC5weQ==) | | |
| [src/transformers/modeling\_tf\_auto.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9hdXRvLnB5) | | |
| [src/transformers/configuration\_distilbert.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2Rpc3RpbGJlcnQucHk=) | | |
| [src/transformers/configuration\_ctrl.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2N0cmwucHk=) | | |
| [src/transformers/modeling\_roberta.py](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19yb2JlcnRhLnB5) | | |
| ... and [125 more](https://codecov.io/gh/huggingface/transformers/pull/1219/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=footer). Last update [80faf22...d97a223](https://codecov.io/gh/huggingface/transformers/pull/1219?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Great, thanks @askerlee 

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.


Oh so it won't be merged? Anyway it's just a small issue @thomwolf

Thanks for the heads up. I forgot to follow up on this one.
It's good to merge indeed (rebased on mater).
cc @LysandreJik 

Hum actually this seems to break a number of tokenization tests.
Do you want to give it a look @askerlee?

@thomwolf sure. will fix it ASAP.


cc @LysandreJik who is working on fixing #2096 and following #2101 which are both related to this PR.

Hey @askerlee, thanks for your pull request. I'm currently working on it and adapting it to all models, I've updated the tests so that they fit the current master so don't worry about it.

@LysandreJik thank you so much! Sorry have been through very busy days. Do I need to do anything now? 

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.