[WIP] Add Tapas model
Author: NielsRogge
Pull request: yes

# What does this PR do?

Since the beginning of August, I'm working in my free time on incorporating the [Tapas](https://arxiv.org/abs/2004.02349) algorithm by Google AI in the Transformers library (because this library is awesome and I want to contribute to it!). Tapas is basically a BERT model with some clever modifications for natural language understanding related to **tabular data** (structured data like tables, or even HTML). Adding this model could foster research in this area 😄 

Demo's of my current implementation:
* [colab notebook](https://colab.research.google.com/drive/1feRe1Jyjtw7iZVRiKWulP6WjBW6hBIJE?usp=sharing) to showcase `TapasForQuestionAnswering` on WTQ (WikiTable Questions by Stanford University)
* [colab notebook](https://colab.research.google.com/drive/1CDPUr7c8uCNnCtAmmFfj91j-sIzdcqym?usp=sharing) to showcase `TapasForQuestionAnswering`on SQA (Sequential Question Answering by Microsoft Research)
* [colab notebook](https://colab.research.google.com/drive/1JDwWrwHSt8KhGBQ57BDlCFZEe0xMGTQA?usp=sharing) to showcase `TapasForSequenceClassification` on TabFact (Table Fact checking, introduced at ICLR this year)

The model weights are available on the [original Github repository](https://github.com/google-research/tapas), and I wrote a conversion script (similar to other models in the Transformers library) to load them into their PyTorch counterpart. 
I suggest reading the [paper](https://arxiv.org/abs/2004.02349) as well as my [notes](https://docs.google.com/document/d/1WIdZX6of1l-c4AmT909PT7Dpj57EfqUh8BBPaf9ztOw/edit?usp=sharing) to gain a full understanding of how the model works and how I implemented it. There's also a [blog post](https://ai.googleblog.com/2020/04/using-neural-networks-to-find-answers.html) by Google AI as well as a [video](https://www.youtube.com/watch?v=cIUtRNhY6Rw&ab_channel=YannicKilcher) by Yannic Kilcher explaining how the algorithm works. 

The main classes are `TapasConfig`, `TapasModel`, `TapasForQuestionAnswering` and `TapasForSequenceClassification` which can all be found in `modeling_tapas.py`. I'm quite sure the models are OK, the output is the same as the Tensorflow implementation. I added a very extensive documentation (docstrings) to all classes, which you can view by running the `make html` command from the docs directory. Feedback appreciated!

However, there are 2 things for which I need some help/opinions to finish this work:

## 1. Making TapasTokenizer fully Transformers-compliant
To implement `TapasTokenizer`, I need some help/opinions. I suggest using Pandas dataframes as the central object for tabular data (as shown in the Colab notebooks above). and let the API be as follows:

```
from transformers import TapasTokenizer
import pandas as pd

data = {'Actors': ["Brad Pitt", "Leonardo Di Caprio", "George Clooney"], 
        'Age': ["56", "45", "59"],
        'Number of movies': ["87", "53", "69"],
        'Date of birth': ["18 december 1963", "11 november 1974", "6 may 1961"]}
table = pd.DataFrame.from_dict(data)
queries = ["When was Brad Pitt born?", 
           "Which actor appeared in the least number of movies?",
           "What is the average number of movies?"]

tokenizer = TapasTokenizer.from_pretrained("tapas-base-finetuned-wtq")
inputs = tokenizer(table=table, queries=queries)
```

Currently I've only implemented the `batch_encode_plus` method of TapasTokenizer, because it's not really clear to me how to make it fully compatible with the Transformers library, since the way that data is prepared for the model is a bit different compared to BERT/RoBERTa/etc (see also my notes above). It's also not straightforward to make it compatible with the different padding/truncation strategies of Transformers. Currently, the way it works is as follows: there’s a function `_get_token_budget` in `tokenization_tapas.py` that calculates the number of tokens left for the flattened table after tokenizing a question. This is currently set to `self.model_max_length - (len(question_tokens) + 2)` (+ 2 for the CLS and SEP tokens), as was done in the original implementation. There is a hyperparameter when initializing `TapasTokenizer` called `drop_rows_to_fit` which drops rows of the table to fit into the token budget if set to `True`. If it’s set to `False` and a table is too big, it throws a `ValueError` indicating 'too many rows'.

## 2. Testing
Currently I've written `test_modeling_tapas.py` (23 tests passed, 5 failed) and `test_modeling_tapas_utilities.py` (9 tests passed). However, there are 4 different settings to use `TapasForQuestionAnswering` (see my notes) and these all need to be tested (currently only 1 setting is tested) - some help here would be great. Besides this, tests should be added to see whether the model can be properly trained, as well as adding `test_tokenization_tapas.py` (which depends on how TapasTokenizer will be implemented). 

Fixes the following issues (people requesting to add the model):
- #4166 
- #4288

## Who can review?
I suggest @sgugger @LysandreJik since we already discussed this on the forum [here](https://discuss.huggingface.co/t/adding-a-new-model-to-transformers-with-additional-dependencies/916/15).
 tokenizers: @mfuntowicz

DISCLAIMER: this is my first PR of my life, never done this before, hopefully I don't mess up anything (just got the Pro Git book 😛). I assume I should not use `git rebase` anymore now since this branch submitted as PR and should only use `git add`, `git commit` and `git push -u origin tapas_v3`? And `git pull origin tapas_v3` in case others make commits to my branch?
Is there a Slack channel where people can help me out in case I have git issues?

Comments:
I think you might need a rebase on the latest master as your PR seems to have taken master from a while ago (all the modifications in the README should not be there for instance). If it messes the diff, we can always close this PR and open a new one, the branch will be safe :-)

EDIT: I've always used `git fetch upstream` and `git rebase upstream/master`, before pushing my local `tapas_v3` branch to my fork on Github. I didn't know that all models starts with 1. now in the README (therefore I manually changed the numbering), should be fixed now, branch is up-to-date.

@sgugger should I keep rebasing this branch everyday, to keep up with master (as long as the code is not being reviewed)?

Also, is it normal that I have to do a force push everytime I perform a rebase and want to push to Github? Because when I want to do simply `git push -u origin tapas_v3`, I always get 
```
(env) PS C:\Users\niels.rogge\Documents\Python projecten\transformers> git push -u origin tapas_v3
To https://github.com/NielsRogge/transformers.git
 ! [rejected]          tapas_v3 -> tapas_v3 (non-fast-forward)
error: failed to push some refs to 'https://github.com/NielsRogge/transformers.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
```

after a local rebase. 

I'm far from being an expert on git and I don't use the command line anyway, so can't really help you with that.

> Hi! Thanks a lot @NielsRogge for implementing this model! It's a great model that definitely deserves its place in the huggingface library, and you did a great job implementing it! I'm reviewing the code below.

Thank you! Great to hear :)

@LysandreJik I addressed all of the comments. To summarize:
* `TapasConfig` and `TapasTokenizer` now inherit from `PretrainedConfig` and `PreTrainedTokenizer` respectively. For `TapasTokenizer`, a lot of the code of `tokenization_bert.py` was copied (such as the `BasicTokenizer` and `WordPieceTokenizer` classes), since the tokenization logic of text itself is the same. However, some things are different (see `tokenization_tapas.py`). 
* `Modeling_tapas_utilities.py` and `tokenization_tapas_utilities.py` are also gone now, they are added to the bottom of `modeling_tapas.py` and `tokenization_tapas.py` respectively. 
* `pandas` is not a dependency in the code (`pandas` is not imported in any of the files), but I assume you want to switch to `datasets` so that people can use Tapas using only the `Transformers` library? However, currently, in `tokenization_tapas.py` some Pandas logic is used, for example in the `_tokenize_table` function `.iterrows()` is used, so this will require some changes.
* concerning git, I assume I should stop rebasing at some point? I can do it as long as I'm the only one committing?

Hey @LysandreJik, thank you for your feedback.

I've fixed all comments that you had, apart from the tokenizer itself. 

> This is a complicated part, so please let us know if you would like some help along the way/if you want us to take over from here, in which case we would open PRs against your branch with proposals.

I'm happy to accept PRs against my branch, because it's not that clear to me how the tokenizer should be implemented in the best possible way. Does that mean I should stop rebasing my branch with `upstream/master`? Since I read about the "golden rule of rebasing", which states to "never use it on public branches" 😄 



Alright, I'll take a look and open a PR on your fork with the proposed changes. Yes, please don't rebase on `master` anymore as it would mess up my history as I start working on your branch.

We can keep it as it is until we merge now, and fix the merge conflict as the last step.

Hi @NielsRogge, I'm nearly done with the tokenizer changes, but we're focusing on getting version v3.5.0 out today and tomorrow. I'll try to open a PR on your repository then. Please hold off from adding commits now, as rebasing/merging would be very painful now! :smile: 

@LysandreJik no worries, I'm not working on the `tapas_v3` branch.

However, there's still some important work left to do in terms of preparing the data for the model. In the original implementation, this is done in 3 steps:

## 1. TSV in the SQA format
Any dataset (SQA, WTQ, WikiSQL) is first transformed into a TSV with the same columns as the SQA format:
* id: id of the table-question pair, if applicable.
* annotator: id of the annotator, if applicable.
* position: integer indicating if the question is the first, second, third,... related to the table. Only required in case of conversational setup (such as SQA)
* question: string
* table_file: string, name of a csv file containing the tabular data
* answer_text: list of strings (each string being a cell value that is part of the answer)
* answer_coordinates: list of string tuples (each tuple being a cell coordinate, i.e. row, column pair that is part of the answer)
* aggregation_label: only required in case of strong supervision for aggregation (such as WikiSQL-supervised)
* answer_float: float answer to the question. Only required in case of weak supervision for aggregation (such as WTQ and WikiSQL)

If people want to fine-tune `TapasForQuestionAnswering` on their own dataset, they must prepare it in this TSV format, and associated csv files containing the tabular data. It would be great if we can upload all datasets (SQA, WTQ, WikiSQL and WikiSQL-supervised) in SQA format to the HuggingFace datasets hub (they can easily be obtained from the official Tapas Github repo).

## 2. Intermediate format: Interaction
Next, each table-question pair is transformed into an intermediate protocol buffer message which the authors call **Interaction**. Its properties are defined [here](https://github.com/google-research/tapas/blob/master/tapas/protos/interaction.proto), and include things like Table, Question, Answer, AnswerCoordinate, Cell, NumericValue, NumericValueSpan, etc. Populating all the fields of an Interaction based on the TSV is defined in [interaction_utils.py](https://github.com/google-research/tapas/blob/master/tapas/utils/interaction_utils.py), [interaction_utils_parser.py](https://github.com/google-research/tapas/blob/master/tapas/utils/interaction_utils_parser.py), [number_annotation_utils.py](https://github.com/google-research/tapas/blob/master/tapas/utils/number_annotation_utils.py), [number_utils.py](https://github.com/google-research/tapas/blob/master/tapas/utils/number_utils.py) and [text_utils.py](https://github.com/google-research/tapas/blob/master/tapas/utils/text_utils.py).

## 3. tf.train.Example
Finally, each interaction is transformed into an actual training example (`tf.train.Example`), containing the input_ids, mask, etc. as `tf.train.Feature` objects. This is defined in [tf_example_utils.py](https://github.com/google-research/tapas/blob/master/tapas/utils/tf_example_utils.py).

_________________
`TapasTokenizer` must be able to directly convert a row (or multiple rows, i.e. a batch) from a TSV file into a dictionary with PyTorch tensors as values (in other words, combine steps 2 and 3). The remaining work is basically step 2. As I worked with `Pandas` as standard format for tables in my implementation, my idea was to define regular Python classes for each property of the Interaction proto. That is why I have defined a `NumericValue` class, `NumericValueSpan` class, `Cell` class, `Date` class, etc. in `tokenization_tapas.py`. Instances of these classes are then created each time `TapasTokenizer` is called.

I've noticed that the creation of the numeric values is not entirely correct in the `tapas_v3` branch. I'm now working on a correct implementation of this in a branch called `tapas_v3_up_to_date_with_master` (in which I also regularly rebase with upstream/master). It only involves changes to `tokenization_tapas.py`. The changes can eventually be added to `tapas_v3`. I'll wait until your PR is merged before I add those changes.

So my questions are:
- for each of the `xxx_utils.py` files which are used in step 2, there are corresponding `xxx_utils_test.py` files. Could you help in setting up tests in `test_tokenization_tapas.py`, to make sure we're following the original implementation? 
- I'm still assuming that tables are `pandas` dataframes in `tokenization_tapas.py`. Is this OK? Or do you want to change to `dataset`? Wouldn't it be more logical to have SQA/WTQ/WikiSQL as `dataset` objects, and the actual tables as `pandas` dataframes? Pandas is not a dependency of `tokenization_tapas.py`, but tables must be provided as a Pandas dataframe to `TapasTokenizer`.





Hi @NielsRogge! I just finished the tokenizer and its tests. The tests were kind of painful, as the API is a bit different (accepting a dataframe instead of a string), so I had to override most of the tests.

Here's how you can review:

- I did a `make style` on your branch, as I find the code easier to navigate once it's on par with our style.
- However, this introduces a bunch of changes that would make the PR hard to review.
- In order to circumvent this, I've pushed two branches: 
    - `tapas-style`, which is the exact branch you have, but with `make style` run on it and a few cosmetic adjustments
    - `tapas-final`, which builds on top of `tapas-style` to implement all the tokenizer API and tests
- For you to review, the easiest would be to review the PR I opened [here](https://github.com/huggingface/transformers/pull/8482), which aims to merge `tapas-final` into `tapas-style`. This way you can see the actual changes and only these.
- I described my changes in that PR's description so as not to clog this one.
- I setup a todo list if items remaining on the tokenizer, which are not blocking for the merge.

Please review https://github.com/huggingface/transformers/pull/8482, and tell me if you're okay with the changes. If you're okay, I'll merge `tapas-final` into `tapas-style`, and open a PR on your fork with the branch `tapas-style`, which will have all the changes.

This is one of the best hands-on introduction to git you could ask for :smile: 


Regarding your questions about data processing:

> for each of the xxx_utils.py files which are used in step 2, there are corresponding xxx_utils_test.py files. Could you help in setting up tests in test_tokenization_tapas.py, to make sure we're following the original implementation?

Yes, I can help you with that.

> I'm still assuming that tables are pandas dataframes in tokenization_tapas.py. Is this OK? Or do you want to change to dataset? Wouldn't it be more logical to have SQA/WTQ/WikiSQL as dataset objects, and the actual tables as pandas dataframes? Pandas is not a dependency of tokenization_tapas.py, but tables must be provided as a Pandas dataframe to TapasTokenizer.

Actually, `datasets.Dataset` behave very similarly to `pd.DataFrame`s. Nevertheless, we can start with Pandas DataFrames for now, and change to dataset's Datasets once progress is made

@LysandreJik I have improved the parsing of numeric values of both the question and table in `prepare_for_model` of `tokenization_tapas.py` to reflect the original implementation. What it does is turn the cells of a table into `Cell` objects (with potentially associated `NumericValue` objects) and the question into a `Question` object (with potentially a list of associated `NumericValueSpan` objects), before adding numeric-related features.

Besides this, I have fixed some of the comments I had on [my review of your PR](https://github.com/huggingface/transformers/pull/8482), and commented "done" on the ones that are fixed.

## To do:

- [x] Add correct implementation of `prev_label_ids` in case of a batch of table-question pairs (in case of a batch, all questions should refer to the same table). The implementation should reflect the [original implementation](https://github.com/google-research/tapas/blob/d8638f0909b3de32a85fe7491769d47d645d8e22/tapas/utils/tf_example_utils.py#L1155) as follows: for a given table-question pair in a batch, 

```
prev_label_ids = self.get_answer_ids(
                column_ids, row_ids, table_data, answer_text, answer_coordinates
                )
```

Here, it's important that the `get_answer_ids` function is called with the `column_ids` and `row_ids` of the **current** table-question pair in the batch, but the `answer_text` and `answer_coordinates` of the **previous** table-question pair in the batch. 

- [x] Fix the error that I'm currently having in the colab notebooks above (see first message of this PR), when `answer_coordinates` and `answer_text` are provided to the tokenizer. However, what's weird is that when calling `TapasTokenizer` on the real SQA dev set, everything works fine. Might be that I'm doing something wrong with the coordinates and text I provide?
- [x] Add support for the `drop_rows_to_fit` and `cell_trim_length` attributes of `TapasTokenizer`, which should reflect the original API (see also my [suggestion](https://github.com/huggingface/transformers/pull/8482#discussion_r522131827) on how this could be done for `cell_trim_length`). Also, setting `truncation=True` in `TapasTokenizer` doesn't do anything currently. 
- [x] I've added support for the special [EMPTY] token for empty cells in a table (based on the `format_text` method, see [here](https://github.com/google-research/tapas/blob/4908213eb4df7aa988573350278b44c4dbe3f71b/tapas/utils/tf_example_utils.py#L330)). Does this have implications for the `add_special_tokens` method? I assume not? What about `get_special_tokens_mask`? To be verified.
- [x] **Testing TapasTokenizer:** make sure that the PyTorch tensors that `TapasTokenizer` creates are exactly the same as those of the original implementation on the same input data. I've created a [notebook](https://colab.research.google.com/drive/1MzyO-QSA5PZNCNoWa2EIrSA8UEqyUZVp) that tests this. Currently there's a misalignment due to the fact that the original implementation tokenizes a cell value like "1.0" into ["1", "."], whereas my implementation tokenizes this into ["1", ".", "0"]. Filed a Github issue to resolve this.
- [x] **Testing forward pass:** make sure that `TapasForQuestionAnswering`/`TapasForSequenceClassification` return the same `sequence_output`, `pooled_output`, `logits`, and `loss` tensors as the original implementation on the same input data. I've created notebooks that test this:
- SQA (`tapas_sqa_inter_masklm_base_reset`): [PyTorch](https://colab.research.google.com/drive/14bdSwdzvCF2gDF3L0z58IT1fSNzOXKey#scrollTo=6fvJbFF-xKfh) vs [Tensorflow](https://colab.research.google.com/drive/1KWD187cWDP-lOOwKjwzGtGfVR9UWzZVL#scrollTo=KTlX8ZEuRTBa) is giving me the same output (inference only). UPDATE: also loss calculation is OK, see [PyTorch](https://colab.research.google.com/drive/1z2ZRIBXOTk3Aqh6OYpRak6iJhs7e1l2S#scrollTo=2kakMASqmrG5) vs [Tensorflow](https://colab.research.google.com/drive/1Ba3jARJcAqRTd0uuPxOTjOrruIcXIV54#scrollTo=2ZIdZEJGw5RK).
- WTQ (`tapas_wtq_wikisql_sqa_inter_masklm_base_reset`): [PyTorch](https://colab.research.google.com/drive/1Z4T9ZzMvg3vGZ3dSNWbiA4FVbgMMkq_9#scrollTo=EXS4MmCy8Dti) vs [Tensorflow](https://colab.research.google.com/drive/1klaSP99q2aicwpVV9GrmL5nvrPGqrSPH#scrollTo=SIE7bTJMVuSh). I'm getting the same `sequence_output` and `logits_aggregation` on the same input data :) UPDATE: also loss calculation is OK, see [PyTorch](https://colab.research.google.com/drive/19Uq6k1f1178okv80Julfa0Zg41fvFN9x#scrollTo=LEOCtWmWt2IH) vs [Tensorflow](https://colab.research.google.com/drive/1ScF4R7Au8gbC5lN1ehTQFdDknmr2mMRz#scrollTo=GLgez6jJx9Xc) notebooks.
- Tabfact (`tapas_tabfact_inter_masklm_base_reset`): [PyTorch](https://colab.research.google.com/drive/1JDwWrwHSt8KhGBQ57BDlCFZEe0xMGTQA?usp=sharing#scrollTo=z6esPfPMFH1p) vs [Tensorflow](https://colab.research.google.com/drive/14-6VFjvrIiXsYPQEtv8MN-a8Mpo1UNH7#scrollTo=LYBkiqo38e7l) is giving me the same classification logits, confirming that the relative position embeddings implementation is OK. 
- [x] **Testing backward pass:** I've created a [notebook](https://colab.research.google.com/drive/17L97m7cq7J_pnUHmQW6-ksGGVpbbGryP#scrollTo=y0YzoGY24I0C) that fine-tunes `TapasForQuestionAnswering` on 10 examples of the WTQ test set, just to see if it's able to overfit them. I've tried both with randomly initialized classification heads, as well as with the already-finetuned WTQ model. This seems to work well for the former (it can overfit the cell selection, however for aggregation this seems more difficult - probably due to the weak supervision). However, for the already fine-tuned one, the loss stays zero after the third example already. Is this a bug, or is this possible? Update: confirmed by the author.

Actually, reviewing the code of `modeling_tapas.py`(loss calculation + backward pass) is the most important. 

> @NielsRogge if you want I can take care of the remaining steps for the tokenizer:
> 
> > * Add support for the drop_rows_to_fit and cell_trim_length attributes of TapasTokenizer, which should reflect the original API (see also my suggestion on how this could be done for cell_trim_length). Also, setting truncation=True in TapasTokenizer doesn't do anything currently.
> > * I've added support for the special [EMPTY] token for empty cells in a table (based on the format_text method, see here). Does this have implications for the add_special_tokens method? I assume not? What about get_special_tokens_mask? To be verified.

@LysandreJik ok that would be great!




Thanks for the reviews, I've updated the requested changes and marked the ones I did as resolved. 

@LysandreJik, could you maybe fix the remaining comments? In short:

* remove the encoder-decoder logic of `TapasModel` (only remove this from the API of `TapasModel`, but leave them in the code that's copied from BERT (that the user won't see and won't use). I'll let you do this since I don't want to mess up anything.
* remove some tests and add a slow test as requested above

... then I'll mark these as resolved. Besides these, there's also the truncation of `TapasTokenizer` which should still be implemented. I copied what was left here:
* Add support for the `drop_rows_to_fit` and `cell_trim_length` attributes of `TapasTokenizer`, which should reflect the original API (see also [my suggestion](https://github.com/huggingface/transformers/pull/8482#discussion_r522131827) on how this could be done for `cell_trim_length`). The original implementation can be found [here](https://github.com/google-research/tapas/blob/4908213eb4df7aa988573350278b44c4dbe3f71b/tapas/utils/tf_example_utils.py#L999).
* Add support for the special `[EMPTY]` token for empty cells in a table (see the `_tokenize` method of `TapasTokenizer`, which now uses the `format_text` method as in the [original implementation](https://github.com/google-research/tapas/blob/4908213eb4df7aa988573350278b44c4dbe3f71b/tapas/utils/tf_example_utils.py#L330)). Does this have implications for the `add_special_tokens` method? I assume not? What about `get_special_tokens_mask`? To be verified.
* There was also a small discrepancy between the tokenization of TAPAS and the original implementation, see this [Github issue](https://github.com/google-research/tapas/issues/90#issuecomment-735723963). I don't expect this too big of an issue, but maybe you know more about this.

And then I assume we're done 👍 (finally)

Sure, will do so. Probably tomorrow morning/afternoon!

Closing this one as the most up-to-date is now #9117 .