-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(ollama): add tools support filtering, configurable timeouts, and… #10947
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?
feat(ollama): add tools support filtering, configurable timeouts, and… #10947
Conversation
All issues resolved. Latest commit (87ccbe1) correctly fixes the connection test to use current UI values for baseUrl and apiKey instead of only using saved state.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| @@ -0,0 +1,119 @@ | |||
| import axios, { AxiosInstance, AxiosError, InternalAxiosRequestConfig, AxiosResponse } from "axios" | |||
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 file is unused and contains duplicate code. The exact same implementation exists in ollama.ts (lines 19-135):
createOllamaAxiosInstancefunctionsetupRetryInterceptorfunctionsetupLoggingInterceptorfunction
This file should be removed to avoid duplication and potential maintenance issues where changes to one file might not be reflected in the other.
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.
Review Summary
Overall, this is a solid PR with valuable functionality for Ollama model discovery. The code is well-tested with comprehensive test coverage (28 tests passing).
One issue needs to be addressed:
The file src/api/providers/fetchers/ollama-axios-config.ts is dead code. It contains the exact same implementation as lines 19-135 of ollama.ts (createOllamaAxiosInstance, setupRetryInterceptor, setupLoggingInterceptor) and is not imported anywhere in the codebase.
Please remove this duplicate file before merging.
Once this is addressed, the PR is ready to merge.
| <thead> | ||
| <tr className="border-b border-vscode-foreground/10"> | ||
| <th className="text-left py-2 px-3 font-medium text-vscode-foreground"> | ||
| Model Name |
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.
Can we update this table to use the internationalized strings?
| provider.postMessageToWebview({ | ||
| type: "ollamaModelsRefreshResult", | ||
| success: true, | ||
| message: `Found ${result.modelsWithTools.length} model(s) with tools support (${result.totalCount} total)`, |
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.
Can we internationalize these messages too?
| }`}> | ||
| <div>{testResult.message}</div> | ||
| {testResult.durationMs !== undefined && ( | ||
| <div className="text-xs mt-1 opacity-80">Completed in {testResult.durationMs}ms</div> |
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 one too
| </thead> | ||
| <tbody> | ||
| {modelsWithTools.map((model) => { | ||
| const formatSize = (bytes?: number): 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.
I think we probably already have a helper for this somewhere
|
Thanks for the feedback! On it! 🚀 Also fixed the Kdenlive render ( av1 apparently doesn't work, so did h265 ) |
|
I tried this, and it works - however it didn't seem to honor a non-default ollama port, if configured. Also noticed that the first time through (perhaps in the first-time-user flow, after being asked if I want to create a roo account or use a 3rd party provider, and I choose 3rd party provider), the ollama settings view didn't seem to scroll far enough, so the content was clipped partway through the list of models with no tools support. As soon as I got to the regular API provider config UI (i.e. not Roo's first-run flow), all the ollama settings UI was accessible. |




feat(ollama): add tools support filtering, configurable timeouts, and improved UI
Features added

Connection settings

Works when squished

Test Base URL

Refresh models

Related GitHub Issue
Closes: #10795
#10795
Description
This PR addresses issue #10795 by adding a "Refresh Models" button and significantly enhancing the Ollama provider integration.
The video for this pull request is (updated) : https://youtu.be/IqmIjSklBe8
The changes include:
Key Implementation Details:
Enhanced Model Discovery (
discoverOllamaModelsWithSorting)/api/tagscall followed by parallel/api/showcalls for all modelscapabilities.includes("tools")model_infofields (fixesObject.keys(null)errors)Axios Configuration (
createOllamaAxiosInstance)ECONNABORTED,ETIMEDOUT)UI Redesign
Type System Extensions
OllamaModelWithToolsinterface for table display dataOllamaModelsDiscoveryResultinterface for discovery resultsProviderSettingswith new configuration options:ollamaRequestTimeout(default: 60 min, max: 120 min)ollamaModelDiscoveryTimeout(default: 10 sec, max: 10 min)ollamaMaxRetries(default: 0, max: 10)ollamaRetryDelay(default: 1 sec)ollamaEnableLogging(default: false)Design Choices:
Reviewer Attention:
parseOllamaModelanddiscoverOllamaModelsWithSorting- we use!= nullchecks to handle bothnullandundefinedcreateOllamaAxiosInstancemodelsWithToolsarray matches backendOllamaModelWithTools[]structureTest Procedure
Unit Tests:
All tests are located in
src/api/providers/fetchers/__tests__/ollama.test.ts. To run:Test Coverage (20 tests, all passing):
parseOllamaModel- Handles null/undefinedmodel_infogracefullygetOllamaModels- Fetches models with tools capability filteringgetOllamaModels- Filters out models without tools capabilitygetOllamaModels- Handles/api/tagscall failuresgetOllamaModels- Handles ECONNREFUSED errorsgetOllamaModels- Handles models with null familiesgetOllamaModels- Includes Authorization header when API key providedgetOllamaModels- Uses custom timeout configurationgetOllamaModels- Handles timeout errors during discoverygetOllamaModels- Handles ECONNABORTED timeout errorsdiscoverOllamaModelsWithSorting- Handles nullmodel_infogracefullydiscoverOllamaModelsWithSorting- Handles undefinedmodel_infogracefullydiscoverOllamaModelsWithSorting- Handles emptymodel_infoobject gracefullydiscoverOllamaModelsWithSorting- Sorts models correctly into tools and non-tools groupsdiscoverOllamaModelsWithSorting- Handles partial failures gracefully (some models succeed, others fail)discoverOllamaModelsWithSorting- Handles models with both tools and vision capabilitiesdiscoverOllamaModelsWithSorting- Handles timeout errors during/api/tagscalldiscoverOllamaModelsWithSorting- Handles timeout errors gracefully for individual/api/showcallsdiscoverOllamaModelsWithSorting- Uses custom timeout configurationdiscoverOllamaModelsWithSorting- VerifiestotalCountmatches sum of both groupsManual Testing Steps:
Model Discovery & Refresh:
ollama pull llama3.2)Connection Testing:
http://localhost:11434)Model Selection:
Configuration Settings:
Error Handling:
Testing Environment:
http://localhost:11434Pre-Submission Checklist
Screenshots / Videos
The video for this pull request is (updated): https://youtu.be/IqmIjSklBe8
Before:
After:
Documentation Updates
Documentation Updates Needed:
This change actually makes the current default configuration much more obvious, so this is partially self documenting.
Ollama Provider Documentation (https://docs.roocode.com/providers/ollama):
Settings Reference:
ollamaRequestTimeout(default: 3600000 ms)ollamaModelDiscoveryTimeout(default: 10000 ms)ollamaMaxRetries(default: 0)ollamaRetryDelay(default: 1000 ms)ollamaEnableLogging(default: false)Additional Notes
Implementation Notes:
Null Safety: The Ollama API can return
model_infoasnullorundefined. We use explicit!= nullchecks (which handles both) instead of truthy checks to avoidObject.keys(null)errors.Zod Schema Validation: The validation is intentionally lenient - if Zod validation fails but essential fields (
family,parameter_size) are present, we proceed with the raw data. This allows the code to work with minimal mock data in tests while still validating real API responses.Axios Timeout Configuration: Timeouts are configured at the Axios instance level, not per-request. This ensures consistent behavior across all API calls.
Model Discovery Performance: The parallel
/api/showcalls significantly improve discovery speed compared to sequential calls. With 14 models, this reduces discovery time from ~14 seconds to ~2-3 seconds.Backward Compatibility: All new configuration options have sensible defaults, so existing configurations continue to work without modification.
Test Coverage: The test suite includes comprehensive coverage for:
Questions for Reviewers:
Get in Touch
Discord:
randomizedcoder(or@randomizedcoder)Important
Enhances Ollama provider with model discovery, UI improvements, and configurable settings, including timeouts and retries, with comprehensive test coverage.
discoverOllamaModelsWithSortingfor model discovery with sorting into tools and non-tools groups.webviewMessageHandler.ts.Ollama.tsx.provider-settings.ts.Ollama.tsxto handle new configuration options.Ollama.tsx.Ollama.tsx.ollama.test.tsandollama-axios-config.spec.tsfor new functionalities.settings.jsonfor new UI text and descriptions.createOllamaAxiosInstancefor centralized Axios configuration inollama.ts.This description was created by
for 77a2cae. You can customize this summary. It will automatically update as commits are pushed.