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

support user-defined embedding dimensions #119

Closed
wants to merge 10 commits into from
Closed

support user-defined embedding dimensions #119

wants to merge 10 commits into from

Conversation

bernard-ng
Copy link
Contributor

support for dimensions for OpenAI Embeddings Generator

see: https://platform.openai.com/docs/api-reference/embeddings/create#embeddings-create-dimensions

dimensions integer Optional
The number of dimensions the resulting output embeddings should have. Only supported in text-embedding-3 and later models.

@MaximeThoonsen
Copy link
Collaborator

MaximeThoonsen commented May 12, 2024

Hey @bernard-ng, thanks for your contribution on this one.

There is a bug when using with ada-002 : " This model does not support specifying dimensions.". It tries to call with dimension, you should handle the case ada-002.

Can you also change the interface signature everywhere for embedText? I don't understand why we don't have a type error but I guess we should 😅.

If you can add an Integration test (and use your openai key in local to run it), that would be better to prevent other bugs.
You can run it with ./vendor/bin/pest ./tests/Integration/Embeddings/EmbeddingGenerator/OpenAI/OpenAIEmbeddingGeneratorTest.php

Otherwise it's a great improvement! It deserves few lines in the doc. If you don't have time, I'll do it.

Thanks a lot,

Maxime

@bernard-ng
Copy link
Contributor Author

Hey @MaximeThoonsen noted, let's fix that 😅

@bernard-ng
Copy link
Contributor Author

@MaximeThoonsen some design considerations

while working on this issue, I realized that adding this functionality to the interface would mean that all embedding models would have to offer this feature, otherwise it would be like adding this “dimensions” option to all EmbeddingGenerator classes without it being used.

what direction should I take? given that this feature is specific to open ai, it doesn't really make sense for it to be available for mistral or ollama.

@MaximeThoonsen
Copy link
Collaborator

Hey @bernard-ng ,

I suggest you throw an "not implemented" exception when it is not used with OpenAI.
Sound good to you?

@MaximeThoonsen
Copy link
Collaborator

Thanks @bernard-ng, can you rebase and I'll merge it!

@bernard-ng bernard-ng changed the title feat(#111): support user-defined embedding dimensions support user-defined embedding dimensions Jun 4, 2024
@bernard-ng bernard-ng closed this Jul 11, 2024
@bernard-ng bernard-ng deleted the user-defined-embedding branch July 11, 2024 14:47
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