Fix issue of canine forward requiring input_ids anyway #26290
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `forward` requires `input_ids` for deriving other variables in all cases. Change this to use the given one between `input_ids` and `inputs_embeds`
The current `forward` requires (the shape of) `input_ids` for deriving other variables whenever `input_ids` or `inputs_embeds` is provided. Change this to use the given one instead of `input_ids` all the time.
younesbelkada
approved these changes
Sep 21, 2023
There was a problem hiding this comment.
Thanks a lot for your PR !
The method should correctly retrieve the sequence length as the second dimension corresponds to it: https://github.com/huggingface/transformers/blob/main/src/transformers/models/canine/modeling_canine.py#L1042
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
ArthurZucker
approved these changes
Sep 22, 2023
Comment on lines
+1172
to
+1174
| char_attention_mask = self._create_3d_attention_mask_from_input_mask( | ||
| input_ids if input_ids is not None else inputs_embeds, attention_mask | ||
| ) |
There was a problem hiding this comment.
we should use the input_shape defined above, since the function is only used once, no problem changing it
I'm pretty sure we have tests in our CI for that make sure generation work with use cache and in that case, you need this to work. Good for me as is
LysandreJik
approved these changes
Oct 2, 2023
blbadger
pushed a commit
to blbadger/transformers
that referenced
this pull request
Nov 8, 2023
…6290) * fix issue of canine forward requires input_ids anyway The `forward` requires `input_ids` for deriving other variables in all cases. Change this to use the given one between `input_ids` and `inputs_embeds` * fix canine forward The current `forward` requires (the shape of) `input_ids` for deriving other variables whenever `input_ids` or `inputs_embeds` is provided. Change this to use the given one instead of `input_ids` all the time. * fix format * fix format
EduardoPach
pushed a commit
to EduardoPach/transformers
that referenced
this pull request
Nov 18, 2023
…6290) * fix issue of canine forward requires input_ids anyway The `forward` requires `input_ids` for deriving other variables in all cases. Change this to use the given one between `input_ids` and `inputs_embeds` * fix canine forward The current `forward` requires (the shape of) `input_ids` for deriving other variables whenever `input_ids` or `inputs_embeds` is provided. Change this to use the given one instead of `input_ids` all the time. * fix format * fix format
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current
forwardrequires (the shape of)input_idsfor deriving other variables wheneverinput_idsorinputs_embedsis provided. Change this to use the given one instead ofinput_idsall the time.What does this PR do?
Fixes #26288
Who can review?
@ArthurZucker and @younesbelkada