GenAI Utils | Update ToolCall Type#4218
GenAI Utils | Update ToolCall Type#4218keith-decker wants to merge 19 commits intoopen-telemetry:mainfrom
Conversation
github-actions
bot
assigned
codefromthecrypt,
karthikscale3,
lmolkova,
lzchen and
nirga
keith-decker
force-pushed
the
pr1-enhance-toolcall-type
branch
from
f766cd6 to
9b3f2a6
Compare
|
I think the docker tests failure is unrelated to this PR. |
| arguments: Any | ||
| name: str | ||
| id: str | None | ||
| type: Literal["tool_call"] = "tool_call" |
There was a problem hiding this comment.
if we are to have 2 different types here, should we also have 2 different type names ? Otherwise why not just have 1 type that encompasses both ?
There was a problem hiding this comment.
I initially started with one type, but ended up blowing up the vertex tests with extra data. It looks like vertexai uses toolcall as part of the body of an LLM request.
I separated these into two types to distinguish between toolcallrequests that are part of an LLM invocation, and a tool execution that may be stand alone.
What are your thoughts on that approach?
There was a problem hiding this comment.
Sounds reasonable but what's in the sem convs ? I think we want these types to match the sem coonvs
There was a problem hiding this comment.
This type here refers to the "type" in the message part for ToolCallRequest (https://github.com/open-telemetry/semantic-conventions/blob/95c6e0a3c6803c6546fc66847855761e9c90f9c6/docs/gen-ai/non-normative/models.ipynb?plain=1)
The type in the new ToolCall refers to the type defined here (https://github.com/open-telemetry/semantic-conventions/blob/95c6e0a3c6803c6546fc66847855761e9c90f9c6/docs/gen-ai/gen-ai-spans.md?plain=1#L618) which is a span level attribute instead of part of the message.
There was a problem hiding this comment.
The type is weirdly named tool_call instead of tool_call_request in the sem conv.. oh well.
That second type https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-spans/#execute-tool-span -- seems like it's just supposed to be a string though unless I'm missing something ??
util/opentelemetry-util-genai/src/opentelemetry/util/genai/types.py
Outdated
Show resolved
Hide resolved
|
I'm a little confused. Is that right? |
|
|
||
|
|
||
| @dataclass() | ||
| class ToolCall(ToolCallRequest): |
There was a problem hiding this comment.
should it inherit class ToolCall(GenAIInvocation) instead ? ToolCallRequest can be separate and ok to have its attributes duplicated in ToolCall?
There was a problem hiding this comment.
Refactored to extend GenAIInvocation instead. Separated ToolCallRequest to align with message parts
tammy-baylis-swi
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
@aabmass As discussed in the SIG call on thursday: |
…e GenericPart for custom message types
| yield user_event(role=content.role, content=request_content) | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
Not sure where this was moved but we can put BlobPart into the Gen AI util types and also add it to Message part --- that's how it exists in the sem convs (https://github.com/open-telemetry/semantic-conventions/blob/95c6e0a3c6803c6546fc66847855761e9c90f9c6/docs/gen-ai/non-normative/models.ipynb?plain=1)
That way we don't need to wrap it in a GenericPart (it should be it's own part)
| @@ -30,6 +30,17 @@ | |||
| ContextToken: TypeAlias = Token[Context] | |||
There was a problem hiding this comment.
Not sure if you saw but https://github.com/open-telemetry/semantic-conventions/pull/3038/changes was merged.. This adds 4 new types:
GenericServerToolCall/GenerticServerToolCallResponse (not added to MessagePart.. maybe because GenericPart is already there)
also ServerToolCallPart/ServerToolCallResponsePart (added to MessagePart)
Maybe we can use this PR to just get completely in sync with https://github.com/open-telemetry/semantic-conventions/blob/95c6e0a3c6803c6546fc66847855761e9c90f9c6/docs/gen-ai/non-normative/models.ipynb?plain=1
9 participants
Description
Update the tool type to match semantic conventions for GenAI Spans.
TelemetryHandler functions for start_tool and stop_tool coming in next PR.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.