Ok, well, that got out of hand #3718
Conversation
| .get_param::<usize>("GOOSE_LEAD_FALLBACK_TURNS") | ||
| .unwrap_or(default_fallback_turns()); | ||
|
|
||
| // Create model configs with context limit environment variable support |
There was a problem hiding this comment.
unrelated to this change but feels bad we have a "factory.rs" as well as a "base.rs" - seems very OO/java like or non rust idiomatic but not sure what proper patterns are.
There was a problem hiding this comment.
there's definitely a better way to do this; lean more into macros is I think one way. ideally I think we'd do something like:
!enabled_providers(OpenAIProvider, AntrhopicProvider, ... )
and it would do the whole factory thing in one sweet go.
base.rs should probably be split into traits and possibly we should move all the actual provider implementations into a subfolder so there's good distinction between support code and implementation. and most providers should just be one macro call, saying, I'm openai compatibile, and this is where my end points are, I support streaming, but not embeddings or some such.
let's do it!
| InvalidRange(String, String), | ||
| } | ||
|
|
||
| static MODEL_SPECIFIC_LIMITS: Lazy<Vec<(&'static str, usize)>> = Lazy::new(|| { |
There was a problem hiding this comment.
are we likely to replace this one day with something provider driven? If not I wonder if we should put this in token_limits.rs so it is easy to find, but doesn't really matter either way.
(some providers can furnish the limits - like openrouter can, but not sure about the general case)
There was a problem hiding this comment.
yeah, I think this goes back to the whole thinking around models <-> providers. in some ideal world I would say we have a models folder which defines all the models and their properties and then providers indicate which of those models they support. llama3-50b-9f has the same properties independent of the provider. I think
|
oh yeah it did get large! nice yeah this makes sense I think and does seem to tidy up the providers and repeated code so LGTM |
|
hrm - getting different errors than locally - I hope this isn't another wildcard wildgoose chase change... |
|
odd, cargo check doesn't cover tests, which ... I don't know why it wouldn't (I guess in the past we had most tests macro'd inline) |
|
ok @DOsinga well it continued to get out of control! even bigger change now |
| /// 4. Global default (128_000) (in get_context_limit) | ||
| pub fn new(model_name: String) -> Self { | ||
| Self::new_with_context_env(model_name, None) | ||
| pub fn new(model_name: &str) -> Result<Self, ConfigError> { |
There was a problem hiding this comment.
@DOsinga this is the very wide reaching change, but it does seem nicely idiomatic rust: (eg https://doc.rust-lang.org/std/fs/struct.File.html#method.open canonical example). Lots of clients to this use ? which I assume is ok when we just want to let it fail?
There was a problem hiding this comment.
yes exactly
well, fail, we should almost never fail I think but bubble up the exception, which is what ? does. as it was, we were ignoring errors in the provider construction, which I think is strictly worse than throwing an exception even if we don't catch it. if you get the model wrong, it will typically result in a 404 at inference time, which means the user can't do antyhing.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
some local basic smoke tests with cli and nothing went boom.
| .unwrap_or(false); | ||
|
|
||
| let model_config = goose::model::ModelConfig::new(model.clone()) | ||
| let model_config = goose::model::ModelConfig::new(&model)? |
There was a problem hiding this comment.
we probably just want to skip this model if we can't load it
| .expect("Did not find a model on payload or in env to update provider with") | ||
| }); | ||
| let model_config = ModelConfig::new(model); | ||
| let model_config = ModelConfig::new(&model).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; |
There was a problem hiding this comment.
the line above where we have .expect() will kill goosed and it is all over. not sure how we would have gotten here, but still, we should probably do another 500; I also think we should change the description of the 500 from internal server error here to something descriptive; could not update provider, make sure that you have the right config or something
* main: Remove unused Memory Mode / Computer-Controller Mode code (#3743) docs: Add Neon MCP Server tutorial (#3639) Update OSX codesigning and notarization (#3658) Fix slow typing in chat input with long running sessions (#3722) Fix html content detection regex to not include markdown autolinks (#3720) Must have missed this one (#3733) Ok, well, that got out of hand (#3718) feat: openrouter out of the box experience for goose installations (#3507)
Ok, it looks like a lot is happening here, but ... ok, fine there is some stuff, but mostly: