Skip to content
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

reasoning-update-phi-2507 #1875

Open
wants to merge 8 commits into
base: refactor/agno
Choose a base branch
from
Open

Conversation

ysolanky
Copy link
Contributor

@ysolanky ysolanky commented Jan 24, 2025

Changes:

  • This update adds DeepSeek as a reasoning model

Next steps:

  • Introduce a reasoning_function parameter that allows users to pass a callable function. This function will be executed, and its output will be added as a message to the Agent. Alternatively, a new parameter can be introduced to accept a function whose result is directly added as a message to the Agent. This approach makes the Agent more flexible, enabling additional use cases beyond reasoning.
  • Improve CLI output during print_response

@ysolanky ysolanky requested a review from a team as a code owner January 24, 2025 05:39
assistant_message = Message(
role=response_message.role or "assistant",
content=response_message.content,
reasoning_content=response_message.reasoning_content
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this an official field on Message?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because then this logic can be part of the main code and we don't have to overwrite here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reasoning_content is limited to just DeepSeek provider with iddeepseek-reasoner. As the scope is very limited, I just made this change for DeepSeek class

More here: https://api-docs.deepseek.com/guides/reasoning_model

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with dirk this should be an official field on Message

Comment on lines 93 to 103
def get_deepseek_reasoning_agent(
reasoning_model: Model,
monitoring: bool = False,
) -> Optional["Agent"]: # type: ignore # noqa: F821
from agno.agent import Agent

return Agent(
model=reasoning_model,
description="You are a meticulous and thoughtful assistant that solves a problem by thinking through it step-by-step.",
monitoring=monitoring,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What makes this particular to deepseek? The description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I plan on refining the description and it does not need the extra params that we are providing to the default agent.

@@ -15,6 +15,27 @@ def get_reasoning_agent(
tools: Optional[List[Union[Toolkit, Callable, Function, Dict]]] = None,
structured_outputs: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should specify specific structured output for the reasoning agent. And not assume it should be passed on from the base agent.

Tools as well, you might want specific tools for it. Also you might have like 50 tools, now it gets sent to your reasoning agent as well.

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 intentionally did not change the logic for our current reasoning. I think that fits in another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

we should 100% specify it but sadly not all models support it, so we need to lean on the user to set structured_outputs=True

then the reasoning agent uses structured_outputs=True

agree the tools thing is a mess, but currently we want the reasoning to be able to run the same tools so it can to the work using CoT and then give it to the final agent. lets have a chat on call about this

for msg in reasoning_agent_response.messages:
if hasattr(msg, "reasoning_content"):
extracted_reasoning = msg.reasoning_content
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't there be multiple messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One reasoning_content block with the reasoning and one content block with the actual content. Since we disable streaming for reasoning, this works

Copy link
Contributor

@dirkbrnd dirkbrnd left a 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 this deals with the gaslighting? We can do that later, but I'm worried we might want to change the interface then? Should we think about how it would look?

@dirkbrnd
Copy link
Contributor

I don't think this deals with the gaslighting? We can do that later, but I'm worried we might want to change the interface then? Should we think about how it would look?

Nevermind I see that is a next step

Comment on lines 2783 to 2786
if reasoning_model.__class__.__name__ == "DeepSeek" and reasoning_model.id == "deepseek-reasoner":
from agno.reasoning.helpers import get_deepseek_reasoning

# Ensure the reasoning model and agent do not show tool calls
reasoning_agent.show_tool_calls = False
reasoning_agent.model.show_tool_calls = False # type: ignore

step_count = 1
next_action = NextAction.CONTINUE
logger.debug("==== Starting Reasoning ====")
while next_action == NextAction.CONTINUE and step_count < self.reasoning_max_steps:
step_count += 1
logger.debug(f"==== Step {step_count} ====")
try:
# Run the reasoning agent
reasoning_agent_response: RunResponse = await reasoning_agent.arun(
messages=run_messages.get_input_messages()
)
if reasoning_agent_response.content is None or reasoning_agent_response.messages is None:
logger.warning("Reasoning error. Reasoning response is empty, continuing regular session...")
break
reasoning: Optional[Message] = get_deepseek_reasoning(
Copy link
Contributor

Choose a reason for hiding this comment

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

testing this, but not particularly happy about his :/

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.

3 participants