-
Couldn't load subscription status.
- Fork 2.4k
fix: do not override Ollama model num_ctx configuration #7160
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
The native Ollama handler was explicitly setting num_ctx to modelInfo.contextWindow, which overrode any user-configured num_ctx value in their Ollama model configuration. This caused issues for users who had specifically configured their models with custom context sizes to fit within their available memory. This fix removes the num_ctx override, allowing Ollama to use the model's configured value as intended. Fixes #7159
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.
Reviewing my own code because apparently I trust no one, not even myself.
| stream: true, | ||
| options: { | ||
| num_ctx: modelInfo.contextWindow, | ||
| // Don't override num_ctx - let Ollama use the model's configured value |
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.
Good fix removing the num_ctx override. However, could we consider adding documentation about how the Ollama context configuration works with this handler? It might help future developers understand why we're not setting this value explicitly.
| messages: [{ role: "user", content: prompt }], | ||
| stream: false, | ||
| options: { | ||
| // Don't override num_ctx - let Ollama use the model's configured value |
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.
These comments are identical in both methods. Is this intentional? We could consider extracting this behavior note to a class-level comment or constant to avoid duplication and make it clear this is a deliberate pattern across all Ollama API calls.
|
|
||
| // Specifically check that num_ctx is not in the options | ||
| const callArgs = mockChat.mock.calls[0][0] | ||
| expect(callArgs.options).not.toHaveProperty("num_ctx") |
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.
Nice test coverage for verifying num_ctx isn't set! Could we also add an integration test that verifies the handler works correctly when Ollama models have different configured context sizes? This would help catch any future regressions.
|
This fix is incomplete. It removes the num_ctx override (good) but leaves a display issue: Roo Code shows the model's default context window, not the user's configured num_ctx value. The proper fix needs to parse the actual num_ctx from Ollama's parameters field to display the correct context size. See issue #7159 for details. |
|
Related to: #7797 |
Description
This PR fixes an issue where the native Ollama handler was overriding user-configured
num_ctxvalues in their Ollama model configurations.Problem
In version 3.25.16, when the Ollama integration was switched to use the native API instead of the OpenAI compatibility layer, the implementation started explicitly setting
num_ctxtomodelInfo.contextWindow. This overrode any customnum_ctxvalue that users had configured in their Ollama models.For users who had specifically configured their models with smaller context sizes to fit within their available memory (e.g., setting
num_ctxto 16384), this caused the models to use the full context window (e.g., 1048576), leading to out-of-memory errors.Solution
This fix removes the explicit
num_ctxsetting from the options passed to Ollama, allowing the service to use the model's configured value as intended. This restores the behavior from version 3.25.15 and earlier.Changes
num_ctx: modelInfo.contextWindowfrom the options in bothcreateMessageandcompletePromptmethodsnum_ctxis not being set in the options passed to OllamaTesting
num_ctxis not overriddenFixes #7159
Important
Fixes issue where
num_ctxwas overridden in Ollama models, restoring user-configured values and preventing out-of-memory errors.num_ctx: modelInfo.contextWindowfromcreateMessageandcompletePromptinnative-ollama.ts.num_ctxvalues.native-ollama.spec.tsto verifynum_ctxis not set in options forcreateMessageandcompletePrompt.This description was created by
for df38bfe. You can customize this summary. It will automatically update as commits are pushed.