-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Add IBM watsonx AI provider support #8091
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 watsonx to provider names and schemas - Create watsonx model definitions with IBM Granite models - Implement WatsonxHandler for API integration - Update UI components to support watsonx provider - Add watsonx to MODELS_BY_PROVIDER configuration Addresses #8087
- Remove unused watsonxModelInfoSaneDefaults import - Document getWatsonxModels function explaining static model list - Simplify function signature as parameters were unused
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 reviewed my own code and found issues. This is fine. Everything is fine.
| }, | ||
| } as const satisfies Record<string, ModelInfo> | ||
|
|
||
| export const watsonxModelInfoSaneDefaults: ModelInfo = { |
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 export is defined but never used anywhere. The PR description mentions it was removed, but it's still here. Should we actually remove it?
| // Default to US South region if not specified | ||
| const region = this.options.watsonxRegion || "us-south" | ||
| const baseURL = this.options.watsonxBaseUrl || `https://${region}.ml.cloud.ibm.com/ml/v1` | ||
| const apiKey = this.options.watsonxApiKey || "not-provided" |
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 intentional? Other providers that extend BaseOpenAiCompatibleProvider throw an error when the API key is missing. Using 'not-provided' as a fallback might lead to confusing error messages later.
Consider either:
- Throwing an error like other providers
- Extending BaseOpenAiCompatibleProvider which handles this validation
| import { getApiRequestTimeout } from "./utils/timeout-config" | ||
| import { handleOpenAIError } from "./utils/openai-error-handler" | ||
|
|
||
| export class WatsonxHandler extends BaseProvider implements SingleCompletionHandler { |
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.
Could we consider extending instead of ? Looking at FeatherlessHandler, it would reduce code duplication significantly. The base class already handles:
- OpenAI client initialization
- Stream creation with error handling
- Default createMessage implementation
- completePrompt implementation
You'd mainly need to override the constructor to pass watsonx-specific configuration.
| } | ||
|
|
||
| let response | ||
| try { |
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 nested try-catch blocks here seem redundant. The outer try-catch doesn't add value since the inner one already handles the OpenAI error. Could we simplify this?
|
|
||
| // Construct the base URL for watsonx API | ||
| // Default to US South region if not specified | ||
| const region = this.options.watsonxRegion || "us-south" |
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.
Would it be helpful to validate the region against known IBM Cloud regions? Currently any string is accepted, which might lead to runtime errors with invalid regions.
| "unbound", | ||
| "deepinfra", | ||
| "vercel-ai-gateway", | ||
| "watsonx", |
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.
While watsonx is correctly added to dynamicProviders, it's worth noting (maybe in a comment) that unlike other dynamic providers, watsonx doesn't actually fetch models dynamically - it uses a static list. This is already documented in the getWatsonxModels function, but a note here might help future maintainers understand the distinction.
Description
This PR attempts to address Issue #8087 by adding support for IBM watsonx AI provider, enabling users to connect to IBM-hosted LLMs.
Changes
Core Implementation
packages/types/src/provider-settings.tsWatsonxHandlerclass extendingBaseProviderfor API integrationbuildApiHandlerinsrc/api/index.tsTechnical Details
Review Feedback Addressed
watsonxModelInfoSaneDefaults)getWatsonxModelsfunction explaining the static model list approachTesting
Notes
Related Issue
Closes #8087
Feedback and guidance are welcome!
Important
Add support for IBM watsonx AI provider, including model definitions, API integration, and UI updates.
watsonxto provider names and schemas inprovider-settings.ts.watsonx.tswith model definitions for IBM Granite and third-party models.WatsonxHandlerinwatsonx.tsfor API integration.watsonxcase tobuildApiHandlerinindex.ts.useSelectedModel.tsto supportwatsonxprovider.watsonxModelInfoSaneDefaults.getWatsonxModelsfunction.This description was created by
for b0ff385. You can customize this summary. It will automatically update as commits are pushed.