-
Notifications
You must be signed in to change notification settings - Fork 135
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
Added defaultModel to config that allows setting a global default model and config #98
Conversation
js/ai/src/generate.ts
Outdated
export function setGlobalDefaultModel< | ||
CustomOptions extends z.ZodTypeAny = z.ZodTypeAny, | ||
>(model: ModelArgument<CustomOptions>, config?: z.infer<CustomOptions>) { | ||
global[DEFAULT_MODEL_GLOBAL_KEY] = { | ||
model, | ||
config, | ||
} as GlobalModelRef; | ||
} |
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'm curious - why not make this part of configureGenkit
? Is it because models aren't a concept in core?
I ask because it could be kinda cool if genkit init
set default models (e.g. Gemini Pro for Vertex/Google AI)
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.
Yeah. I can't strongly type it in the config (circular dep with AI package). We can still make genkit init set it this way.
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.
Can take a string model ref in the config....
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 think I'd probably prefer string model ref in config. Honestly I think we should consider merging ai
and core
packages in the future.
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.
Sure. Do you want config as well?
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.
How about:
interface {
defaultModel?: {
name: string | {name: string};
config: Record<string,any>;
}
}
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.
Sg
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.
Done.
js/ai/src/generate.ts
Outdated
if (genkitConfig?.options.defaultModel) { | ||
model = | ||
typeof genkitConfig?.options.defaultModel.name === 'string' | ||
? genkitConfig?.options.defaultModel.name | ||
: genkitConfig?.options.defaultModel.name.name; | ||
if ( | ||
(!options.config || Object.keys(options.config).length === 0) && | ||
globalModel.config | ||
genkitConfig?.options.defaultModel.config | ||
) { | ||
// use configured global config | ||
options.config = globalModel.config; | ||
options.config = genkitConfig?.options.defaultModel.config; |
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.
Just double-checking - this gets overridden if specified by the user, right?
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.
yes, the condition above that this is only applied if options.config
is not provided (or is {}
).
can then call
generate
(including dotprompt) without having to specify a model.