-
Notifications
You must be signed in to change notification settings - Fork 4
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
Es/feat/ai chat agent promptservice #99
Conversation
/** | ||
* Allows to directly replace placeholders in the prompt. The supported format is 'Hi ${name}!'. | ||
* The placeholder is then searched inside the args object and replaced. | ||
* @param id the id of the prompt | ||
* @param args the object with placeholders, mapping the placeholder key to the value | ||
*/ | ||
getPrompt(id: string, args?: { [key: string]: unknown }): string | undefined; | ||
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.
Shouldn't we still allow new prompts to be stored? For non-agent use cases as well as agents storing new templates.
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 can add it back in
2996ceb
to
99702b4
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.
LGTM
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.
Very nice, thank you! Just a few minor inline comments. Looks great otherwise!
bind(PromptServiceImpl).toSelf().inSingletonScope(); | ||
bind(PromptService).toService(PromptServiceImpl); | ||
bind(DefaultChatAgent).toSelf().inSingletonScope(); | ||
bind(Agent).toService(DefaultChatAgent); |
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 wonder whether we shouldn't automatically register automatically all bindings for ChatAgent
to Agent
too. Having to do two registrations seems superfluous.
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 would I do this?
Looking at inversify I can only find this: https://github.com/inversify/InversifyJS/blob/master/wiki/transitive_bindings.md
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.
Oh, I meant that the class that is collecting the ChatAgent
contributions, just puts them into the agent registry too programmatically.
this.agents.getContributions().forEach(a => { | ||
a.promptTemplates.forEach(t => { | ||
this._prompts[t.id] = t; | ||
}); | ||
}); |
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.agents.getContributions().forEach(a => { | |
a.promptTemplates.forEach(t => { | |
this._prompts[t.id] = t; | |
}); | |
}); | |
this.agents.getContributions().forEach(agent => { | |
agent.promptTemplates.forEach(template => { | |
this._prompts[template.id] = template; | |
}); | |
}); |
@injectable() | ||
export class DefaultChatAgent implements ChatAgent { | ||
@inject(LanguageModelRegistry) | ||
modelProviderRegistry: LanguageModelRegistry; |
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 probably rename the variable to languageModelRegistry
.
const languageModelResponse = await ( | ||
await this.modelProviderRegistry.getLanguageModels() | ||
)[0].request({ messages: getMessages(request.session) }); |
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.
It might be safer check if a language model is available, and show an error message in the chat, if there is no language model.
99702b4
to
66ad85b
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.
Thanks, looks good to me! 👍
What it does
How to test
Follow-ups
Review checklist
Reminder for reviewers