-
Notifications
You must be signed in to change notification settings - Fork 828
Add Tosa to LLM extension #15556
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?
Add Tosa to LLM extension #15556
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15556
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New Failures, 1 Pending, 2 Unrelated FailuresAs of commit f4cf8c0 with merge base b17004c ( NEW FAILURES - The following jobs have failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @SS-JIA this one is outside of Arm stuff so we need help with someone to review it, thanks for your time and effort with this. |
|
Requested review from @larryliu0820 |
| llm_config.quantization.pt2e_quantize.value | ||
| ) | ||
| quantizers.append(coreml_quantizer) | ||
| if llm_config.backend.tosa.enabled and llm_config.quantization.pt2e_quantize: |
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.
Is it possible to add a unit test to cover this logic branch?
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.
Let me know what you think
2efc03e to
c4d72e5
Compare
|
@larryliu0820 woul you mind taking another look with the changes after your comments? |
|
@metascroy Is this OK to merge? |
|
Hi @larryliu0820 (and/or @digantdesai ) is this OK after the added test? We have some follow up PR that would be good to get into PR state :) |
|
@larryliu0820 new test case was added - mind taking a look? Thanks in advance! |
SS-JIA
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 as the requested test was added!
SS-JIA
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.
Temporary RC while I double check internal importing the diff
|
Indeed, the new test is failing due to the Arm targets not being visible to the test target. To fix, update: https://github.com/pytorch/executorch/blob/8293fa64d221c1d694723b9c209fe0eeb34a8748/examples/models/llama/tests/TARGETS Add the |
|
Looks good as long as CI jobs are fixed. |
8293fa6 to
9f93123
Compare
|
Rebasing as we just merged a tosa spec change just to make sure all is still working :) |
|
@SS-JIA we fixed the buck2 things and now it needs to be re-imported as we cant merge with the version diff vs meta internal. |
Change-Id: I2cded64d7ffdf3de441bbd7ee73357c8ba0a3749 Signed-off-by: Sebastian Larsson <sebastian.larsson@arm.com>
c974788 to
f4cf8c0
Compare
|
Updated from TARGETS to BUCK |
|
@SS-JIA Can you help reimporting? And maybe review if it's green? |
|
@zingo apologies for the delay. I triggered a re-import but still seeing some failures internally - currently trying to figure out what is needed to get everything green, and will post an update ASAP |
Summary: cc freddan80 per zingo oscarandersson8218 digantdesai larryliu0820 mergennachin cccclai helunwencser jackzhxng Pull Request resolved: pytorch#15556 Differential Revision: D90692071 Pulled By: SS-JIA
|
@Sebastian-Larsson @zingo I created #17183 to demonstrate the fixes are that needed to get things working in Meta-internal repo! To minimize the land time, I think we can either:
I do notice some arm related failures on this PR, though - I presume those are unrelated to these changes, and there are no other planned updates to this PR? |
Landing the PR you created seems like the smoother option if you don't mind! |
This diff is to facilitate the landing of #15556 Differential Revision: [D92280949](https://our.internmc.facebook.com/intern/diff/D92280949/) [ghstack-poisoned]
This diff is to facilitate the landing of #15556 Differential Revision: [D92280949](https://our.internmc.facebook.com/intern/diff/D92280949/) ghstack-source-id: 338222346 Pull Request resolved: #17205
|
Of course. I'll take it from here! |
cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai @larryliu0820 @mergennachin @cccclai @helunwencser @jackzhxng