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

feat: add gemini vertex model provider #297

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

iwilltry42
Copy link
Contributor

@iwilltry42 iwilltry42 commented Dec 23, 2024

Already implements the /validate endpoint from obot-platform/obot#1019

Ref: obot-platform/obot#872

@thedadams
Copy link
Contributor

My concern with this is that it is not async which we have seen performance issues with non-async model providers.

I noticed that there is a Go package: https://github.com/googleapis/go-genai

In your opinion, would this work for us?

@iwilltry42 iwilltry42 marked this pull request as draft January 7, 2025 20:08
@iwilltry42
Copy link
Contributor Author

Back to draft for the rewrite to Go...

@iwilltry42 iwilltry42 force-pushed the feat/gemini-vertex-model-provider branch 2 times, most recently from 98cb57c to 69fd5cf Compare January 9, 2025 18:57
@iwilltry42
Copy link
Contributor Author

Go Rewrite is done, but I'll continue implementing the embeddings API as well

@iwilltry42 iwilltry42 marked this pull request as ready for review January 10, 2025 12:09
@iwilltry42
Copy link
Contributor Author

iwilltry42 commented Jan 10, 2025

With the Go rewrite, I also the /embeddings endpoint (needed a custom implementation as it's not part of their SDK and I couldn't get it working with what they have there so far... I re-used a bunch of code from knowledge, so it's fine).

@iwilltry42 iwilltry42 force-pushed the feat/gemini-vertex-model-provider branch from c8d48dc to 0fa890a Compare January 10, 2025 12:55
Copy link
Contributor

@thedadams thedadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of nits, but I there are a couple comments that should be addressed.

}

func configure(ctx context.Context) (*genai.Client, error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change

if err := validate(ctx); err != nil {
fmt.Printf("{\"error\": \"%s\"}\n", err)
}
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't want to always exit 1 for `validate.

}

func Run(client *genai.Client, port string) error {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change

Comment on lines +43 to +45
mux.HandleFunc("/v1/models", s.listModels)
mux.HandleFunc("/v1/chat/completions", s.chatCompletions)
mux.HandleFunc("/v1/embeddings", s.embeddings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mux.HandleFunc("/v1/models", s.listModels)
mux.HandleFunc("/v1/chat/completions", s.chatCompletions)
mux.HandleFunc("/v1/embeddings", s.embeddings)
mux.HandleFunc("GET /v1/models", s.listModels)
mux.HandleFunc("POST /v1/chat/completions", s.chatCompletions)
mux.HandleFunc("POST /v1/embeddings", s.embeddings)

http.Error(w, err.Error(), http.StatusInternalServerError)
return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change

return choices, nil
}

func mapToOpenAIChoice(candidates []*genai.Candidate) ([]openai.ChatCompletionChoice, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nits on this function

}

func mapMessagesFromOpenAI(messages []openai.ChatCompletionMessage) ([]*genai.Content, error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change


// embeddings - not (yet) provided by the Google GenAI package
func (s *server) embeddings(w http.ResponseWriter, r *http.Request) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change

defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
http.Error(w, fmt.Sprintf("unexpected status code: %d", resp.StatusCode), http.StatusInternalServerError)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense?

Suggested change
http.Error(w, fmt.Sprintf("unexpected status code: %d", resp.StatusCode), http.StatusInternalServerError)
http.Error(w, fmt.Sprintf("unexpected status code: %d", resp.StatusCode), resp.StatusCode)

Comment on lines +619 to +630
body, err := io.ReadAll(resp.Body)
if err != nil {
http.Error(w, fmt.Sprintf("couldn't read response body: %v", err), http.StatusInternalServerError)
return
}

var embeddingResponse vertexEmbeddingResponse
err = json.Unmarshal(body, &embeddingResponse)
if err != nil {
http.Error(w, fmt.Sprintf("couldn't unmarshal response body: %v", err), http.StatusInternalServerError)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine these with json.NewDecoder(resp.Body).Decode(&embeddingResponse)?

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

Successfully merging this pull request may close these issues.

2 participants