-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Model] Add support for Qwen2 for embeddings #5611
Conversation
+1 this would be great |
I think ppl can edit the config file to change it from |
Should we expect a merge in the near future? |
@Semihal thanks for the reminder! I just merged with main so if it is green we can land |
Can you pass "pytest tests/models/test_embedding.py"? I got "AssertionError: Not all values are within 0.01 of 1.0" with model "ssmits/Qwen2-7B-Instruct-embed-base" . |
@Nickydusk No it does not pass, it seems there is a correctness issue between the implementations so this is not ready to land. This is low on my priority to investigate so if anyone would like to take over this PR, feel welcome and ping me for review. |
@youkaichao @WoosukKwon @zhuohan123 @simon-mo @DarkLight1337 |
if name.startswith(prefix): | ||
name = name[len(prefix):] | ||
|
||
if "rotary_emb.inv_freq" in name: |
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.
need to add following line for loading model Alibaba-NLP/gte-Qwen2-7B-instruct
if "lm_head.weight" in name:
continue
@mgoin @prattcmp @waters222 @Semihal @starmemda I have opened a new PR #6282 to directly support 'gte-Qwen2' embedding models without modifying its Qwen2ForCausalLM architecture. We can collaborate on reviewing the code and merge it in the near future. |
Can this feature be merged? |
@ybbz The output is not correct/matching HF, as seen in the test. Anybody is welcome to debug it! |
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
This pull request has merge conflicts that must be resolved before it can be |
Ping in case you forgot about this @mgoin . It should be quite straightforward to add embedding models now. |
Closing as superseded by #10184 |
FIX #5600
FIX #5827
FIX #6015
This works for
ssmits/Qwen2-7B-Instruct-embed-base
However, this doesn't work for
Alibaba-NLP/gte-Qwen2-7B-instruct
since its config still says it is aQwen2ForCausalLM
, where we have been relying on embedding models to be of the typeXModel
: https://huggingface.co/Alibaba-NLP/gte-Qwen2-7B-instruct/blob/main/config.json