Add BROS
Author: jinhopark8345
Pull request: yes

# What does this PR do?

<!--
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 -->

Add [BROS(BERT Relying On Spatiality)](https://arxiv.org/abs/2108.04539) to 🤗 Transformers


## 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?
- [x] Was this discussed/approved via a [Github issue](https://github.com/huggingface/transformers/issues/23181) or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [x] 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.

@NielsRogge 

Comments:
@jinhopark8345 Awesome work - looking forward to having this model added! Feel free to ping us when the PR is ready for review or you have any implementation questions in the meantime. 

@amyeroberts 

I am confused about what needs to be done.

According to the [How to add a new model](https://huggingface.co/docs/transformers/add_new_model#514-port-brandnewbert-to-transformers) guideline, a big part of it is porting pretrained models (from the original repo) into Huggingface transformers and making sure they are correctly ported by checking the outputs of each layer's forward step.

However, it seems like the authors of the Bros model used `transformers-cli` to create the boilerplate code, and I don't think there is much to change from the [original code](https://github.com/clovaai/bros/blob/master/bros/modeling_bros.py).

Do I need to write a conversion script? Or can I skip this step and move to the step where I add model test codes?

Thanks for the help in advance!

@jinhopark8345  Interesting - that will definitely make things easier! In this case, if the files are already on the hub and in the correct format, there's no need for the conversion script. It's possible there might be additional arguments required in the config files or additional files needed in the hub repo, in which case, I'd suggest writing a script to add these. You probably won't be able to write directly to the org's repo, but can open a PR with any necessary changes. 

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_23190). All of your documentation changes will be reflected on that endpoint.

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.

Still working on it! Writing tutorial/demo notebooks of how to use BROS 

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.

@amyeroberts Is it possible to reopen this PR because I have been working on [forked repo](https://github.com/jinhopark8345/bros/tree/feature/update-data-loading)  of original bros code

> Thanks for adding this model!
> 
> Overall PR is looking good - very clean and easy to read. Most comments are nits about formatting and have the copied from statements.
> 

Thank you for the feedback!!

> What's the difference between Bros, BrosSpadeEL and BrosSpadeEE ? The purpose of these three modules isn't clear. Could you add an explanation in the model's class docstrings as well as the model's doc page - bros.md?

Bros is a encoder transformer. BrosSpadeEL is Bros + [SPADE EL](https://arxiv.org/abs/2005.00642) (entity linking decoder attached) and BrosSpadeEE is Bros +  [SPADE EE](https://arxiv.org/abs/2005.00642) (entity extraction decoder attached). I will add more detailed explanation to model's doc page.

[Original bros repo](https://github.com/clovaai/bros) has total  3 heads for the Bros Model:
- BrosTokenClassification
- BrosEE (entity extraction)
- BrosEL (entity linking)

Both BrosTokenClassification and BrosEE essentially perform the same job. However, in the case of BrosTokenClassification, it assumes input tokens are perfectly serialized (which is very challenging task since they exist in a 2D space), while BrosEE allows for more flexibility in handling serialization errors as it predicts next connection token from one token.

BrosEL predicts relation from first token (of one entity) to the other first token(of the other entity) if these two entities share some relation.


> About the model inputs. We try as much as possible in transformers to have a consistent API. This also means consistent inputs and outputs. It would be better if this model took a standard bbox format e.g. (x0, y0, x1, y1) and then converted to the 6 point needed within its forward pass.

I will change the forward pass to convert from 2 points to 4 points within its forward pass.


@amyeroberts 
Would it be a better idea to add/update `tokenizer_bros.py `and `processing_bros.py` that takes boxes as input, similar to [layoutlmv3](https://huggingface.co/docs/transformers/model_doc/layoutlmv3) ?

while  updating the doctest code for the bros model, I realized that trying out the bros model using ([my demo script](https://github.com/jinhopark8345/FormUnderstanding/blob/main/scripts/bros_on_SROIE_BIO_tagging.py)) would be quite complicated because of making `bounding boxes`, `box first token mask`, update `labels` accordingly.

@jinhopark8345 What I would suggest is following the pattern of LayoutLM model: the model takes in bboxes of format (x0, y0, x1, y1). Then for BROs, within the model these are converted to the 6 points used in the forward pass.

No tokenizer needs to be added as the model can use the already implemented BERTTokenizer. 

Processors classes which bundle together two processing classes e.g. tokenizer + image processor, so isn't needed here.

@amyeroberts I am encountering the following errors during the tests ( `ci/circleci : tests_torch` )

```bash
FAILED tests/utils/test_hub_utils.py::GetFromCacheTests::test_get_file_gated_repo - AssertionError: OSError not raised
FAILED tests/utils/test_hub_utils.py::GetFromCacheTests::test_has_file_gated_repo - AssertionError: OSError not raised
```
Is this related to my pull request, or are there issues within the Huggingface library?

@jinhopark8345  - these aren't related to your PR and are likely transient issues with connecting to the hub. You can ignore them for now. If they persist exclusively on this PR and when the PR is finalised then we can dig into it more. 

@amyeroberts Thank you very much for the feedback and help! 😃 I truly appreciate it. I have implemented your suggestions from the code review, and all necessary tests are passing. However, there were a few suggestions that I wasn't sure how to address, so I left comments explaining my thoughts on those points.

@amyeroberts I added `convert_bros_to_pytorch.py` script because `bbox_projection` (linear layer) moved from `BrosTextEmbeddings` to newly added `BrosBboxEmbedding`.

@jinhopark8345 Great - thank you! Could you rebase on main to resolve the conflicts? We should be good to go after that :) 

@jinhopark8345 Thanks for contributing this model! Make sure to share about it's addition to the library on twitter/linkedin/your medium of choice 🤗 

@jinhopark8345 Thank you for adding this model into `transformers`.

Regarding the test `BrosModelIntegrationTest.test_inference_no_head`, it fails on our T4 GPU VM as the expected and actual output differ too much, as you can see.

Could you double check on your machines, please? And what's your machine (GPU) type? 

Thank you in advance.

```bash
(Pdb) outputs.last_hidden_state[0, :3, :3]
tensor([[-0.3165,  0.0830, -0.1203],
        [-0.0089,  0.0031,  0.0736],
        [-0.0461,  0.0146,  0.0880]], device='cuda:0')
(Pdb) expected_slice
tensor([[-0.4027,  0.0756, -0.0647],
        [-0.0192, -0.0065,  0.1042],
        [-0.0671,  0.0214,  0.0960]], device='cuda:0')

```

You can run the test with

```bash
TF_FORCE_GPU_ALLOW_GROWTH=true RUN_SLOW=1 python3 -m pytest -v tests/models/bros/test_modeling_bros.py::BrosModelIntegrationTest::test_inference_no_head
```

> @jinhopark8345 Thank you for adding this model into `transformers`.
> 
> Regarding the test `BrosModelIntegrationTest.test_inference_no_head`, it fails on our T4 GPU VM as the expected and actual output differ too much, as you can see.
> 
> Could you double check on your machines, please? And what's your machine (GPU) type?
> 
> Thank you in advance.
> 
> ```shell
> (Pdb) outputs.last_hidden_state[0, :3, :3]
> tensor([[-0.3165,  0.0830, -0.1203],
>         [-0.0089,  0.0031,  0.0736],
>         [-0.0461,  0.0146,  0.0880]], device='cuda:0')
> (Pdb) expected_slice
> tensor([[-0.4027,  0.0756, -0.0647],
>         [-0.0192, -0.0065,  0.1042],
>         [-0.0671,  0.0214,  0.0960]], device='cuda:0')
> ```
> 
> You can run the test with
> 
> ```shell
> TF_FORCE_GPU_ALLOW_GROWTH=true RUN_SLOW=1 python3 -m pytest -v tests/models/bros/test_modeling_bros.py::BrosModelIntegrationTest::test_inference_no_head
> ```


@ydshieh Thank you for providing the test command!

I was able to reproduce the issue, but the `outputs.last_hidden_state[0, :3, :3]` value I obtained was different.
Interestingly, not only did the output was different from the `expected_slice` but the value of `outputs.last_hidden_state[0, :3, :3]` changed every time I ran the command you provided. For testing, I am using an RTX3090.

After some testing, I found that some weights weren't being initialized properly.
This issue came from that `bbox_projection` layer (a linear layer) was moved from `BrosEmbeddings` to under `BrosBboxEmbeddings` (`BrosBboxEmbeddings` is newly added class)

By changing:

    model = BrosModel.from_pretrained("naver-clova-ocr/bros-base-uncased").to(torch_device)

to:

    model = BrosModel.from_pretrained("jinho8345/bros-base-uncased").to(torch_device)

I was able to get consistent outputs. (conversion script : `transformers/models/bros/convert_bros_to_pytorch.py`)

I suspect this issue wasn't detected earlier because when running:

    python3 -m pytest -v tests/models/bros/test_modeling_bros.py

torch cuda seed is manually set to certain value, perhaps due to other tests or other reasons.

The update is [here](https://github.com/jinhopark8345/transformers/blob/745f9c8c027a42ddedde4c6d65c4d127ec8614e1/tests/models/bros/test_modeling_bros.py#L413) but I am not sure how I should apply this patch to Transformers library.


Hi @jinhopark8345 Thanks a lot for looking into this!

You can open a PR to update the checkpoint repo used in the test, or we can do it on our own side.

But is it expected that `naver-clova-ocr/bros-base-uncased` doesn't have all the weights? What are the difference between these 2 checkpoints?

> Hi @jinhopark8345 Thanks a lot for looking into this!
> 
> You can open a PR to update the checkpoint repo used in the test, or we can do it on our own side.
> 
> But is it expected that `naver-clova-ocr/bros-base-uncased` doesn't have all the weights? What are the difference between these 2 checkpoints?

 The `naver-clova-ocr/bros-base-uncased` has all the weights. But some weights have been renamed. So if we load `BrosModel` with  `naver-clova-ocr/bros-base-uncased` checkpoint (original checkpoint), the renamed weights won't be initialized correctly with pretrained weights. 
 
 these are the renamed weights!
 ```python
 def rename_key(name):
    if name == "embeddings.bbox_projection.weight":
        name = "bbox_embeddings.bbox_projection.weight"

    if name == "embeddings.bbox_sinusoid_emb.x_pos_emb.inv_freq":
        name = "bbox_embeddings.bbox_sinusoid_emb.x_pos_emb.inv_freq"

    if name == "embeddings.bbox_sinusoid_emb.y_pos_emb.inv_freq":
        name = "bbox_embeddings.bbox_sinusoid_emb.y_pos_emb.inv_freq"

    return name

 ```
 
 If you confirm updating the checkpoint is okay, I would like to open PR!

Sure, go for it. BTW, I see a lot of `naver-clova-ocr/bros-base-uncased` used, in particular in the examples. So just to be sure, is the user expected to use `naver-clova-ocr/bros-base-uncased` or the renamed one `jinho8345/bros-base-uncased`?

From your description, I think it is `jinho8345/bros-base-uncased`. If this is the case, could you update all occurrence (not just in the tests). Thank you!

Hello @jinhopark8345 Thank you again for fixing the checkpoint. I have yet another question needs your help.

For `BrosModel`, the `bbox_position_embeddings` could be `None` before calling `self.encoder` (if `bbox` is `None`)

https://github.com/huggingface/transformers/blob/37c205eb5d5165b70d3100b599a2bcfc483944f5/src/transformers/models/bros/modeling_bros.py#L927-L933

but eventually, `BrosSelfAttention` will fail if it receives `None` for `bbox_pos_emb`

https://github.com/huggingface/transformers/blob/37c205eb5d5165b70d3100b599a2bcfc483944f5/src/transformers/models/bros/modeling_bros.py#L391

Could you double check if `BrosModel` will only work if `bbox` is not `None` in the original implementation? If this is not the case, how is `bbox_pos_emb` being created if `bbox` is `None` etc.

Thank you in advance, again!

Hello @ydshieh Thank you for asking!

Below code is the original implementation
``` 
        scaled_bbox = bbox * self.config.bbox_scale
        bbox_pos_emb = self.embeddings.calc_bbox_pos_emb(
            scaled_bbox, self.config.pe_type
        )
```

In original implementation, `BrosModel` will only work if `bbox` is not  `None`.

Would it be more helpful to users if we remove https://github.com/huggingface/transformers/blob/2d71307dc0ee2849f785568f345837e726209fc6/src/transformers/models/bros/modeling_bros.py#L928 so that `BrosModel` fails earlier? or do you suggest different solutions?

Hi! In this case, you can add a try: except at the beginning of `BrosModel.forward` method as the input validation.

(we might need a few more fixes if CI fails due to this)

Thank you !

Hi @jinhopark8345 , congrats on this amazing contribution.

Feel free to share about it on Twitter/LinkedIn and we'll amplify.

Hi @jinhopark8345,
Can you please provide examples of how to use logits from BrosSpadeELForTokenClassification to identify the intra-relationships? 
TIA

Hi @Prathyusha-Akundi,

You can refer to the [example notebook](https://github.com/jinhopark8345/FormUnderstanding/blob/main/notebooks/Fine_tuning_bros_spade_on_FUNSD_entity_linking_dataset.ipynb) for identifying intra-relationships.

If you are looking for information on entity linking versus entity extraction, you can check out the [entity linking explanation vs entity extraction](https://github.com/jinhopark8345/FormUnderstanding) here.

Thank you @jinhopark8345 , this is extremely helpful!