-
Notifications
You must be signed in to change notification settings - Fork 388
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
Generate tool results when using structured result #179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, some tests are failing, but you can probably fix most of them by running uv run pytest --fix
.
LMK if you want me to finish this off.
# we have a final result, end the conversation | ||
result_data = either.data | ||
# Add all messages to the conversation | ||
messages.extend(r for r in responses if isinstance(r, _messages.Message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we extract the Message
s from the list 3 times, maybe better to return tuple[_MarkFinalResult[ResultData] | None, list[_messages.Message]]
?
Then we don't have to do any extraction aftwards.
We should also test if the responses need to be adjacent in the messages, I didn't think they needed to be, but @sudoskys suggested they do. That can be a separate PR. |
Looking at those tests now and I like your suggestion about the tuple. Responses do need to be adjacent for OpenAI, not sure if others have the same requirement (message expectations vary). The OpenAI requirement is: a message with one or more tool calls must be followed immediately and only by messages containing responses to every tool call. For example if 2 parallel tool calls are made, the next two messages must be responses corresponding to those two tool call ids. |
I think this is GTG. In testing I identified another situation that can result in the same error message, in which the model requests a "regular" tool call and a "final" tool call in parallel in the same payload. Even though this PR adds the |
I think that's what's reported in #161. There's some more discussion needed in that case. |
Ok cool, I have a proposal I'll add on that issue. |
Thanks so much @jlowin. |
Closes #174