-
Notifications
You must be signed in to change notification settings - Fork 828
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
base: main
Are you sure you want to change the base?
Support Thinking part #1142
Conversation
Docs Preview
|
800710b
to
edffe70
Compare
I would like to support OpenAI responses API before working on this PR. |
Any progress on this or any release plan? |
I'll release this in some days. |
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 |
Why GitHub keeps breaking this PR? |
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.
|
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?):
which throws an error:
|
it seems like here:
we should not include if it has an empty id? |
Some of this was fixed with: openai/openai-python#2311 but now seeing:
|
a couple things i found:
specifically, they have a:
Right now both ToolCallPart and ToolReturnPart just have a single I added an I'm not sure if that should just be called |
Changes I had to get this to work with openai: #1587 |
I think |
PR Change SummaryIntroduced support for the 'Thinking' feature, enhancing the model's reasoning capabilities.
Added Files
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 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 |
@@ -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. |
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.
Does this mean that models won't be able to see their previous thinking if we try to pass previous messages in?
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 think we need to use the <think>
tags here.
@Kludex is this PR ready to be merged or is more work to be done? |
Checklist
id
in theThinkingPart