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

Added 2 new embedding models to Model.cs #193

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wrzr123
Copy link

@wrzr123 wrzr123 commented Feb 7, 2024

Added 2 new embedding models text-embedding-3-small and text-embedding-3-large

Added 2 new embedding models text-embedding-3-small and text-embedding-3-large
@OkGoDoIt
Copy link
Owner

OkGoDoIt commented Feb 7, 2024

We should probably also update the embedding endpoint helper functions to add an optional model parameter (currently it's not one of the parameters because there's only been one model), as well as update the embedding request to support dimensions. But adding these models is a good start.

@wrzr123
Copy link
Author

wrzr123 commented Feb 7, 2024

True! I only added what I needed, and the new models were enough for me, as the model can already be stated in the constructor of the EmbeddingRequest. I'm on holiday for the next week, but if you like I can have a look and enhance the helper functions and also the dimensions.
I also thought about changing the default model to text-embedding-3-small, as the ada-model will probably become deprecated in a while. But this would be a problem with backwards compatibility for many users I guess.

@OkGoDoIt
Copy link
Owner

OkGoDoIt commented Feb 11, 2024 via email

Added 2 new models text-embedding-3-small and text-embedding-3-large
Added model as optional parameter in embedding endpoints
Added dimensions and user in embedding requests
Accidentally leaked API-key is already disabled :)
@wrzr123
Copy link
Author

wrzr123 commented Feb 13, 2024

Alright, I made a new commit which includes

  • the 2 new models as in the initial commit
  • the model as optional parameter in all embedding endpoints
  • enhancement of the embedding request object with dimensions and user property
  • a new unit test which tests the dimensions and using one of the new models

I've already found the option to initialize a custom model, was easy to find when looking at the code base. As I don't want to use customized libraries, I did it that way in my codebase which uses your nuget package. But of course I'm happy to update soon to the new version once available. ;)

One more question regarding running the tests. I've let the embeddings tests run locally, to make sure I didn't break anything. To make it work, I manually inserted my OpenAI API key in the constructors. And of course I forgot to take it out again before committing... I'm glad OpenAI and Github both notified me right away about the issue.
Is there a better way to let the tests run locally? Or do you do it the same way and just always make sure to remove the key before publishing?

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