OkGoDoIt / OpenAI-API-dotnet

An unofficial C#/.NET SDK for accessing the OpenAI GPT-3 API
https://www.nuget.org/packages/OpenAI/
Other
1.83k stars 427 forks source link

Model-string conversions should not be implicit as they lose information #46

Open KeithHenry opened 1 year ago

KeithHenry commented 1 year ago

Model has an implicit conversion to a string using the name of the model: https://github.com/OkGoDoIt/OpenAI-API-dotnet/blob/97fb71b7690c2e3b763ae45b1cc02b2133bb3e55/OpenAI_API/Model/Model.cs#L63-L75

This loses information, as the model also contains additional information, so:

var m1 = new Model("text-davinci-003") { OwnedBy = "openai" };
string s = m1;
Model m2 = s;

bool lost = m2.OwnedBy != m1.OwnedBy;

Conversions that lose information (for instance double to int) should use explicit conversions.

OkGoDoIt commented 1 year ago

Yeah, the model class does have extra info and it's best to use the strongly typed model class. However for many use cases, end users will simply want the name as a string, hence making it easy to convert. I'm a big fan of extra convenience, making end usage as simple as possible in the happy path, but still supporting the complex path.

KeithHenry commented 1 year ago

@OkGoDoIt yeah, implicit is easiest, but you have potential insidious bugs if you ever want that extra info on models. For instance there are a few places where there is an implicit cast to string and back again - if you need the extra owner info that bug will be an absolute pain to track down, but if you don't need that extra info why have the class at all? (You could just use string everywhere)