Skip to content
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

Move CONTRIBUTING.md + Add Test for Adding New Models #154

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

NirantK
Copy link
Contributor

@NirantK NirantK commented Mar 18, 2024

No description provided.

@NirantK NirantK enabled auto-merge March 18, 2024 01:24
@NirantK NirantK disabled auto-merge March 18, 2024 01:24
@NirantK NirantK enabled auto-merge (squash) March 18, 2024 01:24
@NirantK NirantK requested a review from KShivendu March 18, 2024 01:25
Copy link
Member

@KShivendu KShivendu left a comment

Choose a reason for hiding this comment

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

The part of checking in notebook is a bit unusual. Ideally it could be a Github workflow that can triggered with a button by providing a branch. But I think it's okay since model merges won't be that frequent.

CONTRIBUTING.md Outdated Show resolved Hide resolved
1. Open Requests for New Models are [here](https://github.com/qdrant/fastembed/labels/model%20request).
2. There are quite a few pull requests that were merged for this purpose and you can use them as a reference. Here is an example: https://github.com/qdrant/fastembed/pull/129
3. Make sure to add tests for the new model
- The CANONICAL_VECTOR values must come from a reference implementation usually from Huggingface Transformers or Sentence Transformers
Copy link
Member

Choose a reason for hiding this comment

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

Please describe what's canonical vector.

Do you mean one has to convert these two

input_texts = [
    "hello world", "flag embedding"
]

Using HF Inference API for the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reference PRs are linked before that sentence

@KShivendu
Copy link
Member

Ohh wait we already have a test for this. Is there any blocker from running it in CI?

Co-authored-by: Kumar Shivendu <[email protected]>
@NirantK
Copy link
Contributor Author

NirantK commented Mar 18, 2024

@KShivendu that test also depends on the CANONICAL_VECTOR_VALUES — that's the ground truth, and contributors mistake test for the implementation

Hence, that needs to be outside of this repository ideally — and I don't want to clutter this repository and make the tests slower than they already are

@NirantK NirantK merged commit 256b226 into main Mar 18, 2024
17 checks passed
@NirantK NirantK deleted the move-contributing branch March 18, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants