-
Notifications
You must be signed in to change notification settings - Fork 785
feat: add thinking parameter to OllamaChatCompleteConfig #1415
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: add thinking parameter to OllamaChatCompleteConfig #1415
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.
🧪 PR Review is completed: Review of Ollama thinking parameter addition - found type safety issue with Params interface
| think: { | ||
| param: 'thinking', | ||
| transform: (params: Params) => { | ||
| if (params.thinking?.type === 'disabled') { | ||
| return false; | ||
| } | ||
| return true; | ||
| }, | ||
| }, |
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.
🟡 Type Safety
Issue: The transform function references params.thinking but there's no guarantee that the Params interface includes a thinking property. This could lead to TypeScript compilation errors or runtime issues if the Params type doesn't match the expected structure.
Fix: Ensure the Params interface includes a thinking property with the correct type definition, or add proper type guards to handle cases where thinking might not exist.
Impact: Prevents TypeScript compilation errors and ensures type safety at compile time
| think: { | |
| param: 'thinking', | |
| transform: (params: Params) => { | |
| if (params.thinking?.type === 'disabled') { | |
| return false; | |
| } | |
| return true; | |
| }, | |
| }, | |
| think: { | |
| param: 'thinking', | |
| transform: (params: Params) => { | |
| if (params.thinking?.type === 'disabled') { | |
| return false; | |
| } | |
| return true; | |
| }, | |
| }, |
| tools: { | ||
| param: 'tools', | ||
| }, | ||
| think: { |
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 would mean
think -> gets mapped to -> thinking
you probably are trying to do the opposite
you should do
thinking: {
param: 'think',
transform: (params: Params) => {
if (params.thinking?.type === 'disabled') {
return false;
}
return true;
},
},
Description
This PR adds thinking parameter support for Ollama provider
Motivation
--
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues
Fixes And Closes #1414