Modular phi by h3110Fr13nd · Pull Request #34361 · huggingface/transformers

h3110Fr13nd · 2024-10-23T21:10:38Z

What does this PR do?

Adds Modular Phi #33916

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?

@ArthurZucker @LysandreJik
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.

ArthurZucker · 2024-10-24T15:36:54Z

Awesome PR! 🥳 CC @Cyrilvallez

h3110Fr13nd · 2024-10-25T20:21:40Z

Rebased for failing CIs

ArthurZucker

Hey~
As a first review, let's try to use more strength of inheritance
If you look at the PhiAttention a lot of stuff can be skimmed out because it already exists in Gemma for example. Thus you can only add what's required and use del to remove what's to remove (for example dense is o_proj I think)

PhiForCausalLM and all should inherit from Gemma rather than Llama as they can be 1-1 the same this way!

Congrats on the PR 🤗

Cyrilvallez

Thanks for the PR! 🤗 I let some comments to use more inheritance, you could try to go even further if you can!

src/transformers/models/phi/modular_phi.py

Cyrilvallez · 2024-10-29T13:33:13Z

src/transformers/models/phi/modular_phi.py

This cannot use Llama here as it causes self.classifier to become self.score, which may break weights initialization of pretrained models. You could try to find another model with the same names in the init 🤗

Found LlamaForTokenClassification closest to PhiForTokenClassification. Although it's copied from MptForTokenClassification, But SubClassing it causes issues for build_phi_alibi_tensor (i.e. build_mpt_alibi_tensor).
But I've addressed the issue by deleting self.score and adding self.classifier (basically renaming) from LlamaForTokenClassification.
@Cyrilvallez Let me know if changes required.

HuggingFaceDocBuilderDev · 2024-10-29T13:51:13Z

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.

ArthurZucker · 2024-11-15T21:59:21Z

Feel free to ping us again once ready! 🤗

h3110Fr13nd · 2024-11-15T22:02:33Z

Feel free to ping us again once ready! 🤗

@ArthurZucker . Oh, I think that's all. Suggest me any changes if required.

Cyrilvallez

Nice! Just some final details to take care of 🤗 Sorry for the delay, all the team was in Martinique for a big offsite lately, we just came back

src/transformers/models/phi/modular_phi.py

Cyrilvallez · 2024-11-20T16:10:47Z

src/transformers/models/phi/modular_phi.py

All this code should not be needed, a single pass should be enough as this is identical with GemmaForCausalLM

Cannot possibly remove the forward method completely, as it is required at least for the documentation. Changed to super().forward(.....). @Cyrilvallez

src/transformers/models/phi/modular_phi.py

h3110Fr13nd · 2024-11-25T21:04:21Z

Rebased. @Cyrilvallez

Cyrilvallez

Hey! Sorry for the delay!! Last small comments to cut even more code, then we're good to go! 🤗

src/transformers/models/phi/modular_phi.py

Cyrilvallez · 2024-12-03T22:55:22Z

src/transformers/models/phi/modular_phi.py

This can be removed

Hi. @Cyrilvallez. I'm concerned. That removing this will result in the lm_head with bias=False as it is in Gemma. And I'm skeptic that it will affect the model initialization Or at least the output. I don't think we can neglect the bias. So kept the lm_head with bias=True in PhiForCausalLM.
Let me know if I'm correct or not.

Oh yes sorry, did not notice the bias! Then of course we need to keep it. You can still remove the self.model = ... and the post_init() lines though

src/transformers/models/phi/modular_phi.py

Cyrilvallez

All right, LGTM! Great job thanks!
If #34858 gets merged before this one, you'll just need to make sure to modify accordingly, but it will be extremely straightforward!

h3110Fr13nd · 2024-12-04T18:21:32Z

🤗

…amline initialization

…tion logic

…e past_key_values type

…osition embeddings handling

h3110Fr13nd · 2024-12-11T17:28:59Z

