-
-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[LoRA][1/N]Remove LoRA extra vocab #28382
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
[LoRA][1/N]Remove LoRA extra vocab #28382
Conversation
Signed-off-by: Jee Jee Li <pandaleefree@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a significant step towards simplifying the codebase by removing the logic for LoRA extra vocabulary. The changes are extensive, touching many model files, and appear to be mostly correct and consistent with the goal of the refactoring. However, I've identified a recurring critical issue in several files that will lead to a TypeError at runtime due to incorrect tuple unpacking syntax. These issues need to be addressed to ensure the models function correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
WoosukKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double check: We still keep the default padding for vocab sizes and only remove additional padding from LoRA, right?
Yes |
|
@jeejeelee Dumb question: Can you explain more? When skimming the code briefly, I thought this PR removed default padding too. IIRC, the vocab size should be padded for TP > 1 (or maybe for other reasons). |
|
@WoosukKwon This is a great question, and I apologize for not describing it clearly in the PR. This PR does not change the vocab default padding size; it simply doesn't explicitly pass qwen2 and llama are two types for comparison The current PR does not modify models like llama in order to ensure CI tests can pass. They will be modified in subsequent PRs |
WoosukKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the explanation!
Signed-off-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Signed-off-by: George D. Torres <gdavtor@gmail.com>
Purpose
Part of #23474
This PR first removes code related to lora extra vocab from the most of models (temporarily keeping
llamaandmixtralfor lora tests to pass), then continues to remove lora extra vocab code in subsequent PRs.PS: The entire work is planned to be completed in this week.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.