-
Notifications
You must be signed in to change notification settings - Fork 501
feat(js/plugins/ollama): migrate ollama plugin to v2 plugin API #3547
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(js/plugins/ollama): migrate ollama plugin to v2 plugin API #3547
Conversation
} | ||
return actions; | ||
}, | ||
async resolve(actionType, actionName) { |
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.
We should factor these out into helpers (like in original)
params?.requestHeaders | ||
); | ||
} | ||
return undefined; |
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.
We probably should handle the case where someone tries to resolve an embedder?
const serverAddress = config?.serverAddress || options.serverAddress; | ||
async (request, config) => { | ||
const serverAddress = | ||
options.serverAddress || DEFAULT_OLLAMA_SERVER_ADDRESS; |
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.
need to double check this, make sure we're not breaking anything by removing config.serverAddress
js/plugins/ollama/package.json
Outdated
"build": "npm-run-all build:clean check compile", | ||
"build:watch": "tsup-node --watch", | ||
"test": "find tests -name '*_test.ts' ! -name '*_live_test.ts' -exec node --import tsx --test {} +", | ||
"test": "find tests -name 'model_test.ts' ! -name '*_live_test.ts' -exec node --import tsx --test {} +", |
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.
Checking if we meant to switch off the glob testing for this package?
} | ||
return actions; | ||
}, | ||
async resolve(actionType, actionName) { |
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 there a reason this is called actionName and not name?
function ollamaPlugin(params?: OllamaPluginParams): GenkitPluginV2 { | ||
if (!params) { | ||
params = {}; | ||
} |
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.
function ollamaPlugin(params?: OllamaPluginParams): GenkitPluginV2 { | |
if (!params) { | |
params = {}; | |
} | |
function ollamaPlugin(params?: OllamaPluginParams = {}): GenkitPluginV2 { |
if (!params.serverAddress) { | ||
params.serverAddress = DEFAULT_OLLAMA_SERVER_ADDRESS; | ||
} | ||
const serverAddress = params.serverAddress; |
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.
if (!params.serverAddress) { | |
params.serverAddress = DEFAULT_OLLAMA_SERVER_ADDRESS; | |
} | |
const serverAddress = params.serverAddress; | |
const serverAddress = params.serverAddress || DEFAULT_OLLAMA_SERVER_ADDRESS; |
const actions: ResolvableAction[] = []; | ||
|
||
const DEFAULT_OLLAMA_SERVER_ADDRESS = 'http://localhost:11434'; | ||
if (params?.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.
Could this be extracted into a helper function?
function getModelActions(params: OllamaPluginParams, serverAddress: string): ResolvableAction[] {
/** Extract variables **/
const { models, requestHeaders } = params;
/** If no models, return empty array; **/
if (!models || !models.length) return [];
/** Return Ollama models **/
return models.map(m => createOllamaModel(m, serverAddress, requestHeaders));
}
init() {
return [
...getModelActions(params, serverAddress),
...getEmbedderActions(params, serverAddress)
];
}
if (actionType === 'model') { | ||
return await createOllamaModel( | ||
{ | ||
name: actionName, | ||
}, | ||
serverAddress, | ||
params?.requestHeaders | ||
); | ||
} | ||
return undefined; | ||
}, |
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 there a reason why we removed the resolveAction function? The original resolveActionWe can return early if undefined. Also should we return undefined here.
if (actionType === 'model') { | |
return await createOllamaModel( | |
{ | |
name: actionName, | |
}, | |
serverAddress, | |
params?.requestHeaders | |
); | |
} | |
return undefined; | |
}, | |
async resolve(actionType, actionName) { | |
/** If no model actions, return undefined **/ | |
if (actionType !== 'model') return undefined; | |
return await createOllamaModel( | |
{ | |
name: actionName, | |
}, | |
serverAddress, | |
params?.requestHeaders | |
); | |
} |
Fixes #3456
Plugin migration guide
Changes of note:
serverAddress
from config, because it doesn't exist on that interface any more in v2 API.Checklist (if applicable):
PR title is following https://www.conventionalcommits.org/en/v1.0.0/
Tested (integration, unit)
Docs updated (updated docs or a docs bug required)
Updated and added integration tests with the core genkit framework, for streaming responses and tool calls.
All existing plugin tests pass
In the process of manually testing the plugin.