-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Fix Function Part re-writing logic to always be valid model inputs #1582
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?
fix: Fix Function Part re-writing logic to always be valid model inputs #1582
Conversation
I have incorporated the additional tests from #1585 in an additional commit. They mostly all passed without changing the implementation from the original in this PR (except catching a missed None case - thank you). I did have to change the shapes of the assertions due to a small contract change in the private methods, but this is only used internally. The one significant change, however, is changing an expectation: Given: [model function call event, model text event, user response] Option one (as implemented in this PR): return [(model text part, model function call part), user_response] I am open to input from library authors, but I do not believe that dropping the model message in this situation is desirable. It is also not desirable to reorder the parts. To that end, I believe the implementation here is missing a tie breaker when parts have equal priority in merging (perhaps the event timestamp would serve as this)). |
Having reflected on this, it seems that preserving the original part order where possible is probably the preferred approach (Option 3 above). To that end, I have adjusted the sorting logic to ensure that within an event, parts are always sorted according to their original timestamps and part indexes. This ensures that parts from different events maintain their conversational order, and parts from the same event maintain their order, while function calls still get pulled up to the latest. As always, keen for comment from the library maintainers. |
f70c6ae
to
641dc73
Compare
What happens
At present, when multiple function responses are provided for the same function call in different events AND those responses occur in parallel with responses for function responses that do not correlate then the event re-writing logic can incorrectly include too many function responses, resulting in as error like:
What should happen
The perfect solution to this scenario is a little complex and unclear, but a few things should be true:
Note: points 6 and 7 seem to suggest that models MUST support parallel function calling, and that this would break with models that don't. This is not in fact a requirement as the only way for these above re-writing rules to bring multiple responses together is if a series of function responses were to be ordered together with no model parts interleaved - i.e. there must be parallel function responses already present to bring this about. Regarding parallel calls, since the above logic will always bring a matching number of calls to responses into the preceding turn, parallel function calling requires parallel responses to already exist. The other situation for parallel calls is when unmatched - which should only occur naturally at the end of a conversation, and in these cases they would also need to already be present to be returned as such.
Solution
A new
_extract_latest_function_parts
method that performs direct creation of thecontents
according to these rules.The function calls and responses are special cased and manipulated. All other parts are kept as they were. This leaves all auth and other invalid event filtering logic in place, as well as foreign agent part re-writing.
Code source
All tests and proposed implementation were coded without AI, so there is negligible likelihood that these snippets of code exist anywhere in copyrighted source code.
Update - added validation
I have added an additional validator to the mock model and tests to catch scenarios where this problem occurs in the future.