feat: enable new gemini embedding model #52
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @XiaoConstantine, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request enables the use of a new Gemini embedding model. The changes modify the CreateEmbedding function in pkg/llms/gemini.go to allow specifying the embedding model to use. It updates the request body, URL, and error messages to dynamically use the specified model instead of hardcoding text-embedding-004.
Highlights
- Model Selection: The code now supports specifying the Gemini embedding model to use, providing more flexibility.
- Dynamic Model Handling: The request body, URL, and error messages are updated to dynamically use the specified model.
- Default Model: If no model is specified, the code defaults to using
text-embedding-004.
Changelog
- pkg/llms/gemini.go
- Introduces an option to specify the Gemini embedding model.
- If no model is specified, defaults to
text-embedding-004. - Updates the request body to use the specified model.
- Updates the URL to use the specified model.
- Updates error messages to include the specified model.
- Updates metadata to include the specified model.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the primary use case for embedding models in machine learning?
Click here for the answer
Embedding models are primarily used to map discrete variables to vectors of continuous numbers. In simpler terms, they transform words, phrases, or other types of data into numerical representations that capture semantic relationships, making them suitable for tasks like semantic search, recommendation systems, and text classification.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request enables the use of a new Gemini embedding model by allowing the user to specify the model in the options. The code changes look good overall, but there are a few areas that could be improved for clarity and error handling.
Summary of Findings
- Default Model ID: The default model ID
text-embedding-004is hardcoded in multiple places. It would be better to define it as a constant to avoid duplication and improve maintainability. - Error Context: The error messages include the model ID, which is good. However, consider adding more context to the error messages, such as the input length or other relevant parameters, to aid in debugging.
Merge Readiness
The pull request is almost ready for merging. Addressing the comments related to the default model ID and error context would improve the code quality and maintainability. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| if opts.Model == "" { | ||
| opts.Model = "text-embedding-004" | ||
| } |
There was a problem hiding this comment.
Consider defining text-embedding-004 as a constant to avoid magic strings and improve maintainability. This also makes it easier to change the default model in the future.
| if opts.Model == "" { | |
| opts.Model = "text-embedding-004" | |
| } | |
| const defaultEmbeddingModel = "text-embedding-004" | |
| if opts.Model == "" { | |
| opts.Model = defaultEmbeddingModel | |
| } |
| @@ -513,10 +513,13 @@ func (g *GeminiLLM) CreateEmbedding(ctx context.Context, input string, options . | |||
| for _, opt := range options { | |||
| opt(opts) | |||
| } | |||
| if opts.Model == "" { | |||
| opts.Model = "text-embedding-004" | |||
There was a problem hiding this comment.
Consider adding a check to ensure that the model specified in opts.Model is a valid Gemini embedding model. This can prevent unexpected errors if an invalid model ID is provided.
if opts.Model == "" {
opts.Model = "text-embedding-004"
} else if !isValidGeminiEmbeddingModel(opts.Model) {
return nil, errors.New(errors.InvalidInput, fmt.Sprintf("invalid Gemini embedding model: %s", opts.Model))
}| return nil, errors.WithFields( | ||
| errors.Wrap(err, errors.InvalidInput, "failed to marshal request body"), | ||
| errors.Fields{ | ||
| "model": "text-embedding-004", | ||
| "model": opts.Model, | ||
| "input_length": len(input), | ||
| }) |
There was a problem hiding this comment.
Consider adding the input to the error fields to provide more context for debugging.
return nil, errors.WithFields(
errors.Wrap(err, errors.InvalidInput, "failed to marshal request body"),
errors.Fields{
"model": opts.Model,
"input_length": len(input),
"input": input,
})| return nil, errors.WithFields( | ||
| errors.Wrap(err, errors.LLMGenerationFailed, "failed to read response body"), | ||
| errors.Fields{ | ||
|
|
||
| "model": "text-embedding-004", | ||
| "model": opts.Model, | ||
| }) |
There was a problem hiding this comment.
Consider including the status code in the error message to provide more context for debugging.
return nil, errors.WithFields(
errors.Wrap(err, errors.LLMGenerationFailed, "failed to read response body"),
errors.Fields{
"model": opts.Model,
"statusCode": resp.StatusCode,
})| if resp.StatusCode != http.StatusOK { | ||
| return nil, errors.WithFields( | ||
| errors.New(errors.LLMGenerationFailed, fmt.Sprintf("API request failed with status code %d: %s", resp.StatusCode, string(body))), | ||
| errors.Fields{ | ||
| "model": g.ModelID(), | ||
| "model": opts.Model, | ||
| "statusCode": resp.StatusCode, | ||
| }) |
There was a problem hiding this comment.
The error message includes the status code and the response body, which is helpful. Consider adding a check to truncate the response body if it's too long to prevent excessively large error messages.
if resp.StatusCode != http.StatusOK {
truncatedBody := string(body)
if len(truncatedBody) > 500 { // Example: truncate to 500 characters
truncatedBody = truncatedBody[:500] + "... (truncated)"
}
return nil, errors.WithFields(
errors.New(errors.LLMGenerationFailed, fmt.Sprintf("API request failed with status code %d: %s", resp.StatusCode, truncatedBody)),
errors.Fields{
"model": opts.Model,
"statusCode": resp.StatusCode,
})
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 58.58% 58.56% -0.03%
==========================================
Files 37 37
Lines 4786 4788 +2
==========================================
Hits 2804 2804
- Misses 1840 1842 +2
Partials 142 142
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Okay, I will review the provided information and respond to the pull request issue comment if appropriate. After reviewing the context, there are no issue comments that require a response from me. Therefore, I will not respond at this time. |
close: #51