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(model-providers): 16 Add Gemini Completion and Embedding Models #56

Merged

Conversation

mateobelanger
Copy link
Member

@mateobelanger mateobelanger commented Oct 11, 2024

🎯 Description & Goal

This pull request adds support for the Google Gemini LLM provider.

  • Add impl. for EmbeddingModel and CompletionModel traits for Google Gemini API
  • Update to the README files,
  • New Gemini-agent/embeddings example

DOCS Updates:

Gemini Models Integration:

Note

  • Anthropic & OpenAi implementation of the CompletionModel trait differ async fn completion(). In OpenAI we add the preamble but not with Anthropic

@mateobelanger mateobelanger linked an issue Oct 11, 2024 that may be closed by this pull request
@mateobelanger mateobelanger changed the title feat(model-provider: 16 Add Gemini Completion and Embedding Models feat(model-providers): 16 Add Gemini Completion and Embedding Models Oct 11, 2024
@mateobelanger mateobelanger added feat model Relevant to new model providers or implementations labels Oct 14, 2024
@mateobelanger mateobelanger marked this pull request as ready for review October 14, 2024 15:10
Copy link
Contributor

@0xMochan 0xMochan left a comment

Choose a reason for hiding this comment

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

Overall approved, but there's a consequence I wrote about related to parsing the additional_parameters into an explicit requests Model that is significant to think about. You might want to collect additional keys into it's own field and then merge that onto the reuqest after the fact (might be good to do this for the other providers in a later PR).

README.md Outdated Show resolved Hide resolved
rig-core/README.md Outdated Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Outdated Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Outdated Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Show resolved Hide resolved
rig-core/src/providers/gemini/client.rs Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Outdated Show resolved Hide resolved
for (key, value) in obj.iter().filter(|(_, v)| !v.is_null()) {
match key.as_str() {
"temperature" => {
if !value.is_null() {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you already check whether value is is null at line 545?

}
}

impl TryFrom<serde_json::Value> for GenerationConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may not need this TryFrom implementation. Instead of doing GenerationConfig::try_from(completion_request.additional_params.unwrap_or_default())?;, you can do serde_json::from_value::<GenerationConfig>(completion_request.additional_params.unwrap_or_default())?. Since GenerationConfig has Option for each field, if the additional params don't contain the generation_config field, it won't fail.

This won't work though if field X of completion_request.additional_params is a string but field X of GenerationConfig expects a float for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mateobelanger is there a reason why you didn't change this?

@mateobelanger mateobelanger self-assigned this Oct 23, 2024
Copy link
Contributor

@marieaurore123 marieaurore123 left a comment

Choose a reason for hiding this comment

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

I added a few comments!

Copy link
Contributor

@cvauclair cvauclair left a comment

Choose a reason for hiding this comment

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

Good first draft but couple things to change (especially the bug which makes GenerationConfig required)

rig-core/examples/gemini_agent.rs Outdated Show resolved Hide resolved
rig-core/examples/gemini_agent.rs Show resolved Hide resolved
rig-core/src/providers/gemini/client.rs Outdated Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Show resolved Hide resolved
rig-core/src/providers/gemini/completion.rs Outdated Show resolved Hide resolved
@mateobelanger mateobelanger force-pushed the feat/model-provider/16-add-gemini-completion-embedding-models branch from 0e33fd8 to 3fb10d6 Compare November 3, 2024 10:42
Copy link
Contributor

@cvauclair cvauclair left a comment

Choose a reason for hiding this comment

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

The bug that cases requests to fail if additional_params is not set still exists!

rig-core/src/providers/gemini/completion.rs Outdated Show resolved Hide resolved
@cvauclair
Copy link
Contributor

@garance-buricatu can you make sure your comments were addressed and approve? Would like to merge this before eod, thanks!

@cvauclair cvauclair merged commit e549b37 into main Nov 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
model Relevant to new model providers or implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Add support for Gemini completion and embedding models
4 participants