-
Notifications
You must be signed in to change notification settings - Fork 821
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. ❌ 8 New FailuresAs of commit c974788 with merge base 1a086f0 ( NEW FAILURES - The following jobs have failed:
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. |
Change-Id: I2cded64d7ffdf3de441bbd7ee73357c8ba0a3749 Signed-off-by: Sebastian Larsson <sebastian.larsson@arm.com>
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 re-imported as we cant merge with the version diff vs meta internal. |
cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai @larryliu0820 @mergennachin @cccclai @helunwencser @jackzhxng