Skip to content

Adding thinkingpart to otel_events in ModelResponse #2237

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

adtyavrdhn
Copy link
Contributor

@adtyavrdhn adtyavrdhn commented Jul 17, 2025

Closes #2131

Moves from each TextPart being its own event to clubbing all TextParts and ThinkingPart events into one body

Updates the tests based on this new approach

if settings.include_content:
body['content'] = part.content
body.setdefault('content', []).append(
{'kind': 'thinking' if isinstance(part, ThinkingPart) else 'text', 'text': part.content}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not too pleased with : 'kind': 'text', 'text': part.content

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only if there's multiple parts. it's the same in ModelRequests, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, In ModelRequests each part again gets its own Event

'role': 'system',
'gen_ai.message.index': 0,
'gen_ai.system': 'my_system',
},
{
'event.name': 'gen_ai.user.message',
'content': 'user_prompt',
'role': 'user',
'gen_ai.message.index': 0,
'gen_ai.system': 'my_system',
},
{
'event.name': 'gen_ai.tool.message',
'content': 'tool_return_content',
'role': 'tool',
'name': 'tool3',
'id': 'tool_call_3',
'gen_ai.message.index': 0,
'gen_ai.system': 'my_system',
},
{
'event.name': 'gen_ai.tool.message',
'content': """\
retry_prompt1
Fix the errors and try again.\
""",
'role': 'tool',
'name': 'tool4',
'id': 'tool_call_4',
'gen_ai.message.index': 0,
'gen_ai.system': 'my_system',
},
{
'event.name': 'gen_ai.user.message',
'content': """\
Validation feedback:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We display {'kind' : 'text'} in ModelRequest when include_content is disabled, otherwise just content

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I'm thinking of UserPromptPart.

anyway, forget analogies. if there's one part, don't change behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if there's just one TextPart then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do this, if it is ThinkingPart, even if it is just one then we add kind. If it is just one TextPart then we don't and let it be derived that it's just text response. If there are multiple then we club these together like we are doing currently in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but it should either be a string or a list of one or more dicts, not a single dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single dict not even for a single ThinkingPart? What would be the ideal way to represent a single ThinkingPart?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think that should actually happen. but yes, even then, a list with a single dict. same for excluded content with a single text part.

@adtyavrdhn adtyavrdhn marked this pull request as ready for review July 17, 2025 16:58
@adtyavrdhn
Copy link
Contributor Author

@alexmojaki

I've clubbed the events together keeping only ToolCallPart separate.

@adtyavrdhn adtyavrdhn requested a review from alexmojaki July 18, 2025 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LLM thinking blocks displayed in Logfire message view
2 participants