feat(go/plugins): Generic OpenAI plugin #2440
Conversation
feat(openai plugin): Initial openai plugin
feat(openai plugin): Add embedding support
feat(go/openai plugin): added function calling
| o.mu.Lock() | ||
| defer o.mu.Unlock() | ||
| if o.initted { | ||
| panic("compat_oai.Init already called") |
There was a problem hiding this comment.
Return error here instead of panicking.
| } | ||
|
|
||
| // DefineModel defines a model in the registry | ||
| func (o *OpenAICompatible) DefineModel(g *genkit.Genkit, name string, info ai.ModelInfo, provider string) (ai.Model, error) { |
There was a problem hiding this comment.
| func (o *OpenAICompatible) DefineModel(g *genkit.Genkit, name string, info ai.ModelInfo, provider string) (ai.Model, error) { | |
| func (o *OpenAICompatible) DefineModel(g *genkit.Genkit, provider, name string, info ai.ModelInfo) (ai.Model, error) { |
| o.mu.Lock() | ||
| defer o.mu.Unlock() | ||
| if !o.initted { | ||
| panic("OpenAICompatible.Init not called") |
There was a problem hiding this comment.
Return error here instead of panicking.
| o.mu.Lock() | ||
| defer o.mu.Unlock() | ||
| if !o.initted { | ||
| panic("OpenAICompatible.Init not called") |
There was a problem hiding this comment.
Return error here instead of panicking.
| ctx := context.Background() | ||
| g, err := genkit.Init(ctx) | ||
| if err != nil { | ||
| log.Fatalf("failed to create Genkit: %v", err) | ||
| } | ||
|
|
||
| apiKey := os.Getenv("OPENAI_API_KEY") | ||
| apiKeyOption := option.WithAPIKey(apiKey) | ||
| oai := oai.OpenAI{ | ||
| Opts: []option.RequestOption{apiKeyOption}, | ||
| } | ||
|
|
||
| oai.Init(ctx, g) | ||
| genkit.WithPlugins(&oai) |
There was a problem hiding this comment.
| ctx := context.Background() | |
| g, err := genkit.Init(ctx) | |
| if err != nil { | |
| log.Fatalf("failed to create Genkit: %v", err) | |
| } | |
| apiKey := os.Getenv("OPENAI_API_KEY") | |
| apiKeyOption := option.WithAPIKey(apiKey) | |
| oai := oai.OpenAI{ | |
| Opts: []option.RequestOption{apiKeyOption}, | |
| } | |
| oai.Init(ctx, g) | |
| genkit.WithPlugins(&oai) | |
| ctx := context.Background() | |
| g, err := genkit.Init(ctx, genkit.WithPlugins(oai.OpenAI{ | |
| Opts: []option.RequestOption{option.WithAPIKey(os.Getenv("OPENAI_API_KEY"))}, | |
| }) | |
| if err != nil { | |
| log.Fatalf("failed to create Genkit: %v", err) | |
| } |
Curious, is there any reason why you didn't do it like this? I wonder if it's not obvious that it should be called this way. The way you had genkit.WithPlugins(&oai) did nothing -- you had already initialized it yourself and the option isn't passed anywhere.
There was a problem hiding this comment.
Migrated initialization in tests here: kekoawong#10. Will wait for @xavidop to migrate behavior in samples
| } | ||
|
|
||
| func (o *OpenAI) Init(ctx context.Context, g *genkit.Genkit) error { | ||
| err := o.openAICompatible.Init(ctx, g) |
There was a problem hiding this comment.
Nit: here and elsewhere, can do this which is more succinct and preferred:
if err := o.openAICompatible.Init(ctx, g); err != nil {
return err
}
| Multiturn: true, | ||
| Tools: true, | ||
| SystemRole: true, | ||
| Media: true, |
| initted bool | ||
| client *openaiGo.Client | ||
| Opts []option.RequestOption | ||
| Provider string |
There was a problem hiding this comment.
Some comments here would be good. What is provider?
| }, | ||
| { | ||
| name: "float and int fields", | ||
| config: &openai.ChatCompletionNewParams{ |
There was a problem hiding this comment.
We should try not to depend directly on the config type defined elsewhere (like in 3P SDKs) so that the SDKs are implementation details and we could in theory swap out to another or even call the REST API directly. We sometimes implement configuration via native Genkit features so if we have it both natively and in the config, it will be confusing as to what happens when you combine them.
| t.Log("genkit initialized") | ||
|
|
||
| // Initialize the OpenAI plugin | ||
| apiKeyOption := option.WithAPIKey(apiKey) |
There was a problem hiding this comment.
Which other options do we (want to) support? Maybe APIKey should be a top level field in OpenAI like it is in GoogleAI? Better to err on the side of being simple and complete rather than supporting everything that the SDK supports.
feat(go/openai plugin): support additional OpenAI models
| ) | ||
|
|
||
| type OpenAI struct { | ||
| Opts []option.RequestOption |
There was a problem hiding this comment.
This Opts field is never used, in other words, the current implementation does not handle the passed options.
For example, even if the API key is passed via the option like this, the OpenAI client is reading the environment variable instead:
apiKeyOption := option.WithAPIKey("sk-xxxxx")
oai := &openai.OpenAI{
Opts: []option.RequestOption{apiKeyOption},
}There was a problem hiding this comment.
+1, we might want to just bring out APIKey as a top level field in the OpenAI struct.
There was a problem hiding this comment.
fixed in here: kekoawong#11
| t.Logf("streaming response: %+v", finalOutput) | ||
| }) | ||
|
|
||
| t.Run("tool usage with basic completion", func(t *testing.T) { |
There was a problem hiding this comment.
This function calling test is flaky depending on the model. It seems to be related to math.Pow(), and I'll give it a try to fix it.
There was a problem hiding this comment.
I believe this is fixed already
| t.Logf("tool usage with basic completion response: %+v", out) | ||
| }) | ||
|
|
||
| t.Run("tool usage with streaming", func(t *testing.T) { |
There was a problem hiding this comment.
It's flaky, just like the function calling test with basic completion.
There was a problem hiding this comment.
I believe this is fixed already
…ls-and-versions chore(go/openai plugin): add OpenAI model details and versions
chore(refactor): added minor refactors before merging
Adding support for an openai go plugin, along with additional generic support for any providers compatible with the openai (from feature request #2204). Collaboration between @xavidop @yukinagae @kekoawong.
Checklist: