-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Refactor AssistantAgent on_message_stream #5642
base: main
Are you sure you want to change the base?
Refactor AssistantAgent on_message_stream #5642
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5642 +/- ##
==========================================
+ Coverage 75.56% 75.64% +0.08%
==========================================
Files 171 171
Lines 10483 10542 +59
==========================================
+ Hits 7921 7975 +54
- Misses 2562 2567 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I think a refactor is good, but I think it can hurt understandability of the function. Is it possible to make any of these inner functions static? That way we know exactly what is going in and out of each of them |
Yeah, there are a lot of things going on in the |
I updated it to make the inner methods either static or class to make it clearer which state goes into each call. It makes it more verbose because there is a lot of class state reuse (not necessarily being changed) such as the model client and tools being passed internally. |
Why are these changes needed?
I'm unsure if everyone will agree, but I started to look into adding new logic and found that refactoring into smaller functions would make it more maintainable.
There is no change in functionality, only a breakdown into smaller methods to make it more modular and improve readability. There is a lot of logic in the method and this refactor breaks it down into context management, llm call and result processing.
Related issue number
Checks