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

Separate fast and smart llm providers #813

Merged
merged 15 commits into from
Oct 10, 2024

Conversation

kesamet
Copy link
Contributor

@kesamet kesamet commented Aug 27, 2024

Different LLM sources for "SMART" and "FAST".
For issues #702, #598

@assafelovic
Copy link
Owner

Hey @kesamet sorry it took me long to reply here. First of all this is super super valuable!! We've been getting a lot of requests to use different LLMs for different actions. A bit of feedback:

  1. resolve conflicts based on latest changes
  2. I think it's in the right direction but I'd challenge a solution a bit more complex. Check out how retrievers is built. You can add as many retrievers in comma seperated and they're all populated. Check out here: https://github.com/assafelovic/gpt-researcher/blob/master/gpt_researcher/config/config.py#L54C9-L54C25
  3. I think it might be better to rewrite the config param llm_provider to support multiple llms.
  4. Then we can use the already existing params fast_llm_model and smart_llm_model automatically.

This approach is probably in the right direction and would be optimal. Lmk if you're up for it or I can take it from here!

@kesamet
Copy link
Contributor Author

kesamet commented Sep 16, 2024

Hey @assafelovic, thanks for the feedback and sorry for the late reply.

Taking inspiration from AWS Bedrock where the model name is something like "anthropic.claude-3-sonnet-20240229-v1:0", I think it is best to combine LLM_PROVIDER and LLM_MODEL in env vars into a combined name "<llm_provider>:<llm_model>" using semi-colon. I call it "FAST_LLM_NAME" and "SMART_LLM_NAME".

Eg

FAST_LLM_NAME = openai:gpt-4o-mini
SMART_LLM_NAME = openai:gpt-4o

config/config.py will then split the names into smart_llm_provider, smart_llm_model, etc params

self.fast_llm_provider = "openai"
self.smart_llm_model = "gpt-4o-mini"
self.smart_llm_provider = "openai"
self.smart_llm_model = "gpt-4o"

What do you think?

@assafelovic
Copy link
Owner

This actually sounds great! Will require some refactoring but love it

@kesamet
Copy link
Contributor Author

kesamet commented Sep 17, 2024

Please help to review the PR.
We can also do the same simplification for embedding env vars, which I can do in another PR.

@assafelovic
Copy link
Owner

Embeddings would be amazing @kesamet as well. Thank you for your contributions, I will dive into this PR in the coming days

@assafelovic
Copy link
Owner

Hey @kesamet is this ready for review?

@kesamet
Copy link
Contributor Author

kesamet commented Oct 4, 2024

@assafelovic yes

@kesamet
Copy link
Contributor Author

kesamet commented Oct 4, 2024

But I noticed that fast_llm_model seems to be no longer in use, right?

@assafelovic
Copy link
Owner

@kesamet it was used to summarize retrieved articles but since we're using embedding retrieval it's in no use right now. Still worth having the configs in case we find other use cases for it

@assafelovic
Copy link
Owner

@kesamet everything looks great, may i ask for one last revision and remove the _NAME from the vars? I think simply FAST_LLM is enough. Thank you and I'll merge!

@kesamet
Copy link
Contributor Author

kesamet commented Oct 5, 2024

@assafelovic Done!

Copy link
Owner

@assafelovic assafelovic left a comment

Choose a reason for hiding this comment

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

Thank you @kesamet this is huge and will open many oppurtunities! I'll send an update on this to the community in a few days

@assafelovic assafelovic merged commit dd62de1 into assafelovic:master Oct 10, 2024
@kesamet kesamet deleted the llm-provider branch October 11, 2024 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants