-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: added groq provider #494
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
Conversation
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
| } | ||
| } | ||
| } | ||
| let concatenated_content = tool_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baxen is this related to the error you were seeing on the llama3 databricks endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed to only apply the concatenate logic to ollama and groq
| databricks::DatabricksProvider, google::GoogleProvider, ollama::OllamaProvider, | ||
| openai::OpenAiProvider, | ||
| }; | ||
| use crate::providers::groq::GroqProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prob move this up into the super
| use crate::providers::openai_utils::{ | ||
| check_openai_context_length_error, messages_to_openai_spec, openai_response_to_message, | ||
| tools_to_openai_spec, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should prob use the get_openai_usage and handle_response in this one as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| "ollama" => OLLAMA_MODEL, | ||
| "anthropic" => "claude-3-5-sonnet-2", | ||
| "google" => "gemini-1.5-flash", | ||
| "google" => GOOGLE_DEFAULT_MODEL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this seems like something we should standardize in the providers for openai, databricks, and anthropic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| Ok(Self { client, config }) | ||
| } | ||
|
|
||
| fn get_usage(data: &Value) -> anyhow::Result<Usage> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider adding get_usage as a trait for Providers in the base object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. This PR has many changes. I can create a follow-up PR after this PR is merged
|
@lifeizhou-ap I wouldn't worry about image support for groq and llama models for now if they are proving a problem. |
| #[serde(default)] | ||
| max_tokens: Option<i32>, | ||
| }, | ||
| Groq { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing I missed in the google provider and here initially was that we also want to add context_limit and estimate_factor here as well
| host, | ||
| api_key, | ||
| model: ModelConfig::new(model) | ||
| .with_temperature(temperature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context_limit and estimate_factor should go here as well for Groq and Google
| tools: &[Tool], | ||
| ) -> anyhow::Result<(Message, ProviderUsage)> { | ||
| let payload = | ||
| create_openai_request_payload(&self.config.model, system, messages, tools, true)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the boolean argument here (true) is a little confusing. I don't think there is a way to specify kwargs, but maybe setting a var before calling the function (e.g. concat_tool_response_contents=true) and passing that in to the function call would be a little more clear?
zakiali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! left a few comments
* v1.0: (43 commits) feat: openrouter provider (#538) [ui] chore: tidy up gui providers (#537) [ui]: Polish and system theme fix (#533) [ui]: General ui polish to more closely match designs (#532) Latency issue fix with prepare_inference (#535) chore: use cross to build binaries (#507) feat: a system for non developers to augment developer system (#524) fix: Broken open directory and new session buttons in more menu (#520) refactor: move get_usage to provider trait (#506) fix: Make stop button more obvious (#516) fix: Enhance Dark mode menu dots visibility (#517) working on automating release of .zip and binaries and having them on each PR as well (#509) conditionally load memory system in goose server (#508) Adds 'instructions' field to InitializeResult (#511) feat: MCP client sdk (#505) Update cli-release.yml feat: added groq provider (#494) fix: use rust tls (#500) fix: Ldelalande/fix scroll (#504) feat: MCP server sdk (simple version first) (#499) ...
openai_utilsNote:
I could not get the capture screenshot working with groq provider as when the message include the toolResult with image the api returns bad request. So far I found they can categorised into 2 scenarios as below:
For most non-vision models, it does not accept the user message with image although in the doc it says it does.
Error message:
{ "request_id": "req_01jfckkap9fcdv2c4cr4tepnfw", "created_at": "2024-12-18T10:04:15.434Z", "error": { "message": "message[0].content must be a string", "type": "invalid_request_error" } }I also tried
llama-3.2-11b-vision-preview, it does not accept the payload including the user message with image and system message at the same time.Error message:
{ "request_id": "req_01jfcmfx6yf0zbjt4028v4gkcd", "created_at": "2024-12-18T10:19:51.904Z", "error": { "message": "prompting with images is incompatible with system messages", "type": "invalid_request_error" } }I tried with the python cli, it does not work either but with different reason. I did not look in-depth. I feel this feature does not work originally and we can enhance this in the future when we have more usages with groq