Word offsets of some fast tokenizers are not compatible with token classification pipeline label aggregation
Author: davidbenton
Pull request: no
Labels: name: bug

### System Info

- `transformers` version: 4.21.0.dev0
- Platform: macOS-12.4-x86_64-i386-64bit
- Python version: 3.9.13
- Huggingface_hub version: 0.8.1
- PyTorch version (GPU?): 1.11.0 (False)
- Tensorflow version (GPU?): 2.9.1 (False)
- Flax version (CPU?/GPU?/TPU?): 0.5.2 (cpu)
- Jax version: 0.3.6
- JaxLib version: 0.3.5
- Using GPU in script?: N
- Using distributed or parallel set-up in script?: N

### Who can help?

Tagging @Narsil for pipelines and @SaulLu for tokenization. Let me know if I should tag anyone for specific models, but it's not really a model issue, except in terms of tokenization.

### Information

- [ ] The official example scripts
- [ ] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

I noticed this issue with a DeBERTa model, but it also affects some others. The high level issue is that some tokenizers include leading spaces in the offset indices, some exclude them, and some are configurable with `trim_offsets`. When offsets include leading spaces (equivalent to `trim_offsets==False`), the pipeline [word heuristic](https://github.com/huggingface/transformers/blob/afe5d42d8d1d80af911ed980c2936bfe887078f6/src/transformers/pipelines/token_classification.py#L294) doesn't work. The result is aggregating all tokens in the sequence to one label. Simple example:

```python
model_name = "brandon25/deberta-base-finetuned-ner"
model = AutoModelForTokenClassification.from_pretrained(model_name)
tokenizer = AutoTokenizer.from_pretrained(model_name)

ner_aggregate = pipeline("ner", model=model, tokenizer=tokenizer, ignore_labels=[], aggregation_strategy="max")
ner_aggregate("We're from New York")
```
Result:
```
[{'entity_group': 'O', 'score': 0.9999778, 'word': " We're from New York", 'start': 0, 'end': 19}]
```

### Expected behavior

Expected result, something like:
```
[{'entity_group': 'O', 'score': 0.9999778, 'word': " We're from", 'start': 0, 'end': 10}, {'entity_group': 'O', 'score': 0.9xxx, 'word': "New York", 'start': 11, 'end': 19}]
```

If you'd like to see actual output, here's a [colab notebook with relevant models](https://colab.research.google.com/drive/1bcWotnqSPNIuAaRNkELKmKiLQheudHu1?usp=sharing) for comparison.

This affects at least these:
- DeBERTa V1
- DeBERTa V2/3
- GPT2 (tested because `DebertaTokenizerFast` is a subclass of `GPT2TokenizerFast`)
- Depending on config, Roberta (and any other tokenizer that honors `trim_offsets==False`)

The easiest solution would be to update the heuristic. [Here is a change](https://github.com/davidbenton/transformers/commit/5c43c63d401f80818d95e9cafb627607680f4dff) that works for preceding space in sequence (like current heuristic) _or_ leading space in token. I can turn into a PR if desired.

I know a lot of the default configuration matches reference implementations or published research, so I'm not sure where inconsistencies between tokenizers are desired behavior. I did notice, for example, that some sentencepiece tokenizers include leading spaces in offset indices (DeBERTa V2/3), and some don't (Albert, XLNet). I looked at the converter config and the rust code (which is pretty opaque to me), but it's not obvious to me why the offsets are different. Do you know, @SaulLu? Is that expected?

I am comparing different architectures to replace a production Bert model and was evaluating models fine tuned on an internal dataset when I ran into this. I have my manager's blessing to spend some time on this (and already have! 😂), so I'm happy to work on a PR or help out how I can.

Comments:
Thank you very much for the detailed issue, that's a good point! 

> I know a lot of the default configuration matches reference implementations or published research, so I'm not sure where inconsistencies between tokenizers are desired behavior. I did notice, for example, that some sentencepiece tokenizers include leading spaces in offset indices (DeBERTa V2/3), and some don't (Albert, XLNet). I looked at the converter config and the rust code (which is pretty opaque to me), but it's not obvious to me why the offsets are different. Do you know, @SaulLu? Is that expected?

I reassure you, it is not obvious to me either why the offsets are different :smile: . In principle I think it's not a problem that the default value is different. But the problem is that for the moment, for many tokenizers it is not possible to change this value. Technically for 2 reasons: 1) for some we don't expose the argument at init and 2) for others, even if we did we couldn't change it some processors such as `Templateprocessor` don't allow to set it (I think it's the case for deberta).

I'll ping you in particular Narsil: 
1. do you think it's worth changing the heuristic for tokenizers that have `trim_offsets` set to False? I think you're the more knowledgeable for this :smile: 
2. does it make sense in principle to allow this argument to be set for all tokenizers that use a rust component that allows you to choose whether you want to trim the offsets or not? I have the impression that the NER use case is one of the main use cases for offsets in general
3. If you agree with the previous point, we will be blocked by the fact that Deberta doesn't use the `bytelevel` processor but the `TemplateProcessor` (which doesn't allow to choose trim_offset). We can surely leverage the processor sequence feature that @mishig25 is doing inside the tokenizers library https://github.com/huggingface/tokenizers/pull/1005 to solve it.

Note to @Narsil , I'm using "deberta-base" to reproduce the issue.

I agree that different defaults are not a problem, and it would be great if `trim_offsets` was configurable on more tokenizers. Maybe with a user warning if specifying it on an unsupported tokenizer?

I'll make a case briefly for updating the heuristic: It would be good if more people tried non-Bert models out for their tasks. For new users, the pipelines and associated doc recipes with a fine-tuned hub model can be a "hello world" for trying out transformers. Having the word heuristic work with default settings (without having to know about `trim_offsets`) for most models will mean more users have early success when they venture away from Bertland.

> f you agree with the previous point, we will be blocked by the fact that Deberta doesn't use the bytelevel processor but the TemplateProcessor (which doesn't allow to choose trim_offset). We can surely leverage the processor sequence feature that @mishig25 is doing inside the tokenizers library

This is the long term solution. As what's the good default for the tokenizer, it's not really up to me to decide, but the tokenizer's creator. I really like not trimming offets, and tokenizers like gpt2 which treat the *full string* as something that has to be fed to the model, meaning spaces have to be somewhere. This is a really great feature as it alleviates lots of headaches about "skipped" spaces, decoding issues and so on. But not all tokenizers are created equal and it's sometime more convenient to trim offsets (for whatever reason) or even it was just done that way in the original implem.

>  I have the impression that the NER use case is one of the main use cases for offsets in general

NER, POS, question-answering, and even mask filling when you want to make a correct replacement and don't have access to the ids. ANY task, which has to treat the original string really. It's also super helpful to debug whenever ids are *incorrect* and you want to know why (like weird unicode looking like ascii but screwing your results).

> I'll make a case briefly for updating the heuristi

@davidbenton I really like the idea of updating the heuristic. As long as it's clear it's a heuristic and not a *real* solution (aggregation_mode="simple" is the only real solution IMHO, others are workaround to poorly performing models adding extra bias)

The heuristic should be simple and elegant, your current solution is ! So I really like that fix. I think we can make it even simpler and commented directly on the diff .
Now to make it become a PR, I think it's really important is to add the slow test which is exactly shown here above.

Then add a fast test which deals only with the aggregation strategy (we can extract the actual entities from the slow test to have a real example), and add it as a fast test (so any regression is detected early on).
All the other tests should help cover any regression this heuristic change might introduce (I hope it doesn't).

Does that strategy seem viable ? Would you have time to set up such a PR ? 
Cheers !!

And thanks for bringing that up, if there's room for improvement in the pipelines I am all up for it. (But I am relatively convinced, that it's impossible to be 100% correct as "words" are ill defined in some languages ;)) 

Thanks a lot for your feedback @Narsil! Super good points!

For the heuristics improvement, couldn't we test if the `trim_offset` argument is defined in the processor of the `backend_tokenizer`, and if:
- yes and it is set to True keep the old logic
- otherwise use your logic @davidbenton  that tests the first character as you suggest? 

(I share the same conviction as you @Narsil that it's impossible to be 100% correct as "words" are ill-defined in some languages )

`trim_offsets` cannot be linked the heuristic IMO.

The heuristic is just trying to determine if what we're looking at is a "word". currently it only looks like if the previous character before the offset ends with a space. But prefix space could also exist so checking that the first character (in the original string) corresponding this token is a space is also valid IMO.
Again extremely biased towards space separated language, but working.

I may have to dive and see really what the issue is, but this is my current understanding without exactly looking at the issue in detail.

> trim_offsets cannot be linked the heuristic IMO.

I understand your point, I also realize that my proposal would not be adapted to tokenizers that have a pre-tokenizer that splits on spaces and removes them! 
 

> I understand your point, I also realize that my proposal would not be adapted to tokenizers that have a pre-tokenizer that splits on spaces and removes them!

Oh it's perfectly fine if that's the desired behavior we want. But I don't think we should bend backwards to make que QA pipeline work within a mode where it tries to recover because a model doesn't work properly ;). And the tokenizer cannot provide "words" boundaries (because it just wasn't made that way)

I'm not sure how to read your answer ahah. The tokenizer I have in mind is for example Bert's: Bert's tokenizer doesn't have trim_offset set to True, but the spaces are removed during the pre-tokenization step and the "words" boundaries are built the other way by adding "##" to the token that doesn't start a word.

Thanks for the comments and direction! I'm sorry to let this go stale, but I had a family emergency and this dropped off my radar. I should be available going forward, and I'll work on adding the tests mentioned above and set up a PR.

@Narsil I answered your question about the heuristic in context on my commit.



@SaulLu I'll note, relating to your suggestion to branch on `trim_offset`, that as my suggested heuristic works now, the logic is unchanged for models that do not tokenize whitespace, as it only checks in the decoded token for a leading space.

@davidbenton Perfect. I think you can do the modifications, and we would really benefit if there was a test making sure that the new heuristic works.

For instance here is a slow test to test the heuristics for spanish:

https://github.com/huggingface/transformers/blob/main/tests/pipelines/test_pipelines_token_classification.py#L200-L215

I have a couple tests added in my local wip, but it looks like there might be a borken pipeline test prior to my changes. The "UN" start/end entity offsets don't seem to match the input sequence on [this line](https://github.com/huggingface/transformers/blob/51227e26ab8fe6d1a19804da697786649f9340e3/tests/pipelines/test_pipelines_token_classification.py#L289), along with a few other diffs. @Narsil Is this expected, or should I be looking for green (or skipped) tests before I create a PR?

(FYI I'm running `RUN_SLOW=1 RUN_PIPELINE_TESTS=yes pytest tests/pipelines/test_pipelines_token_classification.py`; will also run the full suite once that's looking good.)

@davidbenton ,

If all **fast** tests pass, you should be fine. They are tested on every commit, so they should be green.

For slow tests, we run them before releases and in controlled environments, they are sometimes affected by `torch` version or `python` version.  Usually the differences are minor so we can decide how to deal with them on a case-by-case basis.

But for a PR it shouldn't be blocking (that's why we try to have good fast tests, as they are run very often, the slow tests are more like integration tests, usually when we need an actual trained model output to showcase something)

Yeah, that all makes sense. Are we sure slow pipeline tests are ~~running~~ being run? That test I linked has start/end offsets that seem to be incorrect (past the end of the input). I just wanted to flag that, but I'll go ahead and get that PR up too.

Thanks for flagging, I am looking into it right now :)

@davidbenton what's your environement ? I can't seem to reproduce on my local env

Do you mind creating a new issue for this ? Report it like a regular bug, there should be tools to print your exact env.
https://github.com/huggingface/transformers/issues/new?assignees=&labels=bug&template=bug-report.yml

As I said, slow tests can be sometimes a little more flaky that fast tests, but usually within acceptable bounds (pytorch will modify kernels which affects ever so slightly values, but it can pile up, Python version can break dictionary order etc..)