Skip to content
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

Add the position_imbeddings param to LlamaAttention.forward #105

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

nagic0
Copy link
Contributor

@nagic0 nagic0 commented Jul 24, 2024

The function LlamaAttention.forward() has a new param.

https://github.com/huggingface/transformers/blob/2e113422b3504fe6de821bb9911b24273b11aa9c/src/transformers/models/llama/modeling_llama.py#L317

The param position_embeddings is calculated here.

https://github.com/huggingface/transformers/blob/2e113422b3504fe6de821bb9911b24273b11aa9c/src/transformers/models/llama/modeling_llama.py#L917-L918


If the parameter is not added here, an error will occur when running the llama model:

TypeError: LlamaAttention.forward() got an unexpected keyword argument 'position_embeddings'

@alessandropalla alessandropalla self-requested a review July 24, 2024 08:33
@alessandropalla alessandropalla self-assigned this Jul 24, 2024
@alessandropalla alessandropalla added the good first issue Good for newcomers label Jul 24, 2024
@alessandropalla
Copy link
Contributor

What version of the transformers library raises this issue?

@nagic0
Copy link
Contributor Author

nagic0 commented Jul 24, 2024

What version of the transformers library raises this issue?

After v4.43.0

https://github.com/huggingface/transformers/blob/v4.43.0/src/transformers/models/llama/modeling_llama.py#L317

@alessandropalla
Copy link
Contributor

PR is good. Can you please reformat your file using pre-commit (guide here: https://intel.github.io/intel-npu-acceleration-library/developer.html#developer-guide) in order to fix CI and also please add the transformers >= v4.43.0 to requirements.txt in order to not break new installs?

@alessandropalla
Copy link
Contributor

Thanks you very much, merging this now

@alessandropalla alessandropalla merged commit 2c8997b into intel:main Jul 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants