Unify model responses #232
Conversation
Deploying pydantic-ai with
|
| Latest commit: |
55ec996
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e3d2238.pydantic-ai.pages.dev |
| Branch Preview URL: | https://dmontagu-unify-model-respons.pydantic-ai.pages.dev |
| if isinstance(m, SystemPrompt): | ||
| return None | ||
| elif isinstance(m, UserPrompt): | ||
| return _content_user_prompt(m) | ||
| elif isinstance(m, ToolReturn): | ||
| return _content_tool_return(m) | ||
| elif isinstance(m, RetryPrompt): | ||
| return _content_retry_prompt(m) | ||
| elif isinstance(m, ModelResponse): # type: ignore[reportUnnecessaryIsInstance] | ||
| return _content_model_response(m) |
There was a problem hiding this comment.
Note — here and elsewhere I have replaced checks on m.role with isinstance checks on m. This makes it easier to rename things — my IDE won't automatically update the values of role (yes I can find-replace, but I don't love assuming that there aren't unrelated usages of that string), and I can't as easily do a cmd+click jump to the definition of the relevant type if we just check the discriminator to narrow it.
If there was a good reason to do it with the role value checking we can go back to that, but if it was just performance relative to an isinstance, I personally think the codebase ergonomics benefits are worth more than the nearly-insignificant performance cost.
dmontagu
force-pushed
the
dmontagu/unify-model-responses
branch
from
eecafc3 to
555d545
Compare
|
Happy to review more thoroughly, but I'm guessing @samuelcolvin should give some feedback on naming here before we move forward? |
There was a problem hiding this comment.
A few ideas:
# take or leave the `Response` prefix.
[Response]Block : TypeAlias= Union[TextBlock, ToolCallBlock]
ModelResponse:
blocks: list[ResponseBlock]This intersects a bit with anthropic terminology, but we can easily sidestep with AnthropicTextBlock, etc. Just like we have to deal with Message overlap...
|
@dmontagu can you update the description accordingly when you have a chance? |
|
|
||
| calls: list[ToolCall] | ||
| """The tool calls being made.""" | ||
| role: Literal['model-response'] = 'model-response' |
There was a problem hiding this comment.
This would be breaking, but I think it would be valuable to just have kind as 'model-response' and role as 'assistant'. Then we could do the same thing for these other message types, with role either being 'user' or 'assistant' (like Anthropic), and kind being the discriminator that's helpful for our use cases.
There was a problem hiding this comment.
Discussed with @dmontagu offline, I'll do this in a separate PR before we do our upcoming release :)
Replaces
ModelStructuredResponseandModelTextResponsewith a singleModelResponsewhich has a heterogeneous list ofitems, some of which may be tool calls and some of which may be text.This matches the Anthropic data model, but also simplifies the code in various places not specific to Anthropic.
Note that we will probably want to make a similar change to streaming responses, which I have not attempted to do yet (I just attempted to keep things working the same as they currently do).
I'm open to renames; I'll note that the names currently in this PR (at least,
ModelResponse.items) were chosen to avoid naming conflicts with Anthropic's APIs, to make it easier to not get confused about what was from them and what was from us. But now that the refactor is done and tests are passing etc., I'm open to renaming as other see fit.