ProphetNet Positional Embeddings Index Issue
Author: ManavR123
Pull request: no

I am having an issue finetuning a ProphetNet model for question generation. The current error I am running into is an indexing issue when getting the `position_embeddings, position_ids`. In the below line of code, we call the `ProphetNetPositionalEmbeddings` module to get the embeddings

https://github.com/huggingface/transformers/blob/461e8cacf94d1f76367cc9ba2cfd5b9bd3641c81/src/transformers/models/prophetnet/modeling_prophetnet.py#L1221

As you can see, the call isn't passing in anything for the `attention mask` (which I am not sure I fully understand, so I appreciate clarification as to why that is happening) or `position_ids`, which means both will be None by default.  Then, in the forward method for `ProphetNetPositionalEmbeddings`, we see the following logic

https://github.com/huggingface/transformers/blob/461e8cacf94d1f76367cc9ba2cfd5b9bd3641c81/src/transformers/models/prophetnet/modeling_prophetnet.py#L587-L593

Since `attention_mask` is None as noted above, it is set to a tensor of all ones, which makes sense. However, then the `position_ids` would be calculated to be for each sample in the batch a vector from 1 to `max_length`. This is the cause of the indexing issue, I am facing. Should this vector not be from 0 to `max_length - 1`? Is there something I am doing wrong in my setup that would be causing this? Is there some level of logic I am missing that is causing the issue?

I am happy to share more code if needed to give more context, but I believe this issue is isolated to just ProphetNet code. Any thoughts?

Comments:
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.

I believe a "fix" was made to this issue in #10501, but it is still not quite correct I think. The solution was to simply clamp the `position_ids` to be bounded between 0 and `max_length - 1`, but the ids still won't be properly set this way. The current solution will result in a tensor that looks like `[1, 2, ..., max_length - 1, max_length - 1]`. Everything is receiving the wrong position embedding except the last item. I think we also need to subtract `1` from the `position_ids` before clamping it. This should fix the offset issue and then the clamp will prevent any leading 0s in the `attention_mask` from becoming -1.  Is there something else I am missing for why we wouldn't want this?

Hey @ManavR123, 

ProphetNet is actually a bit weird since the position_ids start at 1 and not at 0. This is because ProphetNet was for its most part derived from Bart which actually had 513 position id weights even though only 512 were allowed (the first position id was skipped -> it's the padding_id_token). ProphetNet however has exactly 512 weights, so it actually allows only 511 tokens. Now to nevertheless allow ProphetNet to handle 512 tokens we just clamp the last id which shouldn't make a huge difference in performance. This means all position ids are correct **except** the last one, which is a bug that is accepted since it provides the possibility to run 512 tokens.

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.