- Notifications
You must be signed in to change notification settings - Fork1k
feat: Add output function tracing#2191
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@bitnahian I didn't understand many of the design decisions behind |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@@ -529,7 +530,8 @@ async def _handle_tool_calls( | |||
if isinstance(output_schema, _output.ToolOutputSchema): | |||
for call, output_tool in output_schema.find_tool(tool_calls): | |||
try: | |||
result_data = await output_tool.process(call, run_context) | |||
trace_context = _output.build_trace_context(ctx) | |||
result_data = await output_tool.process(call, run_context, trace_context.with_call(call)) |
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.
looks likeoutput_tool.process
can do the.with_call(call)
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.
please move it there
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@dataclass(frozen=True) | ||
class TraceContext: | ||
"""A context for tracing output processing.""" | ||
tracer: Tracer | ||
include_content: bool | ||
call: _messages.ToolCallPart | None = None | ||
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.
I've noticed a preference towards dataclasses in the code base. Just curious as to why this is the preferred choice? Is it primarily because of serialisation semantics?
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.
a dataclass works nicely here and makes more sense than a 'plain' class.
a pydantic BaseModel would be better suited for data intended to be (de)serialized.
if trace_context.call: | ||
span_manager = trace_context.span(trace_context.call, include_tool_call_id=True) | ||
else: | ||
function_name = getattr(self._function_schema.function, '__name__', 'output_function') |
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.
Is there a better way to extract the function name from the schema?
@alexmojaki thanks for the review. I've made your suggested changes. |
@@ -529,7 +530,8 @@ async def _handle_tool_calls( | |||
if isinstance(output_schema, _output.ToolOutputSchema): | |||
for call, output_tool in output_schema.find_tool(tool_calls): | |||
try: | |||
result_data = await output_tool.process(call, run_context) | |||
trace_context = _output.build_trace_context(ctx) | |||
result_data = await output_tool.process(call, run_context, trace_context.with_call(call)) |
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.
please move it there
@dataclass(frozen=True) | ||
class TraceContext: | ||
"""A context for tracing output processing.""" | ||
tracer: Tracer | ||
include_content: bool | ||
call: _messages.ToolCallPart | None = None | ||
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.
a dataclass works nicely here and makes more sense than a 'plain' class.
a pydantic BaseModel would be better suited for data intended to be (de)serialized.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
Have addressed comments in places I missed. Lmk if any more changes are required. |
Uh oh!
There was an error while loading.Please reload this page.
try: | ||
output = await self._function_schema.call(output, run_context) | ||
output = await trace_context.execute_function_with_span( |
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.
Possible followup: dedupe this whole try/except
Thanks! |
f9f1c03
intopydantic:mainUh oh!
There was an error while loading.Please reload this page.
Fixes#2108