-
Notifications
You must be signed in to change notification settings - Fork 26.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Longformer] Output both local attentions and global attentions when output_attentions=True
-> Good Second Issue
#7514
Comments
output_attentions=True
output_attentions=True
-> Second Good Issue
output_attentions=True
-> Second Good Issueoutput_attentions=True
-> Good Second Issue
I am working on a pull request to address this. I don't see any major challenge so far, but this made me realize how much This means that the interface of Longformers would become incompatible with every other Transformer, but maybe it should be? I don't think that there is a way to plug Longformer What are your thoughts on this @patrickvonplaten? |
I have made the pull request. I checked that the Longformer tests passed with my changes, and I added one more test to check the output of attention probabilities. Quite stupidly I made the pull request to the master branch, I am sorry about this. I left it as is to avoid duplicating pull requests for now. You can reject it and I will make a cleaner pull request to a separate branch. |
sorry to have been so super inactive on this issue :-/ I will find time to solve it in ~1 week :-) . This issue is related as well: https://github.com/huggingface/transformers/pull/8007/files#r514633097. |
No worries, there is no hurry on my side. Anyway, the issue is a little trickier than it looks because you guys have to decide how to encode attention probabilities when they are too large to be represented by a dense matrix. Let me know if there is anything I can do to help. |
Hi @patrickvonplaten. I did not use the 🤗 Transformers since our discussion in November 2020. Today I came back to it ( If I am not mistaken, the issue is in modeling_longformer.py. At lines 1784-1789 the code is
but I think it should be
The same goes for lines 1876 and 2124 (but it is fine for lines 2029 and 2235). |
This sounds correct to me! Would you mind opening a new PR? |
I will do it, no problem. |
I made a minimal pull request #10906. |
🚀 Feature request
Good Second Issue - A more advanced issue for contributors who want to dive more into Longformer's attention mechanism.
Longformer currently only outputs global attentions, which is suboptimal because users might be interested in the local attentions as well. I propose to change the "output_attention" logic as follows in longformer:
attentions
should correspond to the "local" attentions and then we'll add a new output typeglobal_attention
that contains the global_attentions. This is consistent with the naming ofattention_mask
andglobal_attention_mask
IMO and the cleanest way to implement the feature.Implementing this feature would mean to that Longformer will require its own
ModelOutput
class =>BaseModelOutput,
=>LongformerBaseModelOutput
orBaseModelOutputWithGlobalAttention
(prefer the first name though)BaseModelOutputWithPooling,
=> ...Also some tests will have to be adapted.
This is a slightly more difficult issue, so I'm happy to help on it. One should understand the difference between local and global attention and how Longformer's attention is different to e.g. Bert's attention in general.
For more detail check out discussion here: #5646
The text was updated successfully, but these errors were encountered: