-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Generate using exported model and enable gemma2-2b in ExecuTorch #33707
Conversation
Why am I start getting |
This seems to be happening more often, @ydshieh would you know what might be happening? Otherwise I'll reach out to the CircleCI team, this is hindering work on the repo |
I've asked the CircleCI team @guangy10, very sorry for the inconvenience. |
Might be related to
But not happening on all external contributor's PRs. Strange |
Could you first try ..? https://support.circleci.com/hc/en-us/articles/360048210711-How-to-Refresh-User-Permissions |
Another thing to check If you're following the fork instead of the upstream repo
|
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. |
Weird. Used to work fine on my old PRs. Tried "Refresh Permission". Let's see if it can be unblocked. @ydshieh It's getting worse after re-fresh the permissions. check_circleci_user starts failing. |
7ab24f0
to
e96f0cd
Compare
"In these cases, the user unfollows their fork of the project on CircleCI. " I have no idea how to unfollow my fork on CircleCI. I don't even see if I'm following transformers on CircleCI. I created a CircleCI account with exact email and linked to my github, and I can't see any project I'm following.. |
@ydshieh Their Wiki is so bad. I can't find this setting from anywhere. |
I push a commit to trigger the CI jobs. It runs now. Got to say I have no idea what is wrong on the CircleCI side with the permission issue as other external contributors' PRs don't have it. |
Hi @ydshieh thank you so much for debugging this issue together with me. Per the message from CircleCI support team, it seems like the |
Since @ydshieh pushed a commit to trigger the CI and all CIs are green. Can I get a review on this PR? @amyeroberts @qubvel |
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.
Thanks! 🤗 asked a question for general direction as I am wondering if there is a way for us to improve our generate make it a bit more compatible !
Hi @guangy10 Thank you for the message and contacting CircleCI team on your side too. I also think it is from #33594 from the beginning of seeing this issue. But as you mentioned, only a few (external) contributors face this issue, and the answer from CircleCI attached above is somehow confusing (i.e. it doesn't explain what actually causes it). I will create a new personal github account and see if I can reproduce and come up with a solution. |
@guangy10 Could you go https://app.circleci.com/home/ and see what you get there? 🙏 And https://app.circleci.com/projects/project-dashboard/github/guangy10/ |
Well I am able to reproduce now |
Opened a PR #33866 for this CircleCI issue |
2a67bb9
to
e30275e
Compare
Hey! 🤗 Thanks for your contribution to the Before merging this pull request, slow tests CI should be triggered. To enable this:
(For maintainers) The documentation for slow tests CI on PRs is here. |
@ydshieh I don't see there is a way for me to add |
Thanks for the feedback. We will improve the stuffs. For now I just added it manually here. |
@ArthurZucker @qubvel could you help review for this PR? Once it's merged we can add such integration tests for all executorch-compatible models, not only test the exportability but also the generate. |
Don't forget to push an (empty) commit with message: |
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.
Thanks 🤗
e30275e
to
bc1b056
Compare
Comments addressed. |
Nope, just waiting for the CIs right now! |
|
||
tokenizer = AutoTokenizer.from_pretrained("google/gemma-2b", pad_token="</s>", padding_side="right") | ||
EXPECTED_TEXT_COMPLETION = [ | ||
"Hello I am doing a project on the 1990s and I need to know what the most popular music was in the 1990s. I have looked on the internet and I have found a few things but I need more. I have found that the most popular music was rap", |
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.
@ArthurZucker I see a test failure may be related to this. I copied this over from another model with additional texts added, feel free to truncate it to a shorter message and push a new commit
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.
Ah yeah won't have time to update them tonight! But yeah we can merge otherwise!
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.
@ArthurZucker Let me know if there is anything I can do to merge this PR
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.
Sorry lost track was wondering if you could update the expected values, ortherwise we can merge and @ydshieh will take care of them!
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.
I will check this afternoon and merge
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.
Could you share your torch version ..?
Mine is a dev version 2.6.0.dev20241007
. What is the torch version where the test fails?
attn_implementation = None
It doesn't look correct. If we set it to None
, what is the default attention being used? If I recall correctly, SPDA is the only attention impl that supports StaticCache.
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.
Yeah, torch version seem to be the issue. Let me dig into it and will update here shortly
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.
@ydshieh Okay, I can confirm that exporting gemma2
model will require torch==2.5.0
in order to work correctly.
I also verified that running the test on torch==2.4.1
or torch==2.0.0
will get the original prompt as output.
Here are the detailed package info:
pip list | grep torch
executorch 0.5.0a0+f8cec53
executorchcoreml 0.0.1
torch 2.5.0
torchaudio 2.5.0
torchvision 0.20.0
RUN_SLOW=1 pytest tests/models/gemma2/test_modeling_gemma2.py -k test_export_static_cache -v
=========================================================================================== test session starts ===========================================================================================
platform darwin -- Python 3.10.13, pytest-7.2.0, pluggy-1.0.0 -- /Users/guangyang/miniconda3/envs/executorch/bin/python
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/Users/guangyang/transformers/.hypothesis/examples')
rootdir: /Users/guangyang/transformers, configfile: pyproject.toml
plugins: cov-4.1.0, anyio-4.4.0, xdist-3.3.1, hypothesis-6.84.2
collected 389 items / 388 deselected / 1 selected
tests/models/gemma2/test_modeling_gemma2.py::Gemma2IntegrationTest::test_export_static_cache PASSED [100%]
BTW, the model do require torchvision >= 0.19.0
, will fail with lower version. It's already implied by requiring torch>=2.5.0
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.
I updated the PR to bump up the required torch
version to 2.5.0
.
torch==2.5.0
is not available publicly until Oct 17, 2024. We can either merge this PR as-is, the test_export_static_cache
for gemma2
will be skipped in the CI until torch 2.5.0
is available in transformers
. Or we can defer gemma2
enablement until torch 2.5.0
is available in transformers
by splitting the gemma2
related code out of this PR and merge the rest. Personally I would prefer former, but let me know how you want to proceed. cc: @ArthurZucker @ydshieh
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.
Great! We are still using python 3.8 and torch 2.4.1. I plan to switch to python 3.9 at the end of October. Before that, python 2.5 won't be available in the system. We will think about that.
We can merge this PR as it is. Thank you for checking (it works indeed).
If we set it to None, what is the default attention being used?
Just the native implementation of attention (in the modeling files) using torch operators.
If I recall correctly, SPDA is the only attention impl that supports StaticCache.
The native implementation of attention also works with StaticCache
I believe.
Debug the |
…gingface#33707) * Generate using exported model and enable gemma2-2b in ExecuTorch * [run_slow] gemma, gemma2 * truncate expected output message * Bump required torch version to support gemma2 export * [run_slow] gemma, gemma2 --------- Co-authored-by: Guang Yang <[email protected]>
What does this PR do?
Adding
generate
support for exported model.Adding
gemma2-2b
toExecuTorch
with tests.Adding an integration test for
gemma-2b
that we've enabled already.Additional Test in
ExecuTorch
Running
gemma2-2b
E2E:Before submitting
Pull Request section?
to it if that's the case. Gemma is ExecuTorch compatible #33709
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
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
@gante
@amyeroberts
@qubvel