-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Support for Open LLMs #1082
Support for Open LLMs #1082
Conversation
…onsider them as tests. Adding the shell parameter MODEL_NAME to make example clearer
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
==========================================
- Coverage 84.29% 84.18% -0.11%
==========================================
Files 27 27
Lines 1566 1568 +2
==========================================
Hits 1320 1320
- Misses 246 248 +2 ☔ View full report in Codecov by Sentry. |
I managed to resolve the
|
|
||
```bash | ||
export OPENAI_API_BASE="http://localhost:8000/v1" | ||
export OPENAI_API_KEY="sk-xxx" |
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.
Is this required? Can it not just null? Seems like an arbrirary magic string
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.
According to this: https://llama-cpp-python.readthedocs.io/en/latest/server/#multimodal-models yes.
Although I'm not sure why they do it as such. Didn't find an explanation.
Since we use llama-cpp-python
for open model inference I decided to follow their documentation/convention.
openai.api_key = os.getenv("OPENAI_API_KEY") | ||
|
||
if openai.api_key == "sk-xxx": |
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.
this doesnt look clean to me. A boolean variable called isLocalModel or LocalModel or something would be better
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 was thinking about this quite a lot and then decided that the above is cleanest. And requires the least amount of code changes in the code base. At the price of code readability 🤷♂️
If we introduce LocalModel
variable or sth similar then we need to have a list of what clarifies as a local model. There are many options for open LLMs. Model list isn't as trivial as for the OpenAI case.
In principle we could require the user to set another env variable local_model
and then decide on that.
Not sure. I'm open to suggestions here.
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.
@TheoMcCabe, do you have any follow-up comments on @zigabrencic's explanation?
@ErikBjare @captivus @ATheorell do you guys want to weigh-in on this PR, too? |
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 haven't looked super closely, but looks good!
One thing that struck me when reading Aider's blog is that openrouter.ai has a OpenAI-compatible API which let's users dispatch requests to a wide range of models: from OpenAI and Anthropic models, to custom/open models via Together.ai and other providers.
This might be the fastest way to get started with open models, and doesn't require any special hardware. Worth mentioning in the docs imo!
As discussed in today's meeting I switched to And removed the rest except for this line: |
This PR resolves the issue 943 by adding an explanation on how to use
openLLMs
.Overview of changes:
gpt_engineer/applications/cli/main.py
to work with open models.docs/open_models.md
docs/examples
to help users test if theirLLM
setup works.Feel free to let me know if anything is unclear or has to be modified.
Including the relevant reviewers: @ErikBjare @captivus @ATheorell @viborc