Skip to content
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

Allow background exceptions to be fatal #5716

Merged
merged 17 commits into from
Feb 26, 2025

Conversation

jackgerrits
Copy link
Member

@jackgerrits jackgerrits commented Feb 26, 2025

Closes #4904

Does not change default behavior in core.

In agentchat, this change will mean that exceptions that used to be ignored and result in bugs like the group chat stopping are now reported out to the user application.

@jackgerrits
Copy link
Member Author

Based on my testing, I believe it is currently invalid to let an agentchat runtime ignore background exceptions.

What happens is, an agent is invoked to produce its response. Instead of the agent publishing its response, it raises an exception. The runtime catches this, logs it, and continues on. However, since no result was published the runtime now becomes idle and the runtime terminates. This causes early termination of the team, but it also doesn't propagate the error as it was ignored is the background.

Based on this, I think the current design of agentchat does not allow for automatic recovery from exception and we should in fact require these exceptions to be surfaced.

@ekzhu thoughts?

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.67%. Comparing base (dc55ec9) to head (e932c60).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5716      +/-   ##
==========================================
- Coverage   74.91%   71.67%   -3.25%     
==========================================
  Files         175      115      -60     
  Lines       11137     8225    -2912     
==========================================
- Hits         8343     5895    -2448     
+ Misses       2794     2330     -464     
Flag Coverage Δ
unittests 71.67% <100.00%> (-3.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jackgerrits jackgerrits marked this pull request as ready for review February 26, 2025 14:39
@jackgerrits jackgerrits mentioned this pull request Feb 26, 2025
3 tasks
@jackgerrits
Copy link
Member Author

jackgerrits commented Feb 26, 2025

CC @lokitoth for dotnet

@ekzhu
Copy link
Collaborator

ekzhu commented Feb 26, 2025

Agree. I think the exceptions should be surfaced to the application.

@jackgerrits jackgerrits enabled auto-merge (squash) February 26, 2025 18:30
@jackgerrits jackgerrits merged commit 6b68719 into main Feb 26, 2025
53 checks passed
@jackgerrits jackgerrits deleted the feature/4904-exception-handling branch February 26, 2025 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling policy for unhandled agent exceptions
4 participants