feat: Jinja2 support for system_prompt of the Agent#10718
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
anakin87
changed the title
system_prompt of the Agent
Pull Request Test Coverage Report for Build 22771120562Details
💛 - Coveralls |
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Looking good in principle, I'm just wondering why we don't allow users to provide a system prompt template with variables at runtime if they didn't do so when initializing the agent.
| :param system_prompt: System prompt for the agent. Can be a plain string or a Jinja2 string template. | ||
| For details on the supported template syntax, refer to the | ||
| [documentation](https://docs.haystack.deepset.ai/docs/chatpromptbuilder#string-templates). | ||
| :param user_prompt: User prompt for the agent, defined as a Jinja2 string template. If provided, this is | ||
| appended to the messages provided at runtime. | ||
| For details on the supported template syntax, refer to the | ||
| [documentation](https://docs.haystack.deepset.ai/docs/chatpromptbuilder#string-templates). |
There was a problem hiding this comment.
Why do we allow for system_prompt both string and Jinja2 string template but for user_prompt only Jinja2 string template?
There was a problem hiding this comment.
user_prompt was originally introduced in #10638 for Platform needs. Here, I just expanded the docstring to make it clear that we expect Jinja2 templates.
In system_prompt, we accept both because the request was to allow Jinja2 templates, but keeping compatibility with the existing plain string behavior.
|
|
||
| self._chat_prompt_builder: ChatPromptBuilder | None = self._initialize_chat_prompt_builder( | ||
| user_prompt, required_variables | ||
| self._user_chat_prompt_builder = ( |
There was a problem hiding this comment.
Maybe we can add a comment here saying that required variables will be set in _register_prompt_variables?
| if self._system_chat_prompt_builder is None: | ||
| raise ValueError( | ||
| "system_prompt contains Jinja2 template syntax but no system prompt builder is initialized. " | ||
| "Please make sure a system_prompt with Jinja2 template syntax is provided at initialization " | ||
| "time." | ||
| ) |
There was a problem hiding this comment.
To allow passing a system prompt with variables at run time, the user also needs to pass a system prompt with variables when initializing the agent? Why?
There was a problem hiding this comment.
This mostly mirrors the choices made in #10638 for user_prompt
It is also worth noting that, similarly to the ChatPromptBuilder, a
user_promptcan be provided at both init and run time, with one caveat: providing auser_promptat run time, and not init would raise an error as the necessary ChatPromptBuilder instance wouldn't have been initiated prior.
That behavior could be disputed, there is the possibility of instantiating the necessary ChatPromptBuilder ad-hoc at run-time as well, and handle it gracefully. But my understand of that API is that it is already quite 'hacky' and most likely not going to be used. Happy to be told otherwise!
In general, I agree with this view, also because now the failure is clear and explicit.
(I'd even be in favor of restricting the API further to avoid footguns).
Allowing dynamic system prompts:
- we would need to build Chat Prompt Builders at runtime, adding complexity and a performance penalty
- the Agent's inputs would become much more dynamic vs having components' inputs and required variables set at init
- in addition,
system_promptcan accept both a plain string and a Jinja2 template. If we allow any template atrun, this would also enable switching from a static prompt to a dynamic one at runtime, which would complicate things.
Happy to revisit if we see a concrete use case.
haystack/components/agents/agent.py
Outdated
| if len(user_messages) != 1 or not user_messages[0].is_from(ChatRole.USER): | ||
| raise ValueError("user_prompt must render to exactly one user message.") |
There was a problem hiding this comment.
Maybe we can give raise here different error based on whether there are too many messages or the message has the wrong role?
haystack/components/agents/agent.py
Outdated
| if len(system_messages) != 1 or not system_messages[0].is_from(ChatRole.SYSTEM): | ||
| raise ValueError("system_prompt must render to exactly one system message.") |
There was a problem hiding this comment.
Maybe we can give raise here different error based on whether there are too many messages or the message has the wrong role?
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Looking good, thanks for answering my questions 🙏
3 participants
Related Issues
Proposed Changes:
system_promptalso accept and handle the Jinja2 Chat Message templatemeta({% message role="system" meta={"key": "value"}%}...). Seetest_system_prompt_with_meta.test_system_prompt_and_user_promptHow did you test it?
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.