-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix #1238 by enhancing HandoffInputData and enable passing async functions #1302
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
Conversation
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.
comments for reviewers
@@ -785,6 +786,8 @@ async def execute_handoffs( | |||
) | |||
raise UserError(f"Invalid input filter: {input_filter}") | |||
filtered = input_filter(handoff_input_data) | |||
if inspect.isawaitable(filtered): |
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 support this use case: #1238 (comment)
Note that, since this property was added later on, it's optional for backwards compatibility. | ||
""" | ||
|
||
def clone(self, **kwargs: Any) -> HandoffInputData: |
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.
added clone method for convenience but if we don't want to have this, happy to revert this change
@@ -262,7 +263,7 @@ async def test_handoff_filters(): | |||
|
|||
|
|||
@pytest.mark.asyncio | |||
async def test_async_input_filter_fails(): | |||
async def test_async_input_filter_supported(): |
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.
if we didn't intentionally block async functions, i believe changing this is a good addition
@@ -241,6 +241,7 @@ def remove_new_items(handoff_input_data: HandoffInputData) -> HandoffInputData: | |||
input_history=handoff_input_data.input_history, | |||
pre_handoff_items=(), | |||
new_items=(), | |||
run_context=handoff_input_data.run_context, |
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 added run_context param in tests but the ones under examples do not have the param. this means backward compatibility is kept
@@ -49,8 +49,24 @@ class HandoffInputData: | |||
handoff and the tool output message representing the response from the handoff output. | |||
""" | |||
|
|||
run_context: RunContextWrapper[Any] | None = 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.
if a different name is preferred, happy to rename this
This pull request resolves #1238