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: add groq api and model options #108

Closed
wants to merge 3 commits into from

Conversation

gluneau
Copy link

@gluneau gluneau commented Apr 6, 2024

What does this implement

  • Add Groq API and model options

get free API key here: https://console.groq.com/keys

use as usual by naming the groq model in the command:

python run.py --model_name groq-mixtral8x7b \                
 --data_path https://github.com/pvlib/pvlib-python/issues/1603 \
 --config_file config/default.yaml

stop=stop_after_attempt(3),
retry=retry_if_not_exception_type((CostLimitExceededError, RuntimeError)),
)
def query(self, history: list[dict[str, str]]) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

How similar is this and history_to_messages to the OpenaAIModel class? Because if that's just exactly the same, let's just subclass from there to cut down on duplicity.

Copy link
Author

Choose a reason for hiding this comment

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

It is the same since I'm using the OpenAI() client because Groq is design to be compatible with OpenAi.
https://console.groq.com/docs/openai

@klieret
Copy link
Member

klieret commented Apr 8, 2024

Note: #108 and #134 seem to have the same goal.

@crjaensch
Copy link

crjaensch commented Apr 10, 2024

Note: #108 and #134 seem to have the same goal.

I agree. I have no particular preference whether you want to approve #108 or PR #134. In fact, PR #108 seems to have a better separation of concern than the minimal surgical changes proposed by my PR.
However, I would recommend that PR #108 follows the model naming convention that you introduced for example for Azure or Ollama models.
That means, a Groq model like Mixtral 8x7B should be prefixed as groq:<model_name> rather than groq-<model_name> as proposed in PR #108.

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 29.16667% with 17 lines in your changes missing coverage. Please review.

Project coverage is 65.09%. Comparing base (b084c9f) to head (a0c0347).
Report is 873 commits behind head on main.

Files with missing lines Patch % Lines
sweagent/agent/models.py 29.16% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
- Coverage   65.51%   65.09%   -0.43%     
==========================================
  Files          16       16              
  Lines        2117     2140      +23     
==========================================
+ Hits         1387     1393       +6     
- Misses        730      747      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@klieret klieret modified the milestone: 0.2.1 Apr 15, 2024
@EwoutH
Copy link

EwoutH commented Apr 22, 2024

I think groq currently is one of the most interesting options for API-based agents. It's not only the fastest (by far), but also the cheapest, at running many models, including Llama 3.

Would love to see it supported in SWE-agent!

image

@EwoutH EwoutH mentioned this pull request Apr 22, 2024
@asapsav
Copy link

asapsav commented May 12, 2024

hi! just a couple of adds:

  1. in class GroqModel(BaseModel): llama2 should be changed to "llama3-70b-8192"
  2. after a couple of agent iterations (using groq) an "Exit due to cost limit" error appears, which does not make sence cause groq is 0$

@klieret
Copy link
Member

klieret commented May 14, 2024

Apologies letting this PR hang for such a long time. Part of the reason is that models.py is just growing and growing and we should instead just support litellm (potentially leaving claude and gpt as before to make sure the paper results are still reproducible, unless we check it's 100% of a drop-in replacement). The other reason is that so far only the largest models (gpt, claude) were good enough to make sense for swe-agent, which pushed the support for other models down the list of priorities.

But let me come back with a concrete plan later this week!

@klieret
Copy link
Member

klieret commented Oct 9, 2024

Closing because we should use litellm to support more models (#64 )

@klieret klieret closed this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants