-
Notifications
You must be signed in to change notification settings - Fork 4
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
Ollama provider onboarding #55
Conversation
const bool json_response) { | ||
auto provider = GetProviderType(model_details.provider_name); | ||
switch (provider) { | ||
case FLOCKMTL_OPENAI: |
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 don't like this true / false return, why not raise an exception?
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.
Exception is raised earlier, default case will never execute. Had to add to prevent compiler from generating warning
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.
then let's remove the true/false.
|
||
inline SupportedModels GetModelType(std::string model, std::string provider) { | ||
std::transform(provider.begin(), provider.end(), provider.begin(), [](unsigned char c) { return std::tolower(c); }); | ||
if (provider == "ollama") |
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 don't like the use of strings here and everywhere else. Let's make these constants somewhere so we change them in one place.
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.
Fixed
auto res = available_model.find(user_model_name) != std::string::npos; | ||
|
||
if (res) | ||
return true; |
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 should return res with res being initially set to false. It shouldn't return literals true
and false
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.
Fixed
OllamaModelManager &operator=(OllamaModelManager &&) = delete; | ||
|
||
nlohmann::json CallComplete(const nlohmann::json &json, const std::string &contentType = "application/json") { | ||
std::string url = "http://localhost:11434/api/chat"; |
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.
The url
should be provided as part of the input? it is unclear what the port will be. It is also not necessarily a local server and can be a remote one with a different url
.
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.
Fixed
b3647a8
to
beccfb2
Compare
Users can now employ any locally deployed ollama model. Plus some other infra changes for easier provider onboarding.