Transformer-XL: Fixed tokenization of brackets, numbers etc.
Author: RafaelWO
Pull request: yes

Fixes #5136 

As explained in the above issue, this PR fixes the tokenization of the `TransfoXLTokenizer` by using the `sacremoses` library with an extended feature of tokenizing comma-separated and floating point numbers. That way the input text is tokenized the same way as in the WikiText-103 dateset used for pretraining.

Open questions:
- [ ] Should the method `prepare_for_tokenization` be removed, since it just issues a warning for no whitespace before punctuation? This is solved by the `MosesTokenizer` anyway.

Comments:
Test fail due to the test in `test_tokenization_fast`, but I have not touched that file.
For me, the problem is in [test_tokenization_fast.py line 45](../tree/master/tests/test_tokenization_fast.py#L45). It works for me if I replace the path `tests/fixtures/...` with `./fixtures/...`

Should I fix this?

Hey! I'm interested in this as I have noticed the same phenomenon. Some of those files were moved recently; could you rebase from master to check whether that fixes the pathing issue?

That's already looking better! If the tests still fail, it might be because the current expected outputs of the tests are incorrect. If your results are better than the expected outputs, that should be clear quickly.

Is this fine if there are 229 files which have changed? If I do a compare from my repo with the master branch, it's only the 2 as before...

For me, the reason for failure is still the same as mentioned above: `test_tokenization_fast.py:45: FileNotFoundError`
```
>       with open("tests/fixtures/sample_text.txt", encoding="utf-8") as f_data:
E       FileNotFoundError: [Errno 2] No such file or directory: 'tests/fixtures/sample_text.txt'
```

That's weird, I'm seeing

`AssertionError: Sequences differ: [100,[42 chars]832, 6193, 43, 24, 24, 24, 24, 24, 24, 29289, [4136 chars]1, 3] != [100,[42 chars]832, 24, 24, 29289, 476, 35, 24, 19, 5387, 812[3333 chars], 24]`

in both CI tests, at `tests/test_tokenization_fast.py:62`

Hmm strange... Maybe it's because I executed the tests in PyCharm.
**Edit**: In PyCharm the root path for the tests was configured to be `.../tests/`, so that was the problem for me earlier.

Ok if I run it in the console I see the same result as you above!

@TevenLeScao 
> If the tests still fail, it might be because the current expected outputs of the tests are incorrect.

How can I fix this?

The issue seems to be that the Rust and Python tokenizers don't agree as the issue is also present in the Rust tokenizers. @n1t0 @Pierrci maybe could you take a look at this?

I have no idea what changed unfortunately, but yes the rust tokenizers and the python one should have the same output for the tests to pass.

Does it make sense if the `TransfoXLTokenizerFast` would also use the tokenizer from `sacremoses`?

Not really. The fast tokenizers are implemented in Rust, not Python, so it's not possible to use sacremoses. Also, the fast tokenizers keep track of the offsets/alignment information, and I guess this is not something that sacremoses provides, does it? I'm really not familiar with it so I have no idea if it would be easy to re-implement otherwise.

What exactly do you mean with offsets/alignment information? I'm not familiar with Rust nor the fast tokenizers in general. 

Hi again, is there any update on the issue with the fast tokenizers?

Should I maybe open a new PR which is up to date with the master branch so that we get rid of the messy commit history and lots of "changes"?

Hi @RafaelWO , sorry for the delay.

 - A PR without the extra commits would definitely help
 - In the end we couldn't really decide on a solution: I think the consensus with @n1t0 and @thomwolf was that deleting the Rust TransfoXL tokenizer was the best but it's pretty harsh in terms of BC-breaking. I think the best would be to give it a deprecation warning for a bit and remove the Rust = Python tokenizer equality test for TransfoXL in the meantime.

- I will reopen one
- Sounds good, I will post my changes in a new PR and then we can discuss the deletion of the Rust tokenizer

Reopened in PR #6322