Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

calvingiles
Copy link
Contributor

@calvingiles calvingiles commented Jun 23, 2025

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:

{
  "error": {
    "code": 400,
    "message": "Please ensure that the number of function response parts is equal to the number of function call parts of the function call turn.",
    "status": "INVALID_ARGUMENT"
}

What should happen

The perfect solution to this scenario is a little complex and unclear, but a few things should be true:

  1. when processing the events into contents, the result should always be a valid model input.
  2. If multiple function CALLS are present with the same ID, take the most recent as the only call.
  3. If multiple function RESPONSES are present, take the most recent as the only response.
  4. If calls are present in parallel, while moving one to ensure the above should not automatically move the others.
  5. If responses are present in parallel, while moving one to ensure the above should not automatically move the others.
  6. If multiple calls should occur in a sequence, they should be collected into a parallel call
  7. If multiple responses should occur in a sequence, they should be collected into a parallel response

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 the contents 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.

@calvingiles
Copy link
Contributor Author

calvingiles commented Jun 23, 2025

I noticed #1579 and #1580 - looks like this is an active area of development. I'll investigate whether those address any of the issues I have been seeing.

@calvingiles
Copy link
Contributor Author

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]
Option two (as implemented in #1585): return [(model function call part), user_response]
Option three: return [(model function call part, model text 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)).

@calvingiles
Copy link
Contributor Author

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.

@calvingiles calvingiles force-pushed the bug/fix-function-parts-rewriting branch from f70c6ae to 641dc73 Compare June 24, 2025 03:06
@calvingiles
Copy link
Contributor Author

I have pulled the changes from #1585 for now as the CLA has not been signed. I think it might actually be logic from #1580 that was pulled in, so I will merge the two once that hits main.

@hangfei hangfei requested a review from seanzhou1023 June 24, 2025 21:11
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.

2 participants