Skip to content

Support Thinking part #1142

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

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from
Draft

Support Thinking part #1142

wants to merge 37 commits into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Mar 16, 2025

@Kludex Kludex marked this pull request as draft March 16, 2025 13:41
Copy link

github-actions bot commented Mar 16, 2025

Docs Preview

commit: 19c275e
Preview URL: https://66df8787-pydantic-ai-previews.pydantic.workers.dev

@Kludex Kludex force-pushed the support-thinking branch from 800710b to edffe70 Compare March 22, 2025 10:36
@Kludex
Copy link
Member Author

Kludex commented Mar 27, 2025

I would like to support OpenAI responses API before working on this PR.

@sam-cobo
Copy link

Any progress on this or any release plan?

@Kludex
Copy link
Member Author

Kludex commented Apr 20, 2025

I'll release this in some days.

@Wh1isper
Copy link
Contributor

Very nice PR, I was wondering if we could split it into two parts and release the ThinkingPart first so developers can implement it for their own models first.

For example, deepseek-r1 uses reasoning_content to show the full content. this doesn't seem to be in the current primary support.

https://api-docs.deepseek.com/api/create-chat-completion
image

@Kludex Kludex marked this pull request as ready for review April 22, 2025 13:52
@Kludex
Copy link
Member Author

Kludex commented Apr 22, 2025

Why GitHub keeps breaking this PR?

@Kludex Kludex closed this Apr 22, 2025
@Kludex Kludex reopened this Apr 22, 2025
@jerry-reevo
Copy link
Contributor

jerry-reevo commented Apr 24, 2025

I know this is work in progress, but I tested the current PR branch using streaming with Claude 3.7 Extended thinking and it generally works, but I did run into this one strange error.

Hope this is helpful. Excited for this PR to get merged!

This occurred after successfully streaming a full phase of reasoning tokens, followed by a tool call. This occurred on the first part following the tool call.

...site-packages/pydantic_ai/models/anthropic.py", line 346, in _map_message
    assert response_part.signature is not None, 'Thinking part must have a signature'
           │             └ None
           └ ThinkingPart(content=None, signature=None, part_kind='thinking')

AssertionError: Thinking part must have a signature

@mwildehahn
Copy link

I was testing this and noticed that when making requests, it will sometimes include this message to the responses API (i think from a tool call?):

{'id': '', 'summary': [{'text': '', 'type': 'summary_text'}], 'type': 'reasoning'}

which throws an error:

openai.BadRequestError: Error code: 400 - {'error': {'message': "Invalid 'input[7].id': empty string. Expected a string with minimum length 1, but got an empty string instead.", 'type': 'invalid_request_error', 'param': 'input[7].id', 'code': 'empty_string'}}

@mwildehahn
Copy link

it seems like here:

                if thinking_parts:
                    openai_messages.append(
                        responses.ResponseReasoningItemParam(
                            id=thinking_parts[0].signature or '',
                            summary=[Summary(text=item.content, type='summary_text') for item in thinking_parts],
                            type='reasoning',
                        )
                    )

we should not include if it has an empty id?

@mwildehahn
Copy link

Some of this was fixed with: openai/openai-python#2311 but now seeing:

  • UserWarning: Handling of this event type is not yet implemented. Please report on our GitHub: ResponseReasoningSummaryTextDeltaEvent(delta='...', item_id='rs_680b0e42dd748191baf3259afcab72b40792f36bcb0feba3', output_index=0, summary_index=2, type='response.reasoning_summary_text.delta')
  • UserWarning: Handling of this event type is not yet implemented. Please report on our GitHub: ResponseReasoningSummaryPartDoneEvent(item_id='rs_680b0e42dd748191baf3259afcab72b40792f36bcb0feba3', output_index=0, part=Part(text='...', type='summary_text'), summary_index=2, type='response.reasoning_summary_part.done')

@mwildehahn
Copy link

a couple things i found:

  • the response tool call from openai is ResponseFunctionToolCall(arguments='', call_id='call_IstYYmboPGwj3IZpQBm1KOUr', name='...', type='function_call', id='fc_680b21b9319c8191b215aa9e4a73538004237e9105833371', status='in_progress')

specifically, they have a:

  • call_id
  • id

Right now both ToolCallPart and ToolReturnPart just have a single tool_call_id where we need to pass back both id and call_id.

I added an id property to ToolCallPart and ToolReturnPart so that I could capture both id and call_id and it works.

I'm not sure if that should just be called vendor_id or what -- I'll open a PR against this branch to show you the changes.

@mwildehahn
Copy link

Changes I had to get this to work with openai: #1587

@Kludex
Copy link
Member Author

Kludex commented Apr 25, 2025

I think signature should only be available for Anthropic. I think it's confusing to convert id to signature.

Copy link
Contributor

hyperlint-ai bot commented Apr 25, 2025

PR Change Summary

Introduced support for the 'Thinking' feature, enhancing the model's reasoning capabilities.

  • Added a new section on 'Thinking' to the documentation
  • Closed multiple related issues regarding the 'Thinking' feature

Added Files

  • docs/thinking.md

How can I customize these reviews?

Check out the Hyperlint AI Reviewer docs for more information on how to customize the review.

If you just want to ignore it on this PR, you can add the hyperlint-ignore label to the PR. Future changes won't trigger a Hyperlint review.

Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add hyperlint-ignore to the PR to ignore the link check for this PR.

@@ -419,6 +426,9 @@ async def _map_messages(
for item in m.parts:
if isinstance(item, TextPart):
content.append({'text': item.content})
elif isinstance(item, ThinkingPart):
# NOTE: We don't pass the thinking part to Bedrock since it raises an error.

Choose a reason for hiding this comment

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

Does this mean that models won't be able to see their previous thinking if we try to pass previous messages in?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need to use the <think> tags here.

@DouweM DouweM marked this pull request as draft April 30, 2025 20:56
@aristideubertas
Copy link

@Kludex is this PR ready to be merged or is more work to be done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants