-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
APIs for fetching and setting model and embedding providers #101
Conversation
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.
@3coins This is awesome work! Thank you very much for getting this out so quickly! 🎉 This is 95% complete; most of my suggestions are minor improvements. I had a few high-level comments in addition to the more granular ones below:
-
Can you create a separate branch (e.g.
model-config
) that's identical tomain
, and then set the target of this PR to that branch? -
I'm tracking naming suggestions in a separate issue that should be addressed before the branch is merged into
main
: Model configurability naming changes #129
|
||
if not provider: | ||
return None | ||
if provider.__class__.__name__ != self.embeddings.__class__.__name__: |
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.
Can just use provider.id
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.
Same for line 85.
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.
Could not do this for the embedding provider. There is a workaround for this, but want to iterate on this in the following PRs.
@dlqqq |
Fixes #102, #103, #105
Summary
This PR introduces the concept of
ProviderConfig
, that provides the values for the llm and embeddings provider and model, along with the api keys for the chat to work.Actors
ProviderConfig
object which is shared globally with other actors above, and is responsible for loading and saving the config from/to the disk.APIs
ProviderConfig
Notes
The anthropic models don't wrap the code segments in the response in a markdown code block; I have tried updating the prompt template and hasn't seen this working yet.
The current setup is not working with certain embeddings, particularly
Cohere
because of the way theAskActor
references the vectorstore, and uses it to get the retriever.Cohere
embeddings has some non-serializable parts, which causes errors when theAskActor
loads it to set the retriever but is unable to de-serialize.The last line here causes an exception
Here is the exception:
Switching embedding providers will cause issues, since the saved index created with one provider is not portable to adding documents after switching to a new provider. In order to make this safely work for users, we have to make sure that the existing indexes are deleted when embeddings provider changes. Users should be aware of this change when they update config.