-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[MoE][Refactor] Remove most arguments to FusedMoEMethodBase.apply #29066
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
base: main
Are you sure you want to change the base?
Conversation
|
This pull request has merge conflicts that must be resolved before it can be |
FusedMoEMethodBase.apply
FusedMoEMethodBase.applyThere 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".
|
Looks like we need to land #29067 first? marking draft until that lands 👍 |
669f47c to
67fd46e
Compare
Yeah, sorry this should have been a draft to start with. It needs the other PR to land first. |
bf0aaf9 to
587ee3b
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Bill Nell <[email protected]>
587ee3b to
e850c8b
Compare
LucasWilkinson
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 doing this! this is amazing!
Thanks for the review! |
Signed-off-by: Bill Nell <[email protected]>
Signed-off-by: Tyler Michael Smith <[email protected]>
Purpose
After making
select_expertsa non-static method (#29067), we can avoid passing most of the arguments toFusedMoEMethodBase.applyand get them from the layer directly.This doesn't really decrease flexibility, the MoE quantization methods were already fairly tightly coupled with
FusedMoE. A further step could be makingFusedMoEan abstract base class that simply provides thequant_methodparameters. Then the currentFusedMoEclass would become a subclass of that.Test Plan
CI
Test Result
distributed-tests-2-gpus - looks like a spurious failure
cc @varun-sundar-rabindranath
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.