Fix TF Longformer #9348
Fix TF Longformer #9348
Conversation
| if input_ids is not None: | ||
| input_ids = tf.pad(input_ids, paddings, constant_values=pad_token_id) | ||
| if input_ids is not None: | ||
| input_ids = tf.pad(input_ids, paddings, constant_values=pad_token_id) |
There was a problem hiding this comment.
When padding_len==0, then this won't change the input_ids correct?
| @@ -2171,16 +2171,14 @@ def call( | |||
|
|
|||
| # set global attention on question tokens | |||
| if inputs["global_attention_mask"] is None and inputs["input_ids"] is not None: | |||
| if inputs["input_ids"] is None: | |||
| logger.warning( | |||
| "It is not possible to automatically generate the `global_attention_mask`. Please make sure that it is correctly set." | |||
There was a problem hiding this comment.
can we leave this warning?
There was a problem hiding this comment.
We can but here the problem is that we test if inputs["input_ids"] is None inside a if that already test if inputs["input_ids"] is not None, this seems strange
There was a problem hiding this comment.
Agreed with @jplu here ;-)
| ): | ||
| logger.warning( | ||
| f"There should be exactly three separator tokens: {self.config.sep_token_id} in every sample for questions answering. You might also consider to set `global_attention_mask` manually in the forward function to avoid this. This is most likely an error." | ||
| ) | ||
| inputs["global_attention_mask"] = tf.fill(shape_list(inputs["input_ids"]), value=1) |
There was a problem hiding this comment.
this doesn't look correct to me. The "default" global_attention_mask is all 0s so I think it should be:
inputs["global_attention_mask"] = tf.fill(shape_list(inputs["input_ids"]), value=0)Also we could improve the warning a bit by appending a sentence like. Disabling global attention for this forward pass...
| @@ -1523,8 +1523,7 @@ def call( | |||
| training=False, | |||
| ): | |||
| all_hidden_states = () if output_hidden_states else None | |||
| all_attentions = () if output_attentions else None | |||
| all_global_attentions = () if (output_attentions and is_global_attn) else None | |||
| all_attentions = all_global_attentions = () if output_attentions else None | |||
There was a problem hiding this comment.
I like the general direction of this PR! Here we should also run the slow tests to be sure nothing is broken.
IMO, the only thing left to do is to correct the "default" global attention to all 0's instead of 1's (global_attention_mask is different from attention_mask)
|
I have already ran the slow tests as well and they all pass! |
| @@ -2171,16 +2171,14 @@ def call( | |||
|
|
|||
| # set global attention on question tokens | |||
| if inputs["global_attention_mask"] is None and inputs["input_ids"] is not None: | |||
| if inputs["input_ids"] is None: | |||
| logger.warning( | |||
| "It is not possible to automatically generate the `global_attention_mask`. Please make sure that it is correctly set." | |||
There was a problem hiding this comment.
Agreed with @jplu here ;-)
* Fix longformer * Apply style * Remove serving content * Forgot a condition * Apply style * Address Patrick's comments * Fix dtype
What does this PR do?
This PR aims to fix the TF Longformer version in order to make it graph compliant. As seen offline with @patrickvonplaten
all_global_attentionsnow is added in the output whenoutput_attentions=True. The global attentions are filled with zeros in caseis_global_attnis False (see line 897 inTFLongformerSelfAttention.Fix issue
#9333