Allow to set reasoning_effort and capture reasoning content#1447
Allow to set reasoning_effort and capture reasoning content#1447nforro wants to merge 3 commits into
reasoning_effort and capture reasoning content#1447Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for model reasoning content by adding a MessageReasoningContent type and updating AssistantMessage to include reasoning blocks. It also adds a reasoning_effort parameter to chat model options and enhances the LiteLLM adapter to handle reasoning and Anthropic-specific thinking_blocks. Review feedback identifies a logic error in the LiteLLM adapter where metadata could be lost if content parts are empty, suggests making text and tool calls non-exclusive, and recommends adding missing imports for improved type safety.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for model reasoning by adding MessageReasoningContent to the message framework and a reasoning_effort option to the chat model. It also updates the LiteLLM adapter to handle reasoning content and thinking_blocks for conversation history. A potential issue was identified in the LiteLLM adapter where the use of model_dump() might filter out necessary metadata like thinking_blocks if they are not explicitly defined as model fields; simplifying the condition to check for the existence of the update object is recommended.
|
This would partly address the #1448, but we would still lose the content if a tool_call was present in the delta. Could you integrate relevant parts of main...jpodivin:beeai-framework:fix/reasoning-content-drop into this? |
I assume that issue has been resolved when addressing this feedback from gemini-code-assist:
Let me at least include the tests. |
787ab80 to
b5ef17d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for model reasoning and 'thinking blocks' by adding MessageReasoningContent, updating AssistantMessage to handle reasoning parts, and including reasoning_effort parameters. The LiteLLM adapter is updated to extract reasoning content and manage signed thinking blocks for Anthropic models. Feedback suggests adding a type check for thinking_blocks in the LiteLLM adapter to prevent potential runtime errors during iteration.
Tomas2D
left a comment
There was a problem hiding this comment.
Great improvement. I am just worried whether LiteLLM handles the parameter for providers where thinkink_blocks attribute is not supported. Did you test it? If not I can do that.
Good point, I didn't think of that. I didn't test it. |
Signed-off-by: Nikola Forró <nforro@redhat.com> Assisted-by: Claude Opus 4.6 via Claude Code
…tions Signed-off-by: Nikola Forró <nforro@redhat.com> Assisted-by: Claude Opus 4.6 via Claude Code
Signed-off-by: Jiri Podivin <jpodivin@redhat.com> Co-authored-by: Nikola Forró <nforro@redhat.com>
Perhaps we could implement something in littellm. |
|
Sorry, I'm not following, implement what in LiteLLM? I think I misunderstood the comment - are we talking about setting |
Description
For models with thinking enabled, LiteLLM exposes the thoughts as
reasoning_content. This PR captures it withinAssistantMessage. To be able to enable/control model thinking,ChatModelOptionsis extended withreasoning_effort.This can be useful especially when not using/forcing the
Thinktool.Which issue(s) does this pull-request address?
Closes: #1448
Checklist
General
/ TypeScript
Pythonfor Python changes,TypeScriptfor TypeScript changesCode quality checks
mise check(mise fixto auto-fix)Testing
mise test:unitmise test:e2eDocumentation
mise docs:fix