fix: use dynamic model listing for all providers#7065
fix: use dynamic model listing for all providers#7065rabi wants to merge 1 commit intoblock:mainfrom
Conversation
Remove the early return in get_provider_models that returned static model names for declarative/custom providers. Now all providers go through fetch_recommended_models() first, with static known_models from metadata as fallback when fetching fails or returns empty. Respect allows_unlisted_models for free-text model input. Align CLI configure dialog with the same fallback behavior. Signed-off-by: rabi <ramishra@redhat.com>
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
so this seems like an improvement, but it looks like we're returning the static list when the dynamic list throws an error. should we not propogate that error and only return the static list if the remote return an empty list, i.e. is not implemented? we now use the fetch models in the desktop as a way to establish whether we actually have a working provider. before we had something similar as what I think is going on here and if you had your ollama misconfigured, it would show you 5 random models rather than telling you the provider didn't work.
| Path(name): Path<String>, | ||
| ) -> Result<Json<Vec<String>>, ErrorResponse> { | ||
| let loaded_provider = goose::config::declarative_providers::load_provider(name.as_str()).ok(); | ||
| // TODO(Douwe): support a get models url for custom providers |
Thanks @DOsinga for looking at it. I had tried to keep it as UX friendly as possible.
|
Declarative providers may not have a model listing API, we make the API call, but the actual inference endpoint might not implement /models. Built-in providers like Vertex AI handle this by overriding fetch_supported_models with a known model list, but declarative providers can't do that as they're defined by JSON and use the underlying provider's fetch_supported_models which makes a real API call, Having said that, if we want to go that route we can, though the curated model list we maintain would be redundant for the purpose |
|
@rabi My read looking at the logic is that we are suggesting models out of a list of potentially hundreds due to fewer working with tools or tested with goose. this leaves:
That's the idea in simplifying this problem. The more 3 level if statements we have, the harder it is to maintain and also the more unlike this codebase becomes vs model abstractions. I think as long as we make docs more coherent, we can de-tempt folks from being "too smart" with errors and still have a recommended list. My 2p |
|
obviated by #7074 |
As expected #7074 https://github.com/block/goose/blob/main/crates/goose/src/providers/claude_code.rs#L497, broke the current workflow with cli configure, the only way you can workaround it is by forcing the model with env vars.. |
3 participants
Summary
Remove the early return in get_provider_models that returned static model names for declarative/custom providers. Now all providers go through fetch_recommended_models() first, with static known_models from metadata as fallback when fetching fails or returns empty. Respect allows_unlisted_models for free-text model input. Aligns both cli and ui behavior.
Type of Change
AI Assistance
Testing
Unit tests and tested locally.
Related Issues
Relates to #7022