Longformer for question answering
Author: patil-suraj
Pull request: yes

This PR adds `LongformerForQuestionAnswering` following `RobertaForQuestionAnswering`.

The code is almost identical to `RobertaForQuestionAnswering`, just had to remove `head_mask` parameter from forward method of `RobertaForQuestionAnswering`.

Also trained the model to verify this. You can check inference [here](https://colab.research.google.com/drive/1WGgYuBEzGvkvhGOrQxB94Jr1nwCPyblu?usp=sharing)

@patrickvonplaten 

Comments:
Could you add it to the automodel class too ?

@ibeltagy - could you also take a look here

Thank you @patil-suraj, this looks good. One thing I would suggest is to automatically configure the `attention_mask` with global attention on all the question tokens so that the user doesn't need to worry about it. Global attention is not important for a dataset with short documents like squad but crucial for tasks where the document is long. 
You can check [here](https://github.com/allenai/longformer/blob/master/scripts/triviaqa.py#L280) how we set the global attention mask for TriviaQA. It would be good to have something similar in the forward function of `LongformerForQuestionAnswering`; 

```
if attention_mask is None:
    attention_mask = some_function(input_ids)  # All ones. Twos for question tokens. Zero for padding tokens
else:
   pass # do nothing
```

You will need to assume that you know where the question is in the input, usually at the beginning of the sequence, and usually separated from the rest with a certain metatag. Maybe we need extra input from the user to specify the separator tag. 

Notes about the code [here](https://github.com/allenai/longformer/blob/master/scripts/triviaqa.py#L280):
- you don't need the padding step, it is already implemented in `LongformerModel`
- this code assumes that the question length is the same for all examples, but we can't make that assumption here

@ibeltagy 
I did try creating `attention_mask ` automatically in forward method, but as you said this involves knowing where the question is (before or after the context) and the ids of `bos` and `sep` tokens. So model will need access to `tokenizer` to get ids or they'll need to be hardcoded. If we do this then I'm not sure how it will fit with the rest of the pipeline.

So can we provide this as a utility ? Or can we do this in the tokenizer where the user can provide indices in the original string for which global attention should be applied ?

@patrickvonplaten Could you provide some feedback here ?

good points, @patil-suraj. We already have access to `self.config.pad_token_id`, so maybe we can do the same to get access to `bos_token_id` and `sep_token_id`?

Just checked, `self.config.bos_token_id` and `self.config.eos_token_id` are available but not `self.config.sep_token_id`. How about adding a new argument to the forward function that specifies the separator token? This is more general because there are cases where the user wants to use a different separator token from `sep_token_id`. 

@ibeltagy 
Yes,` self.config.bos_token_id` and `self.config.eos_token_id` are available. If I'm not wrong the `eos`  and `sep`  tokens are same for `LongformerTokenizer`.
So we can do it two ways, either make it available in `self.config`  or pass explicitly to the forward function. 
If we make it available   in `self.config`  then the existing `QuestionAnsweringPipeline` won't need to be modified, and the user can override the `self.config.sep_token_id` if its different from `sep_token_id`

👍  sounds good to me. 

Thanks @ibeltagy , I'll try this and let you know.

In the long run we are planning on having a combined tokenizer and model config, so IMO it would be best to add a hardcoded `config.sep_token_id` to the Longformer config.

Okay, so adding `sep_token_id` in `config` and assuming question is at the beginning, can we do it this way 

```
attention_mask = torch.ones_like(input_ids)

for i in range(input_ids.shape[0]):
  sep_index = (input_ids[i, :] == self.config.sep_token_id).nonzero().min().item()
  attention_mask[i, :sep_index] = 2

  # set 0 for padding values if input is padded
  if self.config.pad_token_id in input_ids[i, :]:
    pad_index = (input_ids[i, :] == self.config.pad_token_id).nonzero().min().item()
    attention_mask[i, pad_index:] = 0
```

does this sound good to you ?

> Okay, so adding `sep_token_id` in `config` and assuming question is at the beginning, can we do it this way
> 
> ```
> attention_mask = torch.ones_like(input_ids)
> 
> for i in range(input_ids.shape[0]):
>   sep_index = (input_ids[i, :] == self.config.sep_token_id).nonzero().min().item()
>   attention_mask[i, :sep_index] = 2
> 
>   # set 0 for padding values if input is padded
>   if self.config.pad_token_id in input_ids[i, :]:
>     pad_index = (input_ids[i, :] == self.config.pad_token_id).nonzero().min().item()
>     attention_mask[i, pad_index:] = 0
> ```
> 
> does this sound good to you ?

Thanks a lot for your effort here @patil-suraj !

1) I would prefer to not have a `for loop` here. I think it'd be nicer to just use tensor operations, exactly like @ibeltagy implemented it here: 
https://github.com/allenai/longformer/blob/e007ba9b52c550048e5981c8385980cc84359bc4/scripts/triviaqa.py#L411
I think you only have to replace `self.tokenizer.eos_token_id` with `self.config.sep_token_id`. 

2) No need to pad the `input_ids` with 
```python 
  # set 0 for padding values if input is padded
  if self.config.pad_token_id in input_ids[i, :]:
    pad_index = (input_ids[i, :] == self.config.pad_token_id).nonzero().min().item()
    attention_mask[i, pad_index:] = 0
```

I think you can remove that part of the code because our tokenizers automatically correctly put the 0 in `attention_mask`. 

Thinking a bit more about I'm actually not anymore 100% sure whether this function should be in the `forward()` function of the `LongformerForQuestionAnswering`. Maybe it would be better to have it in the tokenizer function...not sure...will have to think about it.

Let's implement it in the forward function as suggested for now  :-) 

Thanks, @patil-suraj !  If you don't mind, I want to suggest one more thing to add. I think it will be useful if this function alerts the user when the number of global attention tokens is large or the question is on the wrong side. It will be good to add something like: 

```
if num of global attention positions > max(self.config.attention_window)`:
   logger.warning('something something')
```

@patrickvonplaten, I see why you are thinking it might be better to have it in the tokenizer, but I think that it can quickly get complicated because the global attention setting needs to change based on the task.

@ibeltagy 
I think we will need to alert the user about that in all `Longformer` tasks, so can we add that warning in the base `LongformerModel` instead of `LongformerForQuestionAnswering` ?

@patrickvonplaten 
I did tried to vectorize it, but that code assumes that all the questions in the batch have same length. So I'm not sure if we can make that assumption here . Also looking at this function

``` 
def _get_question_end_index(self, input_ids):
        eos_token_indices = (input_ids == self.tokenizer.eos_token_id).nonzero()
        assert eos_token_indices.ndim == 2
        assert eos_token_indices.size(0) == 2 * input_ids.size(0)
        assert eos_token_indices.size(1) == 2
        return eos_token_indices.view(input_ids.size(0), 2, 2)[:, 0, 1]
```
it seems that it makes the assumption that `eos_token_id/sep_token_id` occurs twice in the input, but if we use the default `sep_token_id` then it occurs three times in the input, if we encode que and context as input pair.

So looking at all this, would it be better if we just provide this as a utility and keep the `forward` method same?

You are right about the variable number of global attention per batch, but it can still be vectorized,

1) In the function you mentioned, the following line

```
 return eos_token_indices.view(input_ids.size(0), 2, 2)[:, 0, 1]
```

needs to change to the following because, as you said, you have 3 sep/eos tokens

```
 return eos_token_indices.view(input_ids.size(0), 3, 2)[:, 0, 1]
```

2) Now given `question_end_index` you can set the `attention_mask`  as follows: 

```
question_end_index = question_end_index.unsqueeze(dim=1)   # size: batch_size x 1
# bool attention mask with True in locations of global attention
attention_mask = torch.arange(input_ids.size(1)).expand_as(input_ids) < question_end_index
attention_mask = attention_mask.int() + 1  # from True, False to 2, 1
```


Thanks ! @ibeltagy 

Awesome work so far @patil-suraj - I think after the changes as discussed above, we can merge this :-) 

Oh and one small thing I forgot to add. Could you add a test for `LongformerQuestionAnswering`. I think you can more or less copy this test here: 
https://github.com/huggingface/transformers/blob/a34a9896ac2a4a33ff9cd805c76eed914c8d8965/tests/test_modeling_bert.py#L311

> Awesome work so far @patil-suraj - I think after the changes as discussed above, we can merge this :-)

Happy to contribute 🤗

> Awesome work so far @patil-suraj - I think after the changes as discussed above, we can merge this :-)

Also should we add support for `QuestionAnsweringPipeline` before merging or should that be done in another PR ?

> > Awesome work so far @patil-suraj - I think after the changes as discussed above, we can merge this :-)
> 
> Also should we add support for `QuestionAnsweringPipeline` before merging or should that be done in another PR ?

Let's do this in another PR :-) 

Ok great, I did a little change in the config @patil-suraj, but it looks good to merge for me now! @patil-suraj can you fix the code quality? It's actually quite easy to do:
1) run `flake8 src/transformers/modeling_longformer.py` - it will show you exactly which lines need to be fixed. In your case, all errors are redundant white spaces.
Just delete and re-add lines 791, 794 and 798 without a white space and delete the trailing (at the end of the line) white spaces in line 800.

@ibeltagy - ok for you to be merged? 

> Ok great, I did a little change in the config @patil-suraj, but it looks good to merge for me now! @patil-suraj can you fix the code quality? It's actually quite easy to do:
> 
> 1. run `flake8 src/transformers/modeling_longformer.py` - it will show you exactly which lines need to be fixed. In your case, all errors are redundant white spaces.
>    Just delete and re-add lines 791, 794 and 798 without a white space and delete the trailing (at the end of the line) white spaces in line 800.

Sure.

# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=h1) Report
> Merging [#4500](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/a34a9896ac2a4a33ff9cd805c76eed914c8d8965&el=desc) will **increase** coverage by `0.21%`.
> The diff coverage is `94.44%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/4500/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #4500      +/-   ##
==========================================
+ Coverage   77.87%   78.09%   +0.21%     
==========================================
  Files         123      123              
  Lines       20566    20617      +51     
==========================================
+ Hits        16016    16100      +84     
+ Misses       4550     4517      -33     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_longformer.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19sb25nZm9ybWVyLnB5) | `97.40% <94.00%> (+14.45%)` | :arrow_up: |
| [src/transformers/\_\_init\_\_.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9fX2luaXRfXy5weQ==) | `99.13% <100.00%> (ø)` | |
| [src/transformers/configuration\_longformer.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2xvbmdmb3JtZXIucHk=) | `100.00% <100.00%> (ø)` | |
| [src/transformers/modeling\_auto.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19hdXRvLnB5) | `78.57% <100.00%> (ø)` | |
| [src/transformers/file\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9maWxlX3V0aWxzLnB5) | `73.44% <0.00%> (-0.42%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl91dGlscy5weQ==) | `88.50% <0.00%> (-0.17%)` | :arrow_down: |
| [src/transformers/modeling\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/4500/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ191dGlscy5weQ==) | `90.41% <0.00%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=footer). Last update [a34a989...a198607](https://codecov.io/gh/huggingface/transformers/pull/4500?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Ok great, all green - merging! Hope that's ok with you @ibeltagy 

Looks great. Thanks, @patil-suraj.

https://colab.research.google.com/drive/1ZwnA8NCKOM4HBvaRRpjuVmAaM--x92hN?usp=sharing

Tried to use the longformer with simpletransformers library and tried out the example but I am getting two different errors.

the first error with simpletransformers is an assertion-error
`AssertionError: There should be exactly three separator tokens in every sample for questions answering`

The second error from example is and tensor error
`TypeError: only integer tensors of a single element can be converted to an index`

> This PR adds `LongformerForQuestionAnswering` following `RobertaForQuestionAnswering`.
> 
> The code is almost identical to `RobertaForQuestionAnswering`, just had to remove `head_mask` parameter from forward method of `RobertaForQuestionAnswering`.
> 
> Also trained the model to verify this. You can check inference [here](https://colab.research.google.com/drive/1WGgYuBEzGvkvhGOrQxB94Jr1nwCPyblu?usp=sharing)
> 
> @patrickvonplaten

Btw @patil-suraj, feel free to upload the model you trained on the model hub. It's a `longformer-base-4096` fine-tuned on Squad no? It'd be great if you can upload the model: https://huggingface.co/transformers/model_sharing.html

> > This PR adds `LongformerForQuestionAnswering` following `RobertaForQuestionAnswering`.
> > The code is almost identical to `RobertaForQuestionAnswering`, just had to remove `head_mask` parameter from forward method of `RobertaForQuestionAnswering`.
> > Also trained the model to verify this. You can check inference [here](https://colab.research.google.com/drive/1WGgYuBEzGvkvhGOrQxB94Jr1nwCPyblu?usp=sharing)
> > @patrickvonplaten
> 
> Btw @patil-suraj, feel free to upload the model you trained on the model hub. It's a `longformer-base-4096` fine-tuned on Squad no? It'd be great if you can upload the model: https://huggingface.co/transformers/model_sharing.html

Yes, I'm training the model as we are speaking :). The previous model was trained with question at the end so I'm training it again 

No rush, whatsoever - it's already been super helpful your PR here :-) 

> https://colab.research.google.com/drive/1ZwnA8NCKOM4HBvaRRpjuVmAaM--x92hN?usp=sharing
> 
> Tried to use the longformer with simpletransformers library and tried out the example but I am getting two different errors.
> 
> the first error with simpletransformers is an assertion-error
> `AssertionError: There should be exactly three separator tokens in every sample for questions answering`
> 
> The second error from example is and tensor error
> `TypeError: only integer tensors of a single element can be converted to an index`

The first assertion error is because the model expects the every input sequence encoded like this
`
 question

 context
`.
The model uses this assumption to set global attention on question tokens automatically. And this is how the longformer tokenizer encodes input pair by default. 
So make sure
1. You are encoding the input with question at the beginning and with 3 sep tokens 
2. input_ids is always a batch of examples

The second error because there's mistake in the example
```
start_scores, end_scores = model(torch.tensor([input_ids]), attention_mask=attention_mask)
```

this line should be
```
start_scores, end_scores = model(torch.tensor([input_ids]),attention_mask=torch.tensor([attention_mask]))
```
Thanks for pointing it out.

> `
 question

 context
`

It will be good to mention this explicitly in the docstring. 

> Yes, I'm training the model as we are speaking

@patil-suraj, I am curious what performance number you are getting. Do you mind sharing your training/evaluation scripts, I want to run a few more evals.


@ibeltagy 
Sure, here's the [colab](https://colab.research.google.com/drive/1zEl5D-DdkBKva-DdreVOmN0hrAfzKG1o?usp=sharing). I trained it bit naively though. Didn't focus much on data processing. I've used the new `nlp` library for dataset and `fast tokenizers` alignment method to get index of answer spans. But the metrics are quite good.
`{'exact_match': 85.14664143803216, 'f1': 91.54157494727959}`

Also the model is available [here](https://huggingface.co/valhalla/longformer-base-4096-finetuned-squadv1) 

@patrickvonplaten 

Very helpful. Thanks, @patil-suraj.

Awesome, thanks a lot for adding the model :-) Also feel free to add a little description, the results and a link to the colab to the model card of your newly added model: https://huggingface.co/valhalla/longformer-base-4096-finetuned-squadv1  This way the community usually uses the model much more

@patil-suraj A very insightful notebook! 
Do you plan to write a similar one for text classification anytime soon?