-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: respect user-configured context window for Ollama models #7344
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
- Add ollamaContextWindow field to ProviderSettings schema - Update NativeOllamaHandler to use custom context window when provided - Add tests to verify context window configuration works correctly Fixes #7343
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 is like debugging in a mirror - everything looks backward but the bugs are still mine.
| const ollamaSchema = baseProviderSettingsSchema.extend({ | ||
| ollamaModelId: z.string().optional(), | ||
| ollamaBaseUrl: z.string().optional(), | ||
| ollamaContextWindow: z.number().optional(), |
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.
Should we add validation constraints here? Something like:
| ollamaContextWindow: z.number().optional(), | |
| ollamaContextWindow: z.number().positive().optional(), |
This would prevent invalid values like 0 or negative numbers from being set.
| stream: true, | ||
| options: { | ||
| num_ctx: modelInfo.contextWindow, | ||
| num_ctx: this.options.ollamaContextWindow || modelInfo.contextWindow, |
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 implementation using the fallback pattern! The optional chaining with || fallback to modelInfo.contextWindow is clean and follows the existing patterns in the codebase.
| messages: [{ role: "user", content: prompt }], | ||
| stream: false, | ||
| options: { | ||
| num_ctx: this.options.ollamaContextWindow || modelInfo.contextWindow, |
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 consistency - same pattern applied to the non-streaming method. Though I wonder if we should add more specific error handling here to distinguish between model fetch failures and context window configuration issues?
| expect(results.some((r) => r.type === "text")).toBe(true) | ||
| }) | ||
|
|
||
| describe("context window configuration", () => { |
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.
Excellent test coverage! These tests thoroughly verify both the custom context window usage and the fallback behavior. Consider adding an edge case test for invalid values (0 or negative) once we add schema validation.
|
This implementation is incomplete |
Description
This PR fixes an issue where Roo Code was ignoring custom context window settings for Ollama models. When users created models with specific context windows using Ollama modelfiles, Roo Code would override these settings with its own defaults.
Problem
As reported in #7343, when a user created an Ollama model with a 48k context window (e.g., glm45-48k), Roo Code would load it with 131k context instead of respecting the configured limit.
Solution
ollamaContextWindowfield to the ProviderSettings schemaChanges
ollamaContextWindowfield to Ollama schemacreateMessageandcompletePromptmethodsTesting
Fixes #7343
Important
Fixes issue with Ollama models by respecting user-configured context windows in
NativeOllamaHandler.NativeOllamaHandlernow respectsollamaContextWindowfromProviderSettingsfor context window configuration.ollamaContextWindowis not provided.ollamaContextWindowfield toollamaSchemainprovider-settings.ts.native-ollama.spec.tsto verify custom context window usage increateMessageandcompletePromptmethods.This description was created by
for 2f94833. You can customize this summary. It will automatically update as commits are pushed.