Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions src/api/providers/__tests__/native-ollama.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,69 @@ describe("NativeOllamaHandler", () => {
})
})

describe("num_ctx handling", () => {
it("should not override num_ctx in createMessage", async () => {
// Mock the chat response
mockChat.mockImplementation(async function* () {
yield {
message: { content: "Test response" },
eval_count: 1,
prompt_eval_count: 1,
}
})

const systemPrompt = "You are a helpful assistant"
const messages = [{ role: "user" as const, content: "Test" }]

const stream = handler.createMessage(systemPrompt, messages)

// Consume the stream
for await (const _ of stream) {
// Just consume
}

// Verify that num_ctx was NOT set in the options
expect(mockChat).toHaveBeenCalledWith(
expect.objectContaining({
model: "llama2",
messages: expect.any(Array),
stream: true,
options: expect.objectContaining({
temperature: 0,
}),
}),
)

// Specifically check that num_ctx is not in the options
const callArgs = mockChat.mock.calls[0][0]
expect(callArgs.options).not.toHaveProperty("num_ctx")
Copy link
Author

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.

})

it("should not override num_ctx in completePrompt", async () => {
mockChat.mockResolvedValue({
message: { content: "Test response" },
})

await handler.completePrompt("Test prompt")

// Verify that num_ctx was NOT set in the options
expect(mockChat).toHaveBeenCalledWith(
expect.objectContaining({
model: "llama2",
messages: expect.any(Array),
stream: false,
options: expect.objectContaining({
temperature: 0,
}),
}),
)

// Specifically check that num_ctx is not in the options
const callArgs = mockChat.mock.calls[0][0]
expect(callArgs.options).not.toHaveProperty("num_ctx")
})
})

describe("error handling", () => {
it("should handle connection refused errors", async () => {
const error = new Error("ECONNREFUSED") as any
Expand Down
3 changes: 2 additions & 1 deletion src/api/providers/native-ollama.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

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.

temperature: this.options.modelTemperature ?? (useR1Format ? DEEP_SEEK_DEFAULT_TEMPERATURE : 0),
},
})
Expand Down Expand Up @@ -270,6 +270,7 @@ export class NativeOllamaHandler extends BaseProvider implements SingleCompletio
messages: [{ role: "user", content: prompt }],
stream: false,
options: {
// Don't override num_ctx - let Ollama use the model's configured value
Copy link
Author

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.

temperature: this.options.modelTemperature ?? (useR1Format ? DEEP_SEEK_DEFAULT_TEMPERATURE : 0),
},
})
Expand Down
Loading