-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,26 +14,103 @@ | |
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 | ||
// ***************************************************************************** | ||
import * as React from '@theia/core/shared/react'; | ||
import { PromptCustomizationService } from '../../common/prompt-service'; | ||
import { PromptTemplate } from '../../common'; | ||
import { PromptCustomizationService, PromptService } from '../../common/prompt-service'; | ||
import { AISettingsService, PromptTemplate } from '../../common'; | ||
|
||
export interface TemplateSettingProps { | ||
const DEFAULT_VARIANT = 'default'; | ||
|
||
export interface TemplateRendererProps { | ||
agentId: string; | ||
template: PromptTemplate; | ||
promptCustomizationService: PromptCustomizationService; | ||
promptService: PromptService; | ||
aiSettingsService: AISettingsService; | ||
} | ||
|
||
export const TemplateRenderer: React.FC<TemplateSettingProps> = ({ agentId, template, promptCustomizationService }) => { | ||
export const TemplateRenderer: React.FC<TemplateRendererProps> = ({ | ||
agentId, | ||
template, | ||
promptCustomizationService, | ||
promptService, | ||
aiSettingsService, | ||
}) => { | ||
const [variantIds, setVariantIds] = React.useState<string[]>([]); | ||
const [selectedVariant, setSelectedVariant] = React.useState<string>(DEFAULT_VARIANT); | ||
|
||
React.useEffect(() => { | ||
(async () => { | ||
const variants = promptService.getVariantIds(template.id); | ||
setVariantIds([DEFAULT_VARIANT, ...variants]); | ||
|
||
const agentSettings = await aiSettingsService.getAgentSettings(agentId); | ||
const currentVariant = | ||
agentSettings?.selectedVariants?.[template.id] || DEFAULT_VARIANT; | ||
setSelectedVariant(currentVariant); | ||
})(); | ||
}, [template.id, promptService, aiSettingsService, agentId]); | ||
|
||
const handleVariantChange = async (event: React.ChangeEvent<HTMLSelectElement>) => { | ||
const newVariant = event.target.value; | ||
setSelectedVariant(newVariant); | ||
|
||
const agentSettings = await aiSettingsService.getAgentSettings(agentId); | ||
const selectedVariants = agentSettings?.selectedVariants || {}; | ||
|
||
const updatedVariants = { ...selectedVariants }; | ||
if (newVariant === DEFAULT_VARIANT) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
delete updatedVariants[template.id]; | ||
} else { | ||
updatedVariants[template.id] = newVariant; | ||
} | ||
|
||
await aiSettingsService.updateAgentSettings(agentId, { | ||
selectedVariants: updatedVariants, | ||
}); | ||
}; | ||
|
||
const openTemplate = React.useCallback(async () => { | ||
promptCustomizationService.editTemplate(template.id, template.template); | ||
}, [template, promptCustomizationService]); | ||
const templateId = selectedVariant === DEFAULT_VARIANT ? template.id : selectedVariant; | ||
const selectedTemplate = promptService.getRawPrompt(templateId); | ||
promptCustomizationService.editTemplate(templateId, selectedTemplate?.template || ''); | ||
}, [selectedVariant, template.id, promptService, promptCustomizationService]); | ||
|
||
const resetTemplate = React.useCallback(async () => { | ||
promptCustomizationService.resetTemplate(template.id); | ||
}, [promptCustomizationService, template]); | ||
|
||
return <> | ||
{template.id} | ||
<button className='theia-button main' onClick={openTemplate}>Edit</button> | ||
<button className='theia-button secondary' onClick={resetTemplate}>Reset</button> | ||
</>; | ||
const templateId = selectedVariant === DEFAULT_VARIANT ? template.id : selectedVariant; | ||
promptCustomizationService.resetTemplate(templateId); | ||
}, [selectedVariant, template.id, promptCustomizationService]); | ||
|
||
return ( | ||
<div className="template-renderer"> | ||
<div className="settings-section-title template-header"> | ||
<strong>{template.id}</strong> | ||
</div> | ||
<div className="template-controls"> | ||
{variantIds.length > 1 && ( | ||
<> | ||
<label htmlFor={`variant-selector-${template.id}`} className="template-select-label"> | ||
Variant: | ||
JonasHelming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</label> | ||
<select | ||
id={`variant-selector-${template.id}`} | ||
className="theia-select template-variant-selector" | ||
value={selectedVariant} | ||
onChange={handleVariantChange} | ||
> | ||
{variantIds.map(variantId => ( | ||
<option key={variantId} value={variantId}> | ||
{variantId} | ||
</option> | ||
))} | ||
</select> | ||
</> | ||
)} | ||
<button className="theia-button main" onClick={openTemplate}> | ||
Edit | ||
</button> | ||
<button className="theia-button secondary" onClick={resetTemplate}> | ||
Reset | ||
</button> | ||
</div> | ||
</div> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
); | ||
this.onDidChangeAgentsEmitter.fire(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,16 @@ import { ToolInvocationRegistry } from './tool-invocation-registry'; | |
import { toolRequestToPromptText } from './language-model-util'; | ||
import { ToolRequest } from './language-model'; | ||
import { matchFunctionsRegEx, matchVariablesRegEx } from './prompt-service-util'; | ||
import { AISettingsService } from './settings-service'; | ||
|
||
export interface PromptTemplate { | ||
id: string; | ||
template: string; | ||
/** | ||
* (Optional) The ID of the main template for which this template is a variant. | ||
* If present, this indicates that the current template represents an alternative version of the specified main template. | ||
*/ | ||
variantOf?: string; | ||
} | ||
|
||
export interface PromptMap { [id: string]: PromptTemplate } | ||
|
@@ -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 commentThe 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 commentThe 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. |
||
/** | ||
* Adds a {@link PromptTemplate} to the list of prompts. | ||
* @param promptTemplate the prompt template to store | ||
*/ | ||
storePromptTemplate(promptTemplate: PromptTemplate): void; | ||
/** | ||
* Removes a prompt from the list of prompts. | ||
* @param id the id of the prompt | ||
|
@@ -77,6 +88,20 @@ export interface PromptService { | |
* Return all known prompts as a {@link PromptMap map}. | ||
*/ | ||
getAllPrompts(): PromptMap; | ||
/** | ||
* Retrieve all variant IDs of a given {@link PromptTemplate}. | ||
* @param id the id of the main {@link PromptTemplate} | ||
* @returns an array of string IDs representing the variants of the given template | ||
*/ | ||
getVariantIds(id: string): string[]; | ||
/** | ||
* Retrieve the currently selected variant ID for a given main prompt ID. | ||
* If a variant is selected for the main prompt, it will be returned. | ||
* Otherwise, the main prompt ID will be returned. | ||
* @param id the id of the main prompt | ||
* @returns the variant ID if one is selected, or the main prompt ID otherwise | ||
*/ | ||
getVariantId(id: string): Promise<string>; | ||
} | ||
|
||
export interface CustomAgentDescription { | ||
|
@@ -163,6 +188,9 @@ export interface PromptCustomizationService { | |
|
||
@injectable() | ||
export class PromptServiceImpl implements PromptService { | ||
@inject(AISettingsService) @optional() | ||
protected readonly settingsService: AISettingsService | undefined; | ||
|
||
@inject(PromptCustomizationService) @optional() | ||
protected readonly customizationService: PromptCustomizationService | undefined; | ||
|
||
|
@@ -203,8 +231,22 @@ export class PromptServiceImpl implements PromptService { | |
return commentRegex.test(template) ? template.replace(commentRegex, '').trimStart() : template; | ||
} | ||
|
||
async getVariantId(id: string): Promise<string> { | ||
if (this.settingsService !== undefined) { | ||
const agentSettingsMap = await this.settingsService.getSettings(); | ||
|
||
for (const agentSettings of Object.values(agentSettingsMap)) { | ||
if (agentSettings.selectedVariants && agentSettings.selectedVariants[id]) { | ||
return agentSettings.selectedVariants[id]; | ||
} | ||
} | ||
} | ||
return id; | ||
} | ||
|
||
async getPrompt(id: string, args?: { [key: string]: unknown }): Promise<ResolvedPromptTemplate | undefined> { | ||
const prompt = this.getUnresolvedPrompt(id); | ||
const variantId = await this.getVariantId(id); | ||
const prompt = this.getUnresolvedPrompt(variantId); | ||
if (prompt === undefined) { | ||
return undefined; | ||
} | ||
|
@@ -280,4 +322,12 @@ export class PromptServiceImpl implements PromptService { | |
removePrompt(id: string): void { | ||
delete this._prompts[id]; | ||
} | ||
getVariantIds(id: string): string[] { | ||
return Object.values(this._prompts) | ||
.filter(prompt => prompt.variantOf === id) | ||
.map(variant => variant.id); | ||
} | ||
storePromptTemplate(promptTemplate: PromptTemplate): void { | ||
this._prompts[promptTemplate.id] = promptTemplate; | ||
} | ||
} |
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.
Added
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