-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: respect user-configured num_ctx in Ollama modelfiles #7342
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,16 +39,30 @@ type OllamaModelInfoResponse = z.infer<typeof OllamaModelInfoResponseSchema> | |||||||||||||||||
|
|
||||||||||||||||||
| export const parseOllamaModel = (rawModel: OllamaModelInfoResponse): ModelInfo => { | ||||||||||||||||||
| const contextKey = Object.keys(rawModel.model_info).find((k) => k.includes("context_length")) | ||||||||||||||||||
| const contextWindow = | ||||||||||||||||||
| const defaultContextWindow = | ||||||||||||||||||
| contextKey && typeof rawModel.model_info[contextKey] === "number" ? rawModel.model_info[contextKey] : undefined | ||||||||||||||||||
|
|
||||||||||||||||||
| // Parse the parameters field to check for user-configured num_ctx | ||||||||||||||||||
| let configuredNumCtx: number | undefined | ||||||||||||||||||
| if (rawModel.parameters) { | ||||||||||||||||||
| // The parameters field contains modelfile parameters as a string | ||||||||||||||||||
| // Look for num_ctx setting in the format "num_ctx <value>" | ||||||||||||||||||
| const numCtxMatch = rawModel.parameters.match(/num_ctx\s+(\d+)/i) | ||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this regex pattern comprehensive enough? The current pattern
Might want to consider a more flexible pattern or at least document the expected format. |
||||||||||||||||||
| if (numCtxMatch && numCtxMatch[1]) { | ||||||||||||||||||
| configuredNumCtx = parseInt(numCtxMatch[1], 10) | ||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we validate the parsed
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Use the configured num_ctx if available, otherwise fall back to the default | ||||||||||||||||||
| const actualContextWindow = configuredNumCtx || defaultContextWindow || ollamaDefaultModelInfo.contextWindow | ||||||||||||||||||
|
|
||||||||||||||||||
| const modelInfo: ModelInfo = Object.assign({}, ollamaDefaultModelInfo, { | ||||||||||||||||||
| description: `Family: ${rawModel.details.family}, Context: ${contextWindow}, Size: ${rawModel.details.parameter_size}`, | ||||||||||||||||||
| contextWindow: contextWindow || ollamaDefaultModelInfo.contextWindow, | ||||||||||||||||||
| description: `Family: ${rawModel.details.family}, Context: ${actualContextWindow}, Size: ${rawModel.details.parameter_size}`, | ||||||||||||||||||
| contextWindow: actualContextWindow, | ||||||||||||||||||
| supportsPromptCache: true, | ||||||||||||||||||
| supportsImages: rawModel.capabilities?.includes("vision"), | ||||||||||||||||||
| supportsComputerUse: false, | ||||||||||||||||||
| maxTokens: contextWindow || ollamaDefaultModelInfo.contextWindow, | ||||||||||||||||||
| maxTokens: actualContextWindow, | ||||||||||||||||||
| }) | ||||||||||||||||||
|
|
||||||||||||||||||
| return modelInfo | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,7 +181,7 @@ export class NativeOllamaHandler extends BaseProvider implements SingleCompletio | |
| messages: ollamaMessages, | ||
| stream: true, | ||
| options: { | ||
| num_ctx: modelInfo.contextWindow, | ||
| // Don't override num_ctx - let Ollama use the model's configured value | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a more detailed comment explaining why we're not setting |
||
| temperature: this.options.modelTemperature ?? (useR1Format ? DEEP_SEEK_DEFAULT_TEMPERATURE : 0), | ||
| }, | ||
| }) | ||
|
|
||
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! Though it might be worth adding edge case tests for: