-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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?”). |
This PR is stale because it has been open for 10 days with no activity. |
@rm-openai I implemented this feature because it seemed useful to several people, but if you don't want to include it, just let me know and I'll close the PR. |
Multimodal input is a fundamental feature for agents, so please allow users to handle it through the built-in session management. Currently, I have to call .to_input_list() and manage it manually. |
This PR is stale because it has been open for 10 days with no activity. |
We really need this feature! |
I also have the same problem above, please! |
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.
Codex Review: Here are some suggestions.
openai-agents-python/src/agents/run.py
Lines 551 to 552 in 48f7dc8
# Save the conversation to session if enabled | |
await self._save_result_to_session(session, input, result) |
[P2] Persist modified session inputs when using callbacks
The new session_input_callback
is applied only when preparing the model input, but the conversation saved back to the session still uses the unmodified input
value (await self._save_result_to_session(session, input, result)
). If a callback rewrites or filters the user input/history (e.g. to replace existing history or truncate inputs) the model will see the modified list while the session will store the original values, so the next turn will reload stale messages and undo the callback’s intent. To keep session memory consistent with the callback’s output, _save_result_to_session
should receive the already combined/filtered list or otherwise allow the callback to influence what is persisted.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
""" | ||
|
||
|
||
SessionInputHandler = Union[SessionMixerCallable, 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.
For consistency with other code in this SDK, can you remove None from here and use SessionInputHandler | None
in other source files? Also, can you make the type name consistent with the property name session_input_callback?
@@ -179,6 +179,14 @@ class RunConfig: | |||
An optional dictionary of additional metadata to include with the trace. | |||
""" | |||
|
|||
session_input_callback: SessionInputHandler = 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 avoid confusion for code readers, remove None union type from SessionInputHandler and change the type def here (and others):
session_input_callback: SessionInputHandler = None | |
session_input_callback: SessionInputCallback | None = None |
# ambiguity about whether the list should append to or replace existing session history | ||
if isinstance(input, list): | ||
# If the user doesn't explicitly specify a mode, raise an error | ||
if isinstance(input, list) and not session_input_callback: |
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.
Asking users to pass session_input_callback is a breaking change, plus not having the callback should be fine
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