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

Providing OpenAIOptions not really works #16

Open
sherlock1982 opened this issue Apr 23, 2024 · 8 comments
Open

Providing OpenAIOptions not really works #16

sherlock1982 opened this issue Apr 23, 2024 · 8 comments

Comments

@sherlock1982
Copy link

sherlock1982 commented Apr 23, 2024

I want to create a differeny OpenAIService per key. Here's what I tried:

  1. I can use static OpenAIService.CreateService and provide options. This works but generally ServiceProvider should be disposed. Therefore I'm left with hanging undisposed instance of a container. This is an issue on it's own.

  2. I can use AddForgeOpenAIAsTransient in my root container and after call:

     IOpenAIService CreateService(OpenAIOptions options, IServiceProvider serviceProvider, IProviderEndpointService providerEndpointService)
    

This function gives me the impression that I can really provide OpenAIOptions but in reality each service resolves on it's own IApiHttpService that ends up with default IProviderEndpointService with default options inside.

Specifically this code is copypasted in many services:

        _apiHttpService = serviceProvider.GetRequiredService<IApiHttpService>();
        _providerEndpointService = providerEndpointService;

But it's actually wrong because provided providerEndpointService is not passed to _apiHttpService

  1. It doesn't matter what function I call to register services OpenAIProviderEndpointService is registered as singleton. Therefore it is shared and you can't really have different options in one app. Ok you can call a function from step 2. creating this service with options as constructor parameter but that still leaves IApiHttpService out of the box.
@JZO001
Copy link
Owner

JZO001 commented Apr 23, 2024

Hello,

Thanks for the feedback.

1, Need to understand when and where we can use the methods and what are our responsibilities. This method is just for the very simple scenarios, when a developer wants to use it without DI just with a provided options AND when the instance is global with same lifecycle of the running application. In that case, it does not necessary to dispose the internal ServiceProvider.
You can find examples in the Playgrounds, project named "MultipleApiKeyUsage" project.

2, I am trying to follow the description what you wrote, but I do not understand the workflow. After you registered the servuce the "AddForgeOpenAIAsTransient", than why do you call the CreateService method? This method designed to use when you manually create your own IServiceProvider instance and you are not in a hosting model envronment. If you create your own ServiceCollection -> register the services an your own requirements -> build your ServiceProvider and ask an instance of IOpenAIService from your provider instance. This is what exactly happen in the "CreateService(OpenAIOptions options)".

In a hosting model environment, instances will be injected, for example in blazor, or also use the IServiceProvider of the host.

Btw, the method "CreateService(OpenAIOptions options, IServiceProvider serviceProvider, IProviderEndpointService provider)" is wrong, the third option does not necessary, it was left after one of my refactoring. I will remove the third option in the next release and it was acquired from the given "serviceProvider".

3, You are right, it is a singleton. It is mostly okay for the apps, but I know, it is not good for every scenario. But feel free to copy/paste my extension methods and create your own ones which are fit for your requirements. That was the reason why I was not set the class visibilities to "internal" as normal in other libraries to give freedom for the SDK users to configure thier environment as they wish. So just simply copy/paste this "AddForgeOpenAIAsTransient" into your own extension class, give it a name and change registration modes. For example from singleton to transient.

Once again, thank you for your feedback. I would like to make the SDK as flexible as possible, sometimes this lead to misunderstandings and of course, I cannot provide service registrations for every cases in the universe :)

Happy coding!

@sherlock1982
Copy link
Author

sherlock1982 commented Apr 24, 2024

My point is method registrations *AsScoped will not do what you want in all cases.

For example AddForgeOpenAIAsScoped adds scoped OpenAIOptions but OpenAIProviderEndpointService is still registered as a singleton. Therefore it will not get OpenAIOptions from scope but instead will get a global one.

It doesn't matter how many scopes you create because OpenAIProviderEndpointService is a singleton.

The same issue with ApiHttpLoggerService. Singleton but depends on the scoped OpenAIOptions.

If you want *AsScoped to get scoped options you need to register all services that depend on OpenAIOptions as scoped.

As for *AsTransient this is even worse. Because in all services you directly use

  _apiHttpService = serviceProvider.GetRequiredService<IApiHttpService>();

Which means that ApiHttpService will not get the options you provided. Generally direct usage of GetRequiredService is an antipattern called service locator. If you simply require this service via constructor it will be much better because in this case one can inject:

Func<OpenAIOptions, IOpenAIService> 

And provide transient options into each and every location

@sherlock1982
Copy link
Author

sherlock1982 commented Apr 24, 2024

I tried to write an example but it failed unfortunately because of other reasons.
Note that ambigous constructors issue.

I managed to run it in the end in my system cause I'm using Autofac and can actually resolve ambiguity there.

// See https://aka.ms/new-console-template for more information
using Forge.OpenAI;
using Forge.OpenAI.Authentication;
using Forge.OpenAI.Interfaces.Services;
using Forge.OpenAI.Settings;
using Microsoft.Extensions.DependencyInjection;

ServiceCollection services = new ServiceCollection();
services.AddForgeOpenAIAsScoped();

// Need to do it because OpenAIOptions is not anyhow registered
// services.AddScoped<OpenAIOptions>();

using var provider = services.BuildServiceProvider();

var scope1 = provider.CreateScope();
var opt1 = scope1.ServiceProvider.GetRequiredService<OpenAIOptions>();
opt1.AuthenticationInfo = new AuthenticationInfo()
{
    ApiKey = "first_key"
};
// Fails - ambigous constructors
var service1 = scope1.ServiceProvider.GetRequiredService<IOpenAIService>();

var scope2 = provider.CreateScope();
var opt2 = scope2.ServiceProvider.GetRequiredService<OpenAIOptions>();
opt2AuthenticationInfo = new AuthenticationInfo()
{
    ApiKey = "second_key"
};
// Fails - ambigous constructors
var service2 = scope2.ServiceProvider.GetRequiredService<IOpenAIService>();

@JZO001
Copy link
Owner

JZO001 commented Apr 24, 2024

I got you. I am going to fix this in the next release soon.

@sherlock1982
Copy link
Author

Still great thanks for the library - we already integrated and it works.

My only concern was that it's not really friendly for a person who doesn't do deep dive in DI :-)

@JZO001
Copy link
Owner

JZO001 commented Apr 24, 2024

My original plan was also to use the library without DI as well. Not just because a developer does not have experience with, but because it is not possible to use in the target environment. Maybe I need to improve somehow this question and/or create more examples in the Playground

@JZO001
Copy link
Owner

JZO001 commented Apr 24, 2024

I am just curious, what is your project about? :) I have never got feedback yet!

@JZO001
Copy link
Owner

JZO001 commented Apr 24, 2024

Please check v1.4.0, a couple of improvements made, supporting v2 APIs, DI changes, etc.

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

No branches or pull requests

2 participants