Padding of bbox input in LayoutLM
Author: valentinkoe
Pull request: no

I've been working with LayoutLM and had some issues with different lengths of samples in a batch.

```
ValueError: Unable to create tensor, you should probably activate truncation and/or padding with 'padding=True' 'truncation=True' to have batched tensors with the same length.
```

It turns out that `transformers.tokenization_utils_base.PreTrainedTokenizerBase._pad` does not pad the `bbox` items to maximum length in the batch and eventually trying to join differently sized lists into a tensor crashes.

One way to solve this is to pad all required items when generating samples like e.g. the official implementation does for the [FUNSD data set](https://github.com/microsoft/unilm/blob/master/layoutlm/layoutlm/data/funsd.py#L317-L331). I also implemented it this way for my use case and it seems to work well.

But this is basically repeating the pad functionality and I was wondering if the `_pad` method should allow for additional required input like the `bbox`es are for LayoutLM. I'm happy to work on a PR for that but also wanted to check if there's anything more to consider.

Comments:
Hi! This is a fair request, indeed. The `bbox` values should definitely be padded/truncated by the tokenizer.

I think here we would welcome a PR adding this functionality for the LayoutLM tokenizer, and then think of a way to upstream it to be handled by the tokenizer directly, for LayoutLM but also for any other model that requires special inputs.

Would you be open to contributing a PR which adds this functionality to LayoutLM?

LayoutLM would really benefit from its own tokenizer indeed. Currently you have to use `BertTokenizer`, but this let's you only tokenize text, not really prepare data for the model.

A nice API (in my opinion) would look something like:

`LayoutLMTokenizer(image: PIL.Image, words: List[str], bounding_boxes: List[List[int]], labels: List[str])`

The tokenizer then automatically takes care of normalizing the bounding boxes (users can still choose which OCR engine to use to get words and bounding boxes), transform the words and labels into token-level `input_ids`, `bbox`, padding (as you mention), etc.

The functionality implemented in the function you refer to ([`convert_examples_to_features`](https://github.com/microsoft/unilm/blob/23a7ea35b55279a171a118ac767e863aa92e692c/layoutlm/layoutlm/data/funsd.py#L206)) could be added by overwriting the `prepare_for_model` method, and the padding functionality by overwriting `_pad`. 



I'm definitely up for working on this.

Thanks a lot for the suggestions @NielsRogge , I see you already did some great work in improving the layoutLM implementation :+1: .
What I do not fully understand is what we would need the `image` for at this stage. Can you clarify?

I'd also like to understand the better the normalization of bounding boxes you mention. If I understand correctly, the bounding boxes generated by the OCR engine may be split further according to whether the tokenizer splits the text inside a box (the official  layoutLM code seems to [repeat the same bounding box](https://github.com/microsoft/unilm/blob/master/layoutlm/layoutlm/data/funsd.py#L252) in those cases).
Afaik, most OCR engines do some kind of tokenization already so the additional splitting may not be optimal for all use cases (it is not for mine because of some downstream tasks). There should either be a way to revert that splitting or disable it. What do you think?

> Thanks a lot for the suggestions @NielsRogge , I see you already did some great work in improving the layoutLM implementation 

Thanks! 

> What I do not fully understand is what we would need the `image` for at this stage. Can you clarify?

The image can be used to normalize the bounding boxes for the tokens, based on the width and height of the image. If we decide to let LayoutLMTokenizer to handle the normalization, then it should receive the image. 

> I'd also like to understand the better the normalization of bounding boxes you mention. If I understand correctly, the bounding boxes generated by the OCR engine may be split further according to whether the tokenizer splits the text inside a box (the official  layoutLM code seems to [repeat the same bounding box](https://github.com/microsoft/unilm/blob/master/layoutlm/layoutlm/data/funsd.py#L252) in those cases).

An OCR engine (like Google's Tesseract) recognizes words and corresponding bounding boxes in an image. However, LayoutLM (like BERT) uses wordpieces, so if a word like San Francisco is tokenized into ['San', 'Fran', '##Cisco'], then we need to repeat the bounding box for every subword token indeed. 

> Afaik, most OCR engines do some kind of tokenization already so the additional splitting may not be optimal for all use cases (it is not for mine because of some downstream tasks). There should either be a way to revert that splitting or disable it. What do you think?

Do they? I used Google's Tesseract and it just recognizes words. 



> The image can be used to normalize the bounding boxes for the tokens, based on the width and height of the image. If we decide to let LayoutLMTokenizer to handle the normalization, then it should receive the image.

So you mean normalization in terms of bringing the coordinates to the same scale? That totally makes sense. Is that really something the tokenizer should do? Or should we expect the user to supply boxes with correctly scaled values?
If we want to do the scaling here, do we really need the full image for that and also restrict it to e.g. PIL/Pillow images? Some may use for example opencv where image objects are numpy arrays and don't have `height` and `width` attributes. The height and width values could also be provided as (optional) paramters for the tokenizer.

> An OCR engine (like Google's Tesseract) recognizes words and corresponding bounding boxes in an image. However, LayoutLM (like BERT) uses wordpieces, so if a word like San Francisco is tokenized into ['San', 'Fran', '##Cisco'], then we need to repeat the bounding box for every subword token indeed.

Actually, I referred to this recognition of words by OCR as tokenization as well - after all the text on a document/image could also be delivered as just one large string. Maybe that wasn't the right choice of words, sorry for the confusion.
I totally get that wordpiece "takes it a step further" and that this makes sense. What I wanted to clarify is how that should be dealt with. It might be confusing to a user to get a larger amount of bounding boxes after tokenization. I guess this is in line with the other tokenizers but it should at least be documented very clearly.

> So you mean normalization in terms of bringing the coordinates to the same scale? That totally makes sense. Is that really something the tokenizer should do? Or should we expect the user to supply boxes with correctly scaled values?

That's a design decision. We could choose to let the tokenizer handle normalization or not. And yeah maybe PIL images is too strict. 

> I guess this is in line with the other tokenizers but it should at least be documented very clearly.

If we add bounding boxes for every token, we should add it to the documentation indeed!

I created a PR https://github.com/huggingface/transformers/pull/10719 in which I added the functionality to repeat bounding boxes for text that is split, also solving the padding problem that lead me here.

However, I ended up basically repeating a lot of code (also for the tests) and I'm not sure this is the nicest way to tackle the problem. Maybe it could make more sense to add an optional `additional_input` parameter for the base tokenizers to avoid all this repetition. There may be more models that need additional inputs than the `input_ids`.

I also removed the fast version of the tokenizer for now as I first would like to clarify with the maintainers if this is the right approach.

(I also added optional coordinate normalization)

@LysandreJik could you maybe give this a look and suggest how to proceed with this?

ping @LysandreJik . Would be great to get some feedback on my PR :slightly_smiling_face: 

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/master/CONTRIBUTING.md) are likely to be ignored.