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 support for partial rotary embeddings in Phi3 model #35947

Merged

Conversation

garg-amit
Copy link
Contributor

What does this PR do?

This PR adds support for partial rotary embeddings in the Phi3 model. Key changes include:

  • Introducing the partial_rotary_factor parameter in Phi3Config to control the percentage of queries and keys using rotary embeddings.
  • Modifying the apply_rotary_pos_emb function to accommodate partial rotary embeddings.
  • Updating validation checks in Phi3Config to ensure the correct application of the rotary embedding dimensions.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@Rocketknight1
Copy link
Member

Hi @garg-amit, are there any trained models using this right now?

@garg-amit
Copy link
Contributor Author

@Rocketknight1 Not at the moment, but I believe users will have more flexibility when experimenting with different rotary embeddings.
Note: This does not affect the current behavior.

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a clean addition to the RoPE code that shouldn't have any backward compatibility effects to me! cc @ArthurZucker @Cyrilvallez for core maintainer review

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the change is quite minimal and not breaking. This is a bit against our philo, but we'll justify later! 🤗

@ArthurZucker ArthurZucker merged commit 0ae93d3 into huggingface:main Feb 14, 2025
10 checks passed
@ykim362
Copy link

ykim362 commented Mar 1, 2025

Thanks! Now, the Phi-4-mini is delivered. :) Justified!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants