Return current model as-is when no preset found #64
Conversation
Previously, if a model wasn't found in the presets list, the session would fail to load. This prevented using custom model providers with arbitrary model names. Now falls back to the configured model string, allowing custom providers to work without being in the preset list.
| ModelId(format!("{id}/{effort}").into()) | ||
| } | ||
|
|
||
| fn parse_model_id(id: ModelId) -> Result<(String, ReasoningEffort), Error> { |
There was a problem hiding this comment.
I think we need to update this as well.
Either here or where we call it, if this fails, we should return the model string as is and return an Option<ReasoningEffort> of None so that we can update the config accordingly
There was a problem hiding this comment.
Yes, I've added graceful parsing and a fallback to the user's config.
src/conversation.rs
Outdated
| })?; | ||
| Ok((model.to_owned(), reasoning)) | ||
|
|
||
| let reasoning = serde_json::from_value(reasoning.into()).ok(); |
There was a problem hiding this comment.
I wonder if here, we should also return the original model name?
Like in case the model name had a / in it, we don't want to return the first part, but rather treat this as a failure as well.
I also wonder if model name should always return a string, not an option, and then reasoning only returns an option if we can parse.
OR we treat this as a parse, and this instead an Option<(String, ReasoningEffor)>, so we only return values at all if it is one we expect
There was a problem hiding this comment.
I also think we don't need a Result anymore
…ed clarity and error handling
|
Could you please help review and release this PR when you have a moment? It would be really helpful for my current work. Thanks a lot! @benbrandt |
3 participants
Fixes session load failure when using models not in the preset list (e.g., custom model providers zed-industries/zed#40292).
@benbrandt plz review