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

Support for Open LLMs #1082

Merged
merged 28 commits into from
Apr 8, 2024
Merged

Conversation

zigabrencic
Copy link
Collaborator

This PR resolves the issue 943 by adding an explanation on how to use openLLMs.

Overview of changes:

  • minor change in gpt_engineer/applications/cli/main.py to work with open models.
  • Rewritten docs/open_models.md
  • Added docs/examples to help users test if their LLM 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

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 84.18%. Comparing base (164730a) to head (4e7b072).
Report is 28 commits behind head on main.

Files Patch % Lines
gpt_engineer/applications/cli/main.py 0.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@zigabrencic
Copy link
Collaborator Author

I managed to resolve the tox/pytest issues by re-naming the example scripts to openai_api_interface.py and langchain_interface.py

pytest thought that they were "tests" 😉


```bash
export OPENAI_API_BASE="http://localhost:8000/v1"
export OPENAI_API_KEY="sk-xxx"
Copy link
Collaborator

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

Copy link
Collaborator Author

@zigabrencic zigabrencic Mar 24, 2024

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":
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

@viborc
Copy link
Collaborator

viborc commented Mar 27, 2024

@ErikBjare @captivus @ATheorell do you guys want to weigh-in on this PR, too?

Copy link
Collaborator

@ErikBjare ErikBjare left a 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!

@zigabrencic
Copy link
Collaborator Author

As discussed in today's meeting I switched to export LOCAL_MODEL=true.

And removed the rest except for this line: elif os.getenv("LOCAL_MODEL"): which is needed to properly compute local API costs.

@viborc viborc merged commit d00be76 into gpt-engineer-org:main Apr 8, 2024
6 checks passed
@zigabrencic zigabrencic deleted the docs/open-llm-suport branch May 16, 2024 18:32
@zigabrencic zigabrencic restored the docs/open-llm-suport branch May 16, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants