-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: add CometAPI as new model provider #7708
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
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.
Thank you for your contribution! I've reviewed the CometAPI provider integration and found several issues that need attention before merging.
|
||
// this should be changed to accurately filter chat models based on server-side fields, and remove the local regex-based ignore list and loose parsing logic. | ||
|
||
// TODO(CometAPI): After the official model list interface is upgraded (returning richer type/capability fields or stable OpenAI compatible 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 regex-based filtering approach is fragile as noted in your TODO. Could we prioritize implementing capability-based filtering? This would be more maintainable and accurate than maintaining a regex pattern list.
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 regex-based filtering approach is fragile as noted in your TODO. Could we prioritize implementing capability-based filtering? This would be more maintainable and accurate than maintaining a regex pattern list.
This will be improved after we complete the upgrade of the model list interface.
} | ||
|
||
const url = `${baseUrl.replace(/\/$/, "")}/models` | ||
console.log(`CometAPI: Fetching models from ${url}`) |
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.
These console.log statements should be removed or wrapped in a debug condition before production. Consider using a debug flag or removing them entirely.
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.
These console.log statements should be removed or wrapped in a debug condition before production. Consider using a debug flag or removing them entirely.
You are correct, I have already removed these statements.
|
||
export const cometApiDefaultModelInfo: ModelInfo = { | ||
maxTokens: undefined, // Let system determine based on contextWindow | ||
contextWindow: 200000, // Reasonable default for modern models |
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 a 200k context window a reasonable default for all models? This seems quite high and might not be accurate for many models. Consider using a more conservative default like 8192 or 16384.
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 a 200k context window a reasonable default for all models? This seems quite high and might not be accurate for many models. Consider using a more conservative default like 8192 or 16384.
Here I choose the same default parameters as LiteLLM, which will be improved after the model list interface is upgraded
if (axios.isAxiosError(error)) { | ||
console.error(`CometAPI: API request failed: ${error.response?.status} ${error.response?.statusText}`) | ||
console.error(`CometAPI: Response data:`, error.response?.data) | ||
throw new 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.
Could we make the error messages more specific? For example, distinguishing between authentication failures (401/403) vs network issues (timeout) vs server errors (500+) would help users troubleshoot.
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 make the error messages more specific? For example, distinguishing between authentication failures (401/403) vs network issues (timeout) vs server errors (500+) would help users troubleshoot.
Good suggestion, I have added better error code feedback.
Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com>
Closing, please see #7688 (comment) |
Related GitHub Issue
Open: #7688
Description
This PR integrates and hardens the CometAPI provider and improves the CometAPI Settings UX:
API fetcher
{ success, data }
shape with lenient Zod parsing.COMETAPI_IGNORE_PATTERNS
and a compiled regex to filter out non-chat and utility models (image/audio/video/embeddings/moderation, etc.).Router models plumbing
cometapi
entry inrouterModels
when an API key is present and the fetch succeeds (no default emptycometapi: {}
).singleRouterModelFetchResponse
.Provider runtime
Webview Settings (CometAPI)
ModelPicker
API.Types/Defaults
Test Procedure
Unit tests
routerModels
tests should pass withcometapi
only present when a CometAPI API key is configured.singleRouterModelFetchResponse
per provider.Manual verification
routerModels
now includescometapi
populated from live API results (plus fallback merge).singleRouterModelFetchResponse
.cometapi
is not present when no key is provided; present when key exists and fetch succeeds.Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Discord: https://discord.gg/HMpuV6FCrG @james
Important
Integrates CometAPI as a new model provider, including API handling, model fetching, UI components, and validation support.
CometAPIHandler
insrc/api/providers/cometapi.ts
to handle CometAPI requests, supporting both OpenAI-like and CometAPI-specific response formats.buildApiHandler()
insrc/api/index.ts
.getCometApiModels()
insrc/api/providers/fetchers/cometapi.ts
, filtering out non-chat models.cometApiModels
and default model info inpackages/types/src/providers/cometapi.ts
.providerSettingsSchema
inpackages/types/src/provider-settings.ts
to include CometAPI settings.CometAPI
component inwebview-ui/src/components/settings/providers/CometAPI.tsx
for settings UI.ApiOptions.tsx
andModelPicker.tsx
to support CometAPI model selection.PROVIDERS
andMODELS_BY_PROVIDER
inconstants.ts
.validate.ts
to include CometAPI in API key and model validation.ProfileValidator
insrc/shared/ProfileValidator.ts
.SECRET_STATE_KEYS
inglobal-settings.ts
for secret management.webviewMessageHandler.ts
to handle CometAPI model fetching and error reporting.This description was created by
for e676bf7. You can customize this summary. It will automatically update as commits are pushed.