- Notifications
You must be signed in to change notification settings - Fork1k
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 ourterms of service andprivacy 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?
Conversation
| ifsettings.include_content: | ||
| body['content']=part.content | ||
| body.setdefault('content', []).append( | ||
| {'kind':'thinking'ifisinstance(part,ThinkingPart)else'text','text':part.content} |
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 am not too pleased with : 'kind': 'text', 'text': part.content
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.
only if there's multiple parts. it's the same in ModelRequests, right?
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.
No, In ModelRequests each part again gets its own Event
pydantic-ai/tests/models/test_instrumented.py
Lines 587 to 623 in490c3b4
| '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: |
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.
We display {'kind' : 'text'} in ModelRequest when include_content is disabled, otherwise just content
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.
ah, I'm thinking of UserPromptPart.
anyway, forget analogies. if there's one part, don't change behaviour.
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.
ok, if there's just one TextPart then
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.
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.
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.
right, but it should either be a string or a list of one or more dicts, not a single dict.
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 single dict not even for a single ThinkingPart? What would be the ideal way to represent a single ThinkingPart?
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 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.
I've clubbed the events together keeping only ToolCallPart separate. |
Uh oh!
There was an error while loading.Please reload this page.
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