Skip to content

qzhou-embedding model_meta & implementation #2975

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Aug 7, 2025
Merged

qzhou-embedding model_meta & implementation #2975

merged 12 commits into from
Aug 7, 2025

Conversation

PennyYu123
Copy link
Contributor

I have corrected the unreasonable part in the implementation and resubmitted the PR. Thank you.

If you add a model or a dataset, please add the corresponding checklist:

@Samoed
Copy link
Member

Samoed commented Aug 3, 2025

Why do you creating and closing PRs? Can you work in one PR?

@KennethEnevoldsen
Copy link
Contributor

To expand on @Samoed's comment. Please make your changes in the existing PR, otherwise it makes it hard for us to keep track of. If you do end up making a new PR, then please reference the old one (there are many submissions to keep track of)

@PennyYu123
Copy link
Contributor Author

To expand on @Samoed's comment. Please make your changes in the existing PR, otherwise it makes it hard for us to keep track of. If you do end up making a new PR, then please reference the old one (there are many submissions to keep track of)

Okay, sorry to bother you, I will continue from our first PR

@PennyYu123
Copy link
Contributor Author

Why do you creating and closing PRs? Can you work in one PR?

OK， I'll reorganize it and continue from the first PR

KennethEnevoldsen
KennethEnevoldsen requested changes Aug 3, 2025
View reviewed changes
@KennethEnevoldsen
Copy link
Contributor

OK， I'll reorganize it and continue from the first PR

Just continue from this one, but link the other PRs in the top comment

@PennyYu123
Copy link
Contributor Author

OK， I'll reorganize it and continue from the first PR

Just continue from this one, but link the other PRs in the top comment

OK, then I will not raise a new PR and continue from this one. I will carefully check and correct the issues you mentioned above.

@PennyYu123
Update qzhou_models.py
26c5e9b 
Processing todo items（Add default instruction）
@PennyYu123
Update qzhou_models.py
88822b8 
correct bge datalist
@PennyYu123
Update qzhou_models.py
b33036c 
correct 'public_training_data'
@PennyYu123
Copy link
Contributor Author

I made the changes as you requested. Thank you for reviewing it again.
All of past PRs：
#2965 my first pr
#2973 second pr，You suggested that I integrate the instruction list into the model implementation
#2974 third pr，I just created it and closed it myself

KennethEnevoldsen
KennethEnevoldsen approved these changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@KennethEnevoldsen KennethEnevoldsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are getting there. There is some clean up and @Samoed can you take an extra look at the model impl. I think we can move the complexity over to HF.

@PennyYu123
Copy link
Contributor Author

I have corrected all the suggestions you made this time. Thank you for your hard work.

All of past PRs：
#2965 my first pr
#2973 second pr，You suggested that I integrate the instruction list into the model implementation
#2974 third pr，I just created it and closed it myself

Samoed
Samoed reviewed Aug 5, 2025
View reviewed changes
Samoed
Samoed reviewed Aug 5, 2025
View reviewed changes
Copy link
Member

@Samoed Samoed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Samoed
Samoed reviewed Aug 5, 2025
View reviewed changes
PennyYu123
PennyYu123 commented Aug 7, 2025
View reviewed changes
Copy link
Contributor Author

@PennyYu123 PennyYu123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completed

@KennethEnevoldsen
Copy link
Contributor

@PennyYu123 did you forget to push your changes? There are still comments which are unresolved

PennyYu123 and others added 2 commits August 7, 2025 22:21
@PennyYu123 @Samoed
Update mteb/models/qzhou_models.py
0661f83 
Co-authored-by: Roman Solomatin <samoed.roman@gmail.com>
@PennyYu123 @KennethEnevoldsen
Update mteb/models/qzhou_models.py
f13924d 
Co-authored-by: Kenneth Enevoldsen <kenevoldsen@pm.me>
@PennyYu123
Copy link
Contributor Author

@PennyYu123 did you forget to push your changes? There are still comments which are unresolved

Ok I rechecked all changes and pushed.

PennyYu123
PennyYu123 commented Aug 7, 2025
View reviewed changes
Copy link
Contributor Author

@PennyYu123 PennyYu123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just format the qzhou_models.py file to make it more standardized

@PennyYu123
Copy link
Contributor Author

Now all the checks have passed 😄

@Samoed Samoed merged commit 6c1f1c6 into embeddings-benchmark:main Aug 7, 2025
10 checks passed
@PennyYu123 PennyYu123 deleted the qzhou-embedding-model-meta branch August 7, 2025 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Samoed Samoed Samoed left review comments

@KennethEnevoldsen KennethEnevoldsen Awaiting requested review from KennethEnevoldsen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PennyYu123 @Samoed @KennethEnevoldsen