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

implement groq completions #2

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Conversation

stargazing-dino
Copy link
Contributor

Implements groq api. See their quickstart

Which for the most part follows the openAI schema.

One relatively larger change this PR made was to the AdapterKind::from_model.

The models from groq do not have any common prefix or suffix to do partial matching on so I had to change it to just match the exact string. For that I made the models pub. I thought to use Adapter::list_models but it required kind to be passed through which was unfeasible since we don't have one at that point and it's also async so it'd require some more changes than just making the models pub to the crate.

I tested this locally and it seemed to work fine.

@jeremychone
Copy link
Owner

First, thanks so much for this PR.

The only thing I'm not a fan of right now is losing the "open" matching for the models that have clear prefixes.

For the from_mod, could we make it so that only Grok has the fixed name match, and the others remain as before?

If you do not have time to make the change, I will do it, no problem.

Having the fixed names for the list_models is okay for now. That will change later as I would like to have them query when the API is available.

Note: by design, the list_models is decoupled from the from_models, as the first one is more for user info and tries to be as live as possible, and the second is more for a wider match focused on in-memory/static processing.

@stargazing-dino
Copy link
Contributor Author

I moved back to the prefix solution :)

Note: by design, the list_models is decoupled from the from_models, as the first one is more for user info and tries to be as live as possible, and the second is more for a wider match focused on in-memory/static processing.

ah makes sense

@jeremychone
Copy link
Owner

Perfect, I will merge this.

So Grok supports the OpenAI API to the letter?
I found one bug in the Ollama OpenAI compatibility layer (it does not support multiple system messages), but I worked around it, so it all works. Hopefully, Grok does not have those types of issues.

@stargazing-dino
Copy link
Contributor Author

So Grok supports the OpenAI API to the letter?

I think so, this is even their endpoint https://api.groq.com/openai/v1/chat/completions

it does not support multiple system messages

I've tested groq and from what I remember it supports multiple system messages (for the llama models I tested at least) fine so no issue there hopefully

@jeremychone
Copy link
Owner

Cool. I found this page: https://console.groq.com/docs/openai

So it seems OpenAI API is their API strategy, so they should support it pretty well.

I am going to add the temperature, max_token, … all in the ChatRequestOptions, so we will get that soon, and that will include Grok.

@jeremychone jeremychone merged commit c519f52 into jeremychone:main Jun 24, 2024
@jeremychone
Copy link
Owner

@stargazing-dino Thanks, I just merged it.

@jeremychone
Copy link
Owner

Btw, very clean code and PR. Thanks!

@stargazing-dino stargazing-dino deleted the groq branch June 24, 2024 11:58
@jeremychone jeremychone added the PR-MERGED Added for PR that somehow did not get the Merge label Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-MERGED Added for PR that somehow did not get the Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants