OpenAI-API-dotnet icon indicating copy to clipboard operation
OpenAI-API-dotnet copied to clipboard

Added 2 new embedding models to Model.cs

Open wrzr123 opened this issue 1 year ago • 4 comments

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

wrzr123 avatar Feb 07 '24 10:02 wrzr123

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.

OkGoDoIt avatar Feb 07 '24 18:02 OkGoDoIt

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.

wrzr123 avatar Feb 07 '24 22:02 wrzr123

Yeah, I think it’s probably best to leave the current default which is important for backwards compatibility. OpenAI has explicitly stated they won’t be deprecating that old model.

Perhaps I need to make this more clear on the documentation, but you can always provide a text string for the model name rather than one of the strongly typed models. So whenever OpenAI releases a new model, you can just pass in the model name without having the modify the SDK at all.

Anyway it’s a pretty hectic weekend for me, but I’ll do my best to double check all this and merge it early in the week.

-Roger

On Wed, Feb 7, 2024 at 2:13 PM wrzr123 @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/OkGoDoIt/OpenAI-API-dotnet/pull/193#issuecomment-1933024808, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJBNJCV4LQEDUP2ICUSNXDYSP4CFAVCNFSM6AAAAABC5TILSGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTGAZDIOBQHA . You are receiving this because you commented.Message ID: @.***>

OkGoDoIt avatar Feb 11 '24 23:02 OkGoDoIt

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?

wrzr123 avatar Feb 13 '24 10:02 wrzr123