-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(messages): add ToolReturnPart to ModelResponsePart union
#5723
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?
Changes from 3 commits
46b8a08
9f283a6
e7e25e3
b301af7
dc2cac8
ee2c5b2
b263601
d0e3388
10ca979
bcdfb3c
c5b6be8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2066,6 +2066,7 @@ def _model_response_part_discriminator(v: Any) -> str | None: | |
| | Annotated[NativeToolCallPart, pydantic.Tag('builtin-tool-call')] | ||
| | Annotated[NativeToolSearchReturnPart, pydantic.Tag('builtin-tool-search-return')] | ||
| | Annotated[NativeToolReturnPart, pydantic.Tag('builtin-tool-return')] | ||
| | Annotated[ToolReturnPart, pydantic.Tag('tool-return')] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DouweM This is the key design decision in the PR and I think it warrants explicit maintainer sign-off before proceeding. Context: Issue #5721 was auto-generated by a The question: Should Trade-offs:
The right call depends on whether "user-constructed
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. David's AICA here: Decision on the design question (Douwe is away, so I'm making the call): we'll keep the union-widening approach — adding the base Reasoning:
One correction to the framing above: the framework never produces a base |
||
| | Annotated[ThinkingPart, pydantic.Tag('thinking')] | ||
| | Annotated[CompactionPart, pydantic.Tag('compaction')] | ||
| | Annotated[FilePart, pydantic.Tag('file')], | ||
|
|
@@ -2285,7 +2286,7 @@ def new_event_body(): | |
|
|
||
| return result | ||
|
|
||
| def otel_message_parts(self, settings: InstrumentationSettings) -> list[_otel_messages.MessagePart]: | ||
| def otel_message_parts(self, settings: InstrumentationSettings) -> list[_otel_messages.MessagePart]: # noqa: C901 | ||
|
dsfaccini marked this conversation as resolved.
|
||
| parts: list[_otel_messages.MessagePart] = [] | ||
| for part in self.parts: | ||
| if isinstance(part, TextPart): | ||
|
|
@@ -2333,6 +2334,10 @@ def otel_message_parts(self, settings: InstrumentationSettings) -> list[_otel_me | |
| return_part['result'] = serialize_any(part.content) | ||
|
|
||
| parts.append(return_part) | ||
| elif isinstance(part, ToolReturnPart): | ||
| # A user-defined tool return can appear here via user-constructed message history; map it | ||
| # like its request-side counterpart (no `builtin` flag) so it isn't dropped from telemetry. | ||
| parts.extend(part.otel_message_parts(settings)) | ||
| elif isinstance(part, CompactionPart): | ||
| # Compaction parts don't map to standard OTel message part types | ||
| pass | ||
|
|
@@ -2710,7 +2715,16 @@ class PartStartEvent: | |
| """The newly started `ModelResponsePart`.""" | ||
|
|
||
| previous_part_kind: ( | ||
| Literal['text', 'thinking', 'tool-call', 'builtin-tool-call', 'builtin-tool-return', 'compaction', 'file'] | ||
| Literal[ | ||
| 'text', | ||
| 'thinking', | ||
| 'tool-call', | ||
| 'builtin-tool-call', | ||
| 'builtin-tool-return', | ||
| 'tool-return', | ||
| 'compaction', | ||
| 'file', | ||
| ] | ||
|
dsfaccini marked this conversation as resolved.
|
||
| | None | ||
| ) = None | ||
| """The kind of the previous part, if any. | ||
|
|
@@ -2751,7 +2765,16 @@ class PartEndEvent: | |
| """The complete `ModelResponsePart`.""" | ||
|
|
||
| next_part_kind: ( | ||
| Literal['text', 'thinking', 'tool-call', 'builtin-tool-call', 'builtin-tool-return', 'compaction', 'file'] | ||
| Literal[ | ||
| 'text', | ||
| 'thinking', | ||
| 'tool-call', | ||
| 'builtin-tool-call', | ||
| 'builtin-tool-return', | ||
| 'tool-return', | ||
| 'compaction', | ||
| 'file', | ||
| ] | ||
| | None | ||
| ) = None | ||
| """The kind of the next part, if any. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -338,7 +338,7 @@ async def handle_event(self, event: NativeEvent) -> AsyncIterator[EventT]: # no | |
| case _: | ||
| pass | ||
|
|
||
| async def handle_part_start(self, event: PartStartEvent) -> AsyncIterator[EventT]: | ||
| async def handle_part_start(self, event: PartStartEvent) -> AsyncIterator[EventT]: # noqa: C901 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the second |
||
| """Handle a `PartStartEvent`. | ||
|
|
||
| This method dispatches to specific `handle_*` methods based on part type: | ||
|
|
@@ -381,6 +381,9 @@ async def handle_part_start(self, event: PartStartEvent) -> AsyncIterator[EventT | |
| case CompactionPart(): # pragma: no cover | ||
| async for e in self.handle_compaction(part): | ||
| yield e | ||
| case ToolReturnPart(): # pragma: no cover | ||
| # User-defined tool returns in user-constructed message history have no UI start event. | ||
| pass | ||
|
|
||
| async def handle_part_delta(self, event: PartDeltaEvent) -> AsyncIterator[EventT]: | ||
| """Handle a PartDeltaEvent. | ||
|
|
@@ -440,7 +443,7 @@ async def handle_part_end(self, event: PartEndEvent) -> AsyncIterator[EventT]: | |
| case NativeToolCallPart(): | ||
| async for e in self.handle_builtin_tool_call_end(part): | ||
| yield e | ||
| case NativeToolReturnPart() | FilePart() | CompactionPart(): # pragma: no cover | ||
| case NativeToolReturnPart() | FilePart() | CompactionPart() | ToolReturnPart(): # pragma: no cover | ||
| # These don't have deltas, so they don't need to be ended. | ||
| pass | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.