-
Notifications
You must be signed in to change notification settings - Fork 424
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 GroqChatTarget (#704) #705
base: main
Are you sure you want to change the base?
Conversation
e5070b8
to
d1355cf
Compare
@microsoft-github-policy-service agree |
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.
Sure, this solution is acceptable for me. We'd need unit tests, though. Can you essentially replicate the openai ones with modifications?
Thanks! I've now added unit tests as well. In the class, I refined the implementation to make it fully compatible with Groq’s API. This includes enforcing the correct API base URL (https://api.groq.com/openai/v1/), updating the environment variables to GROQ_API_KEY and GROQ_MODEL_NAME, and adapting _initialize_non_azure_vars to align with Groq’s authentication and request structure. For the unit tests, I followed the OpenAI ones as a base, adapting them to match the new Groq implementation. |
a04d162
to
1c9b997
Compare
@@ -34,6 +35,7 @@ | |||
"CrucibleTarget", | |||
"GandalfLevel", | |||
"GandalfTarget", | |||
"GroqChatTarget", |
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.
Should we rename to OpenAITextChatTarget
? Because it will work on:
- GPT-4, GPPT-4o, etc (without multi-modal)
- DeepSeek, LLAMA, other "openAI compatible" models that support this format but not OpenAI's dictionary specs.
And if we go that route, we could potentially move under the openAI folder.
I'm still looking into whether I agree with my own recommendation though...
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.
^^^ @romanlutz I recommend making my comment non-blocking. Not sure I have enough time to look into it today. But it may be nice to have an open-ai text target that will work other places wherever they pop up.
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.
My main goal was to ensure compatibility with Groq's API while keeping the implementation simple and consistent with OpenAI's existing structure. Renaming it to something more general like OpenAITextChatTarget could make sense if we anticipate supporting more OpenAI-compatible platforms with similar constraints.
Do you think it’s worth making that change now, or should we wait and see if more platforms adopt these constraints?
fcba28e
to
d666166
Compare
Description
Adds GroqChatTarget, a new chat target for interacting with Groq’s OpenAI-compatible API.
messages.0.content
to be a string, while OpenAI/Azure accept a list of dictionaries.Changes
pyrit/prompt_target/groq_chat_target.py
)OpenAIChatTarget
_complete_chat_async
to ensure content is formatted correctly for Groqtests/unit/target/test_groq_chat_target.py
) adapted from OpenAIChatTarget testsdoc/code/targets/groq_chat_target.py
)Related Issue
#704