-
Notifications
You must be signed in to change notification settings - Fork 466
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
Add support for nomic-ai/nomic-embed-text-v1.5 model #1874
base: main
Are you sure you want to change the base?
Conversation
Thanks for this! 🤗 Can you confirm that the exported model produces the same results as the python version for |
Hi!
Yes I plan to do that. This PR is still a work in progress. Thanks for taking a look!
…On Fri, May 24, 2024 at 7:44 AM Joshua Lochner ***@***.***> wrote:
Thanks for this! 🤗 Can you confirm that the exported model produces the
same results as the python version for 2048 < context length <= 8192?
That would be very helpful!
—
Reply to this email directly, view it on GitHub
<#1874 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFZX33USECLSKIZQMJXSV3ZD4R25AVCNFSM6AAAAABIG65RD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRZGMZDKNJZHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Testing with longer texts:
Relevant output:
|
Nice! As a last check, can you test with 8192 tokens? |
Sure! @xenova here it is, redacting the very long text I threw in there: Note that I am logging how many tokens we end up with in the output:
Output:
The last check about the inputs being close together never finishes because this process seems to consume too much memory (I have 64 GB on an M3 macbook). 💀 |
UpdateTried the test script here with a GPU instance - https://gist.github.com/bhavika/8827463b68a327dfe334a2a7fcc723de
@xenova could I get a review here? |
@fxmarty @mht-sharma Hi! 👋🏽 just wondering if there's any interest in accepting this PR? Anything else I can do to land it now? |
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.
Added a comment about the test that needs to be moved, but otherwise it looks good to me! Thanks a lot!
Could you also add a test in tests/exporters/onnx/test_exporters_onnx_cli.py
with this model? As it uses a custom modeling, you'll need to use trust_remote_code=True
. Please decorate it with @slow
as well (as there is no tiny model for nomic on the Hub).
@fxmarty thanks for the feedback! I can update the tests for sure. Any tips on how to run the test suite/checks for this PR? |
Thank you! You could for example run: |
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. |
@fxmarty could you trigger/approve the workflow runs for this PR please? |
@bhavika To solve
can you add a optimum/tests/exporters/exporters_utils.py Line 303 in d9bd7c3 optimum/tests/exporters/onnx/test_onnx_export.py Lines 321 to 326 in d9bd7c3
test_custom_model accordingly to use PYTORCH_REMOTE_CODE_MODELS .
There is also
not sure why. |
@fxmarty could you approve the workflow runs for this PR? I made some changes to the tests as you suggested and want to see if they work now. |
@fxmarty just bumping this PR! Could we re-run the workflows? |
What does this PR do?
Adds support for nomic-embed-text-v1.5 which is a variation of BERT.
I've tested this PR using the following script:
which yields:
CLI exporter
gives me very different results, and these vary on every error so the diff is sometimes very large:
@xenova any thoughts on what I should check here? I'll test for sequence lengths too but this difference has me worried.
Before submitting
Who can review?