All right, LGTM! Great job thanks!
If #34858 gets merged before this one, you'll just need to make sure to modify accordingly, but it will be extremely straightforward!

Done! @Cyrilvallez

ArthurZucker

Nice! Do you want to include the recent changes in #35235 ? 🤗

ArthurZucker · 2024-12-20T11:12:14Z

src/transformers/models/phi/modeling_phi.py


 class PhiForCausalLM(PhiPreTrainedModel, GenerationMixin):
    _tied_weights_keys = ["lm_head.weight"]
+    _tp_plan = {"lm_head": "colwise_rep"}


for TP, the config needs TP as well!

ArthurZucker · 2024-12-20T11:14:10Z

src/transformers/models/phi/modular_phi.py

+                config.hidden_size // self.num_heads, eps=config.layer_norm_eps, elementwise_affine=True
+            )
+
+        self.rotary_emb = PhiRotaryEmbedding(config=self.config)


should be removed!

ArthurZucker requested a review from Cyrilvallez October 24, 2024 15:36

h3110Fr13nd force-pushed the modular/phi branch from 48303e6 to d942994 Compare October 25, 2024 17:44

ArthurZucker reviewed Oct 29, 2024

View reviewed changes

Cyrilvallez reviewed Oct 29, 2024

View reviewed changes

h3110Fr13nd force-pushed the modular/phi branch from d942994 to 12b3901 Compare November 15, 2024 20:47

Cyrilvallez reviewed Nov 20, 2024

View reviewed changes

h3110Fr13nd force-pushed the modular/phi branch 2 times, most recently from d67d5b8 to 89199ac Compare November 25, 2024 21:03

Cyrilvallez self-requested a review November 27, 2024 14:02

Cyrilvallez reviewed Dec 3, 2024

View reviewed changes

h3110Fr13nd force-pushed the modular/phi branch from 77d1fd3 to 67ebbaf Compare December 4, 2024 06:49

Cyrilvallez approved these changes Dec 4, 2024

View reviewed changes

h3110Fr13nd added 6 commits December 11, 2024 21:56

Modular Phi

9356611

refactor: remove unnecessary blank lines in modular_phi.py

e573b45

refactor: update PhiAttention to inherit from GemmaAttention and stre…

adde071

…amline initialization

refactor: update PhiAttention to use nn.Module and improve initializa…

628bf38

…tion logic

refactor: update PhiForCausalLM to simplify forward method and enhanc…

44c8b2f

…e past_key_values type

refactor: remove deprecated rotary embedding classes and streamline p…

9bf4eae

…osition embeddings handling

h3110Fr13nd force-pushed the modular/phi branch from 67ebbaf to 9bf4eae Compare December 11, 2024 17:10

Make Style Fixup.

cbcfe15

ArthurZucker reviewed Dec 20, 2024

View reviewed changes

Conversation

h3110Fr13nd commented Oct 23, 2024

What does this PR do?

Before submitting

Who can review?

Uh oh!

ArthurZucker commented Oct 24, 2024

Uh oh!

h3110Fr13nd commented Oct 25, 2024

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

Cyrilvallez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h3110Fr13nd Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuggingFaceDocBuilderDev commented Oct 29, 2024

Uh oh!

ArthurZucker commented Nov 15, 2024

Uh oh!

h3110Fr13nd commented Nov 15, 2024

Uh oh!

Cyrilvallez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

h3110Fr13nd commented Nov 25, 2024

Uh oh!

Cyrilvallez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Cyrilvallez left a comment

Choose a reason for hiding this comment

Uh oh!

h3110Fr13nd commented Dec 4, 2024

Uh oh!

h3110Fr13nd commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

h3110Fr13nd Nov 15, 2024 •

edited

Loading

Cyrilvallez left a comment •

edited

Loading

h3110Fr13nd commented Dec 11, 2024 •

edited

Loading