Remove unnecessary views of position_ids#26059
Merged
ArthurZucker merged 2 commits intohuggingface:mainfrom Oct 6, 2023
Merged
Remove unnecessary views of position_ids#26059ArthurZucker merged 2 commits intohuggingface:mainfrom
views of position_ids#26059ArthurZucker merged 2 commits intohuggingface:mainfrom
Conversation
Contributor
Author
|
For context, I work in the Torch-MLIR project, which converts PyTorch models into MLIR. Handling the |
ArthurZucker
approved these changes
Sep 8, 2023
Collaborator
ArthurZucker
left a comment
There was a problem hiding this comment.
Fine with me, could you confirm that the slow tests pass using RUN_SLOW pytest tests/models/llama ?
Collaborator
|
pinging @gante for a second look |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
Contributor
Author
|
Running |
Contributor
Author
|
@gante, bumping this :) |
Collaborator
|
He is off for a while so won't be able to review this! le me ping @fxmarty for a second look! 👁️ |
fxmarty
approved these changes
Sep 15, 2023
Contributor
fxmarty
left a comment
There was a problem hiding this comment.
LGTM, saw the same thing. I believe other architectures have the same issue, e.g.
. It could be nice to fix it in this PR as well. Some that may be impacted: codgen, gpt2, gpt_neo, gpt_neox, gptj, gpt_bigcode, imagegpt, llama.@ramiro050 Beware that this code path if position_ids is None: should not be trusted for batched generation, as it assumes that all generated sequences have the same length.
fxmarty
reviewed
Sep 15, 2023
Contributor
Author
Done! PTAL |
view of position_ids in modeling_llamaviews of position_ids
When `position_ids` is `None`, its value is generated using `torch.arange`, which creates a tensor of size `(seq_length + past_key_values_length) - past_key_values_length = seq_length`. The tensor is then unsqueezed, resulting in a tensor of shape `(1, seq_length)`. This means that the last `view` to a tensor of shape `(-1, seq_length)` is a no-op. This commit removes the unnecessary view.
8ce4d78 to
8d3b4a7
Compare
8d3b4a7 to
a0887a6
Compare
Contributor
Author
|
@ArthurZucker @fxmarty, bumping this |
Collaborator
ArthurZucker
left a comment
There was a problem hiding this comment.
Hey! Just wondering why we remove the casting to long which I think is required for backward compatibility!
Comment on lines
-478
to
-480
| if position_ids is not None: | ||
| position_ids = position_ids.view(-1, input_shape[-1]).long() | ||
|
|
Collaborator
There was a problem hiding this comment.
hey! Not sure why this was removed as well? Casting to long is probably required / might be breaking if removed!
Contributor
Author
There was a problem hiding this comment.
The docstring mentions that position_ids should be a tensor of type torch.LongTensor, so there should not be a need for the cast. (position_ids is also marked as having type Optional[torch.LongTensor] in the signature of forward)
Contributor
Author
|
@ArthurZucker, bumping this |
ArthurZucker
approved these changes
Oct 6, 2023
Collaborator
ArthurZucker
left a comment
There was a problem hiding this comment.
Thanks for the fix
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
When
position_idsisNone, its value is generated usingtorch.arange, which creates a tensor of size(seq_length + initial_val) - initial_val = seq_length. The tensor is then unsqueezed, resulting in a tensor of shape(1, seq_length). This means that the lastviewto a tensor of shape(-1, seq_length)is a no-op.Simiarly, when
position_idsis notNone, the documentation for the models specifies that it should be of shape(batch_size, seq_length), making theviewmade in this case also unnecessary.This commit removes the unnecessary views.
@gante @ArthurZucker