fix: use llama.cpp context metadata for OpenAI-compatible models - #9531
fix: use llama.cpp context metadata for OpenAI-compatible models#9531he-yufeng wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a51e11728
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| fn context_limit_from_model_entry(model: &Value) -> Option<usize> { | ||
| let value = model.get("meta").and_then(|meta| meta.get("n_ctx"))?; |
There was a problem hiding this comment.
Read llama.cpp's documented context field
For llama.cpp /v1/models, the documented meta object exposes the context size as n_ctx_train and does not include n_ctx. In that common response shape this helper returns None, so model_info_from_models_api_entry falls back to DEFAULT_CONTEXT_LIMIT instead of the advertised model limit; for local models with smaller windows, Goose can over-budget prompts and hit context errors despite the new metadata support.
Useful? React with 👍 / 👎.
|
Definitely have this problem, it does not read the context limit that is present in the API. I checked the code over, this is a legit fix. Recommend approval. |
7a51e11 to
a7b4152
Compare
|
Rebased this branch onto current Validation: The test run emits existing warnings outside this PR's touched provider file ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7b4152269
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn model_info_from_models_api_entry(provider_name: &str, model: &Value) -> Option<ModelInfo> { | ||
| let name = model.get("id").and_then(|v| v.as_str())?; | ||
| let config = ModelConfig::new_or_fail(name) | ||
| .with_canonical_limits(provider_name) | ||
| .with_context_limit(context_limit_from_model_entry(model)); |
There was a problem hiding this comment.
Apply metadata parsing to declarative OpenAI providers
For the local OpenAI-compatible providers this is meant to fix (for example the lmstudio and llama_swap declarative configs), this parser is never reached: non-HuggingFace engine: "openai" providers are registered with OpenAiProvider::from_custom_config in crates/goose/src/config/declarative_providers.rs:605-614, and OpenAiProvider::fetch_models_from_api still parses /models into Vec<String> only at crates/goose/src/providers/openai.rs:606-609. In those local llama.cpp-compatible environments, /v1/models metadata is still discarded and Goose continues to use the default/canonical context limit instead of the server-advertised limit.
Useful? React with 👍 / 👎.
a7b4152 to
0b932b9
Compare
|
Rebased this onto current Validated locally:
CI is running again on the rebased head. |
|
Thanks so much for taking the time to put this together, @he-yufeng! 🙏 It turns out this overlaps heavily with #9530, which tackles the same underlying issue (#9529) — surfacing llama.cpp's context metadata from One thing worth flagging for future reference: this PR patches Going to close this as a duplicate of #9530, but please don't be discouraged — we'd love to have you contribute again, and jumping in on #9530's review thread is a great place to keep the momentum going. Thanks again! 🪿 |
Summary
meta.n_ctxfrom OpenAI-compatible/modelsentries and surface it asModelInfo.context_limitWhy
Local llama.cpp-compatible servers can expose their runtime context window in
/modelsasmeta.n_ctx. Goose currently only keeps the model IDs from that response, so custom OpenAI-compatible local providers fall back to the default 128k context limit. That makes compaction/token-limit UI decisions too optimistic for servers running with smaller contexts.To verify
cargo fmt --package goosecargo test -p goose models_api --libcargo test -p goose openai_compatible --lib