-
Notifications
You must be signed in to change notification settings - Fork 484
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
Do not add system prompt part when dynamic system prompt function returns empty value #864
base: main
Are you sure you want to change the base?
Conversation
The use of |
Thanks for the contribution. I'll chat with the team about this idea tomorrow. |
@@ -20,15 +20,15 @@ def __post_init__(self): | |||
self._takes_ctx = len(inspect.signature(self.function).parameters) > 0 | |||
self._is_async = inspect.iscoroutinefunction(self.function) | |||
|
|||
async def run(self, run_context: RunContext[AgentDepsT]) -> str: | |||
async def run(self, run_context: RunContext[AgentDepsT]) -> Union[str, None]: # noqa UP007 |
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.
You should be fine to use str | None
thanks to from __future__ import annotations
. There may be some places where you need to use Union
, but those would only be places where we analyzed the type-hint at runtime.
if self._takes_ctx: | ||
args = (run_context,) | ||
else: | ||
args = () | ||
|
||
if self._is_async: | ||
function = cast(Callable[[Any], Awaitable[str]], self.function) | ||
function = cast(Callable[[Any], Awaitable[Union[str, None]]], self.function) |
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, this is a place where you need Union[str, None]
, though I think it would probably work to just use the string (i.e., 'str | None'
)
if updated_part_content: | ||
msg.parts[i] = _messages.SystemPromptPart( | ||
updated_part_content, dynamic_ref=part.dynamic_ref | ||
) |
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 seems wrong to me — with this change, if the function returns None
(or ''
) we just don't update the part, rather than removing it. I think there are a few ways to do this more correctly:
- option 1: remove the part from the
msg.parts
list (which would need to be done carefully..) - option 2: allow
None
inmsg.parts
, I don't love this - option 3: set the
msg.parts[i]
to be aSystemPromptPart
with empty string as the value, and modify the handling in theModel
implementations to ignoreSystemPromptPart
s with empty content.
I pushed a commit to undo some of the unnecessary typing changes. I feel like the "right" way to fix the issue I pointed out might be to use Either way, we need to make sure that a dynamic system prompt that changes from returning something truthy to returning something non-truthy, or vice versa, doesn't lead to misbehavior. |
The
system_prompt
decorator allows to dynamically add parts to the messages. However, a part generated by a decorated function is always added, even if the function output is useless, e.g. an empty string. Sometimes the dynamic behaviour we need is to just increment the system prompt, according to the context.This PR add this behaviour by:
str | None
, so that it is explicit that these function can optionally return a valueExample of a dynamic system prompt where we add some information only if the retrieve call is successful