-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support prompt variants #14487
Support prompt variants #14487
Conversation
Thanks, this enables a nice use case! I'd be for option (b) or (c) -- tendency towards (b). I think the agent shouldn't really be aware of that at all when obtaining the prompt. I tend to prefer keeping it as an agent setting too: The confusing thing is, that anyone can request this prompt template, and they would obtain the "agent-scoped" variant of the prompt when resolving it too. This is because prompt templates are per se indepenent of agents, but we only use it in an agent-specific way so far. However, I guess this is a rather theoretical problem, I can't see a lot of valid use cases where another agent than the agent who contributed the prompt template would consume it? |
f3a3bf8
to
e1b9102
Compare
fixed #14485 Signed-off-by: Jonas Helming <[email protected]>
e1b9102
to
e770ba6
Compare
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 like the new feature, I left some small remarks
template={template} | ||
promptCustomizationService={this.promptCustomizationService} />)} | ||
{agent.promptTemplates | ||
?.filter(template => !template.variantOf) |
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.
what if the agent does not provide a template without a variant? shouldn't we handle this? eg show 'no default template available'?
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.
Great suggestions, see above
packages/ai-core/src/browser/ai-configuration/template-settings-renderer.tsx
Outdated
Show resolved
Hide resolved
@@ -99,7 +99,7 @@ export class AgentServiceImpl implements AgentService { | |||
registerAgent(agent: Agent): void { | |||
this._agents.push(agent); | |||
agent.promptTemplates.forEach( | |||
template => this.promptService.storePrompt(template.id, template.template) | |||
template => this.promptService.storePromptTemplate(template) |
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.
do we need the old method?
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 break the API, I don't know whether people are already using it. For conveniance, I would probably leave it.
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.
mark it as deprecated? with a hint to use the new method instead?
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.
Hmm, it is just a simpler version for all manditory values, I would keep it.
@@ -68,6 +74,11 @@ export interface PromptService { | |||
* @param prompt the prompt template to store | |||
*/ | |||
storePrompt(id: string, prompt: string): void; |
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.
as above, do we need this anymore?
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 break the API, I don't know whether people are already using it. For conveniance, I would probably leave it.
const selectedVariants = agentSettings?.selectedVariants || {}; | ||
|
||
const updatedVariants = { ...selectedVariants }; | ||
if (newVariant === DEFAULT_VARIANT) { |
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.
why do we need to handle that case special?
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 dont understand the question. If the default template is selected for a template ID, we remove the entry from the selected variants?
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 see that, why not just store that the user selected the default variant?
I'm fine also with the current solution was just wondering, whether there is a special reason to do so
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.
Hmm, I find it cleaner to only store if variants are actually selected.
Tested it, works great. |
Signed-off-by: Jonas Helming <[email protected]>
Adressed all comments (except the last question) |
Signed-off-by: Jonas Helming <[email protected]>
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. Works for me.
As follow-ups:
- allow to add variants
- if no default template is provided, the user should still be able to select/use a variant instead.
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.
LGTM! Thanks!
As discussed, preferably a single method for storing templates would simplify the interface.
Signed-off-by: Jonas Helming <[email protected]>
fixed #14485
What it does
Introduce the capability for prompt variants, i.e. variants of a main prompt to e.g. support specific LLMs.
There is always a main prompt (default) that can have multiple variants.
The currently selected variant is a setting.
How to test
NOTE: There is an unrelated bug in the prompt watching. I can sometimes get into a state, where I change a prompt template, but the prompt customization service is not notified anymore. This usually happens during demos, but I have no reproducible path yet.
Review checklist
Reminder for reviewers