-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Allow to pass both session and input list #1298
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
base: main
Are you sure you want to change the base?
Conversation
src/agents/run.py
Outdated
@@ -1212,8 +1223,17 @@ async def _prepare_input_with_session( | |||
# Convert input to list format | |||
new_input_list = ItemHelpers.input_to_new_input_list(input) | |||
|
|||
# Combine history with new input | |||
combined_input = history + new_input_list | |||
if session_input_handling == "append" or session_input_handling is None: |
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.
To me, allowing to pass callback function here to combine the item list to save sounds more flexible. @rm-openai what do you think?
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.
Yes, this would offer much more flexibility.
However, would you still preserve the option for the users to specify "replace" or "append" in order to apply some predefined processing?
In that scenario, the RunConfig
attribute could accept a Callable
, a str
or None
.
Or would you prefer to have a more complex implementation like the one done for MCP filters (ToolFilterCallable
, etc)?
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.
Sessions are supposed to be used for maintaining conversation history across multiple agent runs. So, I don't think "replace" is a common use case. If "replace" works well, do you really need a session for the case?
If a developer would like to make some adjustments to stored history based on some conditions, that could be a valid use case (I still think it's an edge case though). That's why I thought enabling callback functions here would make more sense.
Let me know if I am missing anything!
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 agree. And what should happen if the callback function is left unspecified (None
) and the input is a list? We may raise an error, as in the current implementation.
Moreover, does the callback function only take the history and the list of new items as input, or should I allow additional parameters?
I implemented a new version, where the Let me know what else should be improved. |
I'm missing some context, what's the use case for this? I read the issue but couldn't quite figure it out |
@rm-openai Adding some context, as I just faced this limitation: An agent currently cannot be run using built-in sessions from the SDK while receiving two new inputs from the user. This is requested, for example, when passing an image and text (i.e., Product Image + “Do you have this product in black?”). |
Summary
This PR has been implemented to resolve #1140 . I added a parameter to the
RunConfig
object such that it can be specified if the given input list will replace the session history or it will be appended to it.The current implementation is a possible approach but I would be happy to change the code if there are better solutions.
@seratch @rm-openai whenever you have a moment, please let me know your thoughts.
Test plan
I added 2 unit tests in
tests/test_session.py
.Checks
make lint
andmake format