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

Google: handle FinishReason.MALFORMED_FUNCTION_CALL #1442

Merged

Conversation

tadamcz
Copy link
Contributor

@tadamcz tadamcz commented Mar 6, 2025

This PR contains:

  • New features
  • Changes to dev-tools e.g. CI config / github tooling
  • Docs
  • Bug fixes
  • Code refactor

What is the current behavior? (You can also link to an open issue here)

When FinishReason.MALFORMED_FUNCTION_CALL, Inspect raises inside completion_choice_from_candidate because content.parts is None

def completion_choice_from_candidate(candidate: Candidate) -> ChatCompletionChoice:

What is the new behavior?

Properly handle the case FinishReason.MALFORMED_FUNCTION_CALL. I decided to map it to StopReason "unknown", but I am also open to adding a new option to StopReason if that is preferred.

Added a test

@tadamcz
Copy link
Contributor Author

tadamcz commented Mar 6, 2025

Looks like some mypy errors on unchanged files (in the mistral provider), that should not be possible 🤔

Could this be because we don't use lockfiles?

@tadamcz tadamcz force-pushed the google-handle-malformed-function-call branch from ee55fc2 to c962767 Compare March 6, 2025 20:14
@tadamcz
Copy link
Contributor Author

tadamcz commented Mar 6, 2025

@jjallaire I'm not going to fix what seems to me like an unrelated CI bug, would you consider either (a) fix the CI behaviour or (b) merge despite the CI failures?

@jjallaire
Copy link
Collaborator

Some related discussion: googleapis/python-aiplatform#4472

@jjallaire
Copy link
Collaborator

@jjallaire I'm not going to fix what seems to me like an unrelated CI bug, would you consider either (a) fix the CI behaviour or (b) merge despite the CI failures?

Yeah that's mistral making a breaking change in their API. I'll take care of that in a separate PR.

@jjallaire
Copy link
Collaborator

I am partial to "unknown" just to not knee-jerk spray another stop reason, but let me take a closer look and think on it tomorrow.

@tadamcz
Copy link
Contributor Author

tadamcz commented Mar 6, 2025

Yeah that's mistral making a breaking change in their API

Makes sense, but with lockfiles this would not cause random breakages :)

@jjallaire
Copy link
Collaborator

Makes sense, but with lockfiles this would not cause random breakages :)

It's a feature that there is breakage --- otherwise we would never be alerted that they have made a breaking change which just throws the problem into users laps. The mindset "we never want anything external to break our CI" is fine for a production deployment but a package that wants to be a good citizen and flexible w/r/t dependency resolution in myriad settings needs to know about these ASAP.

@jjallaire
Copy link
Collaborator

To be consistent with the way this is handled for other providers, we would ideally have a stop reason of "tool_calls" and allow the invalid function call (or some simulation thereof) to propagate through. It is quite common for models to call tools that don't exist or provide incorrect JSON schema -- the default tool loop handles these cases by replying to the model letting them know that they've made an incorrect tool call (and they will very often successfully recover).

If we do stop reason "unknown" the tool loop will just end (note that for some agents including basic_agent() the model will be re-prompted to continue, but critically they won't get any feedback that their tool call was wrong, possibly leading them to just make the same mistake again.

All of that said I don't even know whether anything like what I am suggesting is possible. It really depends on what Google returns if anything along with this stop reason. In the case that the context is insufficient we may be stuck with "unknown".

@tadamcz
Copy link
Contributor Author

tadamcz commented Mar 7, 2025 via email

@jjallaire jjallaire merged commit 895282c into UKGovernmentBEIS:main Mar 7, 2025
10 checks passed
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