openai / openai-dotnet

The official .NET library for the OpenAI API
https://www.nuget.org/packages/OpenAI/AbsoluteLatest
MIT License
722 stars 62 forks source link

feat: Added net4.6.2/net8.0. #28

Closed HavenDV closed 3 weeks ago

HavenDV commented 3 weeks ago
stephentoub commented 3 weeks ago

Does adding a net462 target here actually address cited concerns, given that this depends on System.ClientModel for core functionality and it only targets netstandard2.0 and net6.0?

HavenDV commented 3 weeks ago

Good point, didn't notice that. Yes, it is obvious that System.ClientModel should also have the same TargetFrameworks, but it seems a little more complicated

stephentoub commented 3 weeks ago

Good point, didn't notice that. Yes, it is obvious that System.ClientModel should also have the same TargetFrameworks, but it seems a little more complicated

Not sure it's actually needed. Who asking for it?

Petermarcu commented 3 weeks ago

Thanks for looking into thie @HavenDV and helping try to make this library awesome! I think the plan here will be this:

  1. Keep the targets minimal as they are today until we get a report that not having a .NET Framework target is an issue. A lot of very popular libraries ship without this target and are completely fine on 4.6.2. 4.6.1 had a bunch of bugs with .NET standard. 4.6.2 I believe only had some very targeted ones and they were more for libraries building on top of System.Drawing (IIRC).

  2. We will move the netX.0 target up as the lowest one goes out of support. If we see concrete use cases where cross targeting to the newer version is needed for the types and scenarios of this library, we can consider adding another target.

System.ClientModel is following the learnings from the Azure SDK where none of those libraries that have millions of downloads have had any feedback that I know of that they need to add these targets.

I mainly want to make sure we need more targets in the package before we add them because they aren't free. They make the package larger, the build times of consumers slower, and for the project that builds them, it increases the number of assets and scenarios that need to be considered and tested.

Does this sound like a good plan? Again, I appreciate your passion here!

HavenDV commented 3 weeks ago

Thanks for the detailed answer, I appreciate it Overall I agree with the position of waiting for more usecases to apply net4.6.2, I don't use it often myself. But as part of the development of LangChain .NET, we had the people who needed it. Your position is also 100% correct if we take the backend world. But .NET is a little broader, and at the moment the OpenAI SDK is standard when interacting with many other LLMs providers, like Ollama/Together/Anyscale/OpenRouter and others, which opens up non-obvious use cases like a completely offline local client for a local llms, where it can have meaning of .Net Framework

An additional argument for .net8 is the ability to get rid of polyfills, such as in the current parallel PR about trimming support

In any case, these are trivial changes, I'll close this for now, you can apply it a little later if you get additional feedback and it makes sense

Petermarcu commented 3 weeks ago

Thank you. Yeah, we expect people will use the library on .NET Framework. Let's see if there are any issues with that.

The polyfill scenario is one I need to think through a little more. My thought is that if the application that is referencing this library is targeting net8.0 then it will still drop the polyfills as part of build but I'm not sure.

Thanks again.