-
Notifications
You must be signed in to change notification settings - Fork 759
feat: add krutrim as provider #1201
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.
This PR adds Krutrim as a new provider to the gateway, following the established patterns for provider integration. The implementation is clean and well-structured. I have a few suggestions to improve error handling and type safety.
if (responseStatus !== 200 && 'html-message' in response) { | ||
// Handle Krutrim's error format | ||
return generateErrorResponse( | ||
{ | ||
message: response['html-message'] ?? '', | ||
type: 'error', | ||
param: null, | ||
code: String(responseStatus), | ||
}, | ||
KRUTRIM | ||
); | ||
} |
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.
🛠️ Code Refactor
Issue: The error handling only checks for 'html-message' property but doesn't handle standard error responses that might not have this property.
Fix: Add a more comprehensive error handling approach that can handle both Krutrim-specific and standard error formats.
Impact: Improves error handling robustness and prevents potential unhandled error scenarios.
if (responseStatus !== 200 && 'html-message' in response) { | |
// Handle Krutrim's error format | |
return generateErrorResponse( | |
{ | |
message: response['html-message'] ?? '', | |
type: 'error', | |
param: null, | |
code: String(responseStatus), | |
}, | |
KRUTRIM | |
); | |
} | |
if (responseStatus !== 200) { | |
// Handle Krutrim's error format | |
return generateErrorResponse( | |
{ | |
message: 'html-message' in response ? response['html-message'] ?? '' : 'Unknown error from Krutrim API', | |
type: 'error', | |
param: null, | |
code: String(responseStatus), | |
}, | |
KRUTRIM | |
); | |
} |
headers: ({ providerOptions, fn }) => { | ||
const headersObj: Record<string, string> = { | ||
Authorization: `Bearer ${providerOptions.apiKey}`, | ||
}; | ||
return headersObj; | ||
}, |
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.
⚡️ Performance Improvement
Issue: The headers function creates a new object on every call, which is unnecessary since the object structure is static.
Fix: Optimize the headers function to directly return the object without the intermediate variable.
Impact: Slightly improves performance by reducing unnecessary object creation and variable assignment.
headers: ({ providerOptions, fn }) => { | |
const headersObj: Record<string, string> = { | |
Authorization: `Bearer ${providerOptions.apiKey}`, | |
}; | |
return headersObj; | |
}, | |
headers: ({ providerOptions, fn }) => { | |
return { | |
Authorization: `Bearer ${providerOptions.apiKey}`, | |
}; | |
}, |
import { ChatCompletionResponse, ErrorResponse } from '../types'; | ||
|
||
import { generateErrorResponse } from '../utils'; | ||
|
||
import { KRUTRIM } from '../../globals'; | ||
|
||
interface KrutrimChatCompleteResponse extends ChatCompletionResponse {} | ||
interface KrutrimChatCompleteErrorResponse extends ErrorResponse { | ||
'html-message'?: 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.
🛠️ Code Refactor
Issue: The interface KrutrimChatCompleteResponse is defined but doesn't add any properties beyond the base ChatCompletionResponse.
Fix: Either add Krutrim-specific properties to the interface or use the base type directly to simplify the code.
Impact: Improves code clarity and maintainability by avoiding unnecessary type definitions.
import { ChatCompletionResponse, ErrorResponse } from '../types'; | |
import { generateErrorResponse } from '../utils'; | |
import { KRUTRIM } from '../../globals'; | |
interface KrutrimChatCompleteResponse extends ChatCompletionResponse {} | |
interface KrutrimChatCompleteErrorResponse extends ErrorResponse { | |
'html-message'?: string; | |
} | |
import { ChatCompletionResponse, ErrorResponse } from '../types'; | |
import { generateErrorResponse } from '../utils'; | |
import { KRUTRIM } from '../../globals'; | |
interface KrutrimChatCompleteErrorResponse extends ErrorResponse { | |
'html-message'?: string; | |
} |
@narengogi can you take a look at this PR? |
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.
formatting is a little off in /providers/krutrim/chatComplete
+ 1 minor comment, rest looks good!
src/providers/krutrim/index.ts
Outdated
api: KrutrimAPIConfig, | ||
chatComplete: chatCompleteParams([ | ||
'max_tokens', | ||
'temperature', |
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.
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.
Hi @narengogi,
I've pushed a commit to remove the excludes. I had mistakenly marked the params as required.
additionally, added a default model.
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Description
Type of Change