-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add on_complete
callback to AG-UI functions to get access to AgentRunResult
#2429
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
Add on_complete
callback to AG-UI functions to get access to AgentRunResult
#2429
Conversation
on_complete
callback to AG-UI functions to get access to AgentRunResult
a3ad970
to
73af2b7
Compare
if _utils.is_async_callable(on_complete): | ||
await on_complete(run) | ||
else: | ||
await _utils.run_in_executor(on_complete, run) |
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.
@DouweM not sure this is what you meant...
if _utils.is_async_callable(on_complete):
await on_complete(run)
"None" is not awaitable
"None" is incompatible with protocol "Awaitable[_T_co@Awaitable]"
"await" is not present
And
else:
await _utils.run_in_executor(on_complete, run)
Is saying it's unreachable.
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.
FWIW, I just ran in my project with both a sync and async callback and it seems to work as expected... Maybe just a type issue?
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.
Weird, I'm seeing the same, looks like is_async_callable
's TypeIs
return annotation is subtly broken... I'm Ok with going back to what you had in that case, running this in an executor (i.e. a thread) seems less important than doing so for tools anyway.
This PR is stale, and will be closed in 3 days if no reply is received. |
@DouweM is this still a priority? If the type error is blocking, I can go ahead and revert back to:
|
@ChuckJonas Yeah let's go back to what you had! |
Thanks @ChuckJonas for suggesting and looking into this! It's something I'm also interested in using for accessing |
ec706f1
to
9dab158
Compare
9dab158
to
badcea0
Compare
@DouweM I think I correctly reverted these line and I rebased to resolve conflicts. AFIAK the PR should be good (not sure why the CI coverage check is failing, I believe it's unrelated but not familiar enough with the tooling to say for sure). |
@ChuckJonas Thanks Charlie! |
Fixes #2398