-
-
Notifications
You must be signed in to change notification settings - Fork 35
Add dynamic model discovery via /models endpoint #245
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
Conversation
ericdallo
left a comment
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.
That's cool! will take a closer look soon
|
@ericdallo I thought about making this the default when no models are filled in for a provider (for better UX), but fetching models takes some time so I decided not to (yet?). Motivation: I have a lot of providers and I am lazy to fill in all models I might use. |
676b98f to
a03e175
Compare
src/eca/llm_api.clj
Outdated
| callbacks) | ||
|
|
||
| model-config | ||
| (:api provider-config) |
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.
@ericdallo I think this is the only place that can introduce some regression.
I am not quite sure what was the idea behind model-config check.
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.
it's not clear, but if there is a model-config, user has some config for a provider that we don't support built-in, so it's a custom provider with a model that matches the requested model
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.
with this change, we only check if the provider matches the requested model provider, but not the model name, but since we have dynamic it may be the only way, maybe we should:
- if dynamic-model is avaialble for that api, keep this check like that
- if not do the current check
WDYT?
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.
Yes, I think it is a better approach.
a03e175 to
0042ab6
Compare
Add fetchModels option to enable automatic model discovery from OpenAI-compatible providers. - fetchModels: true enables dynamic model discovery from /models endpoint - Static model configs override discovered models for customization - Graceful error handling with user-visible warnings The feature is opt-in and only works with providers exposing OpenAI-compatible /models endpoints.
0042ab6 to
7c382ab
Compare
@zikajk yes makes sense, but we already have the |
ericdallo
left a comment
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.
Looks good!
Add fetchModels option to enable automatic model discovery from OpenAI-compatible providers.
The feature is opt-in and only works with providers exposing OpenAI-compatible /models endpoints.