-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: use native Ollama API endpoints instead of OpenAI-compatible routes #7071
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
base: main
Are you sure you want to change the base?
Conversation
- Replace OpenAI client with direct axios calls to Ollama native API - Use /api/chat endpoint instead of /v1 OpenAI-compatible endpoint - Update tests to reflect the new native API implementation - Maintain backward compatibility with existing configurations Fixes #7070
}) as const, | ||
) | ||
|
||
let buffer = "" |
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.
Unused variable 'buffer' declared here isn't used; please remove for clarity.
let buffer = "" |
for (const matcherChunk of matcher.update(delta.content)) { | ||
yield matcherChunk | ||
|
||
// Convert Anthropic messages to Ollama format |
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.
The inline conversion of Anthropic messages to Ollama format (lines 82–117) could be extracted into a utility function for improved readability and maintainability.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
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 grading my own homework - I already know where I cut corners.
}) as const, | ||
) | ||
|
||
let buffer = "" |
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.
Is this buffer
variable intentional? It's declared but never used. Could we remove it to keep the code clean?
const ollamaMessages: OllamaMessage[] = [{ role: "system", content: systemPrompt }] | ||
|
||
// Convert messages to Ollama format | ||
for (const message of messages) { |
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.
I notice we check for deepseek-r1
models and adjust temperature, but unlike the previous OpenAI implementation, we don't apply R1 format conversion to the messages. Is this intentional? DeepSeek R1 models might need special message formatting for optimal performance.
} else { | ||
throw new Error(`Ollama completion error: ${error.message}`) | ||
} | ||
} | ||
if (error instanceof Error) { |
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.
This error handling seems redundant - axios errors are already handled above. Could we remove this duplicate handler to simplify the error flow?
model: string | ||
created_at: string | ||
message: { | ||
role: string |
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.
Consider using the more specific type "system" | "user" | "assistant"
instead of string
for better type safety?
apiKey: "ollama", | ||
timeout: getApiRequestTimeout(), | ||
}) | ||
this.baseUrl = this.options.ollamaBaseUrl || "http://localhost:11434" |
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.
The default Ollama URL appears in multiple places. Would it be cleaner to extract this to a constant like const DEFAULT_OLLAMA_BASE_URL = "http://localhost:11434"
?
This PR fixes issue #7070 where Ollama models were incorrectly using OpenAI-compatible routes instead of native Ollama API endpoints.
Problem
When using models like
gpt-oss:120b
with Ollama, the plugin was trying to get completions from OpenAI routes (/v1
) instead of the native Ollama/api/chat
endpoint.Solution
/api/chat
endpoint for chat completions instead of/v1
OpenAI-compatible endpointChanges Made
Modified
src/api/providers/ollama.ts
:/api/chat
endpoint using axiosUpdated tests:
src/api/providers/__tests__/ollama.spec.ts
: Updated to mock axios instead of OpenAI clientsrc/api/providers/__tests__/ollama-timeout.spec.ts
: Updated timeout tests to work with new implementationTesting
Fixes #7070
Important
This PR updates
OllamaHandler
to use native Ollama API endpoints with axios, replacing OpenAI-compatible routes, and updates tests to reflect these changes.OllamaHandler
./api/chat
endpoint for chat completions.ollama.ts
.ollama.spec.ts
andollama-timeout.spec.ts
to mock axios and test new implementation.This description was created by
for 14c33f8. You can customize this summary. It will automatically update as commits are pushed.