-
Notifications
You must be signed in to change notification settings - Fork 1.2k
return proper error from agent instance to the client #493
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,7 +147,8 @@ async def create_session(self, name: str) -> str: | |
async def create_and_execute_turn( | ||
self, request: AgentTurnCreateRequest | ||
) -> AsyncGenerator: | ||
assert request.stream is True, "Non-streaming not supported" | ||
if not request.stream: | ||
raise NotImplementedError("Non-streaming not supported") | ||
|
||
session_info = await self.storage.get_session_info(request.session_id) | ||
if session_info is None: | ||
|
@@ -561,9 +562,10 @@ async def _run( | |
self.tools_dict, | ||
[message], | ||
) | ||
assert ( | ||
len(result_messages) == 1 | ||
), "Currently not supporting multiple messages" | ||
if len(result_messages) != 1: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is still an assertion failure so this change is not good |
||
raise ValueError( | ||
"Currently not supporting multiple messages" | ||
) | ||
result_message = result_messages[0] | ||
|
||
yield AgentTurnResponseStreamChunk( | ||
|
@@ -682,7 +684,9 @@ async def _retrieve_context( | |
bank_ids = [] | ||
|
||
memory = self._memory_tool_definition() | ||
assert memory is not None, "Memory tool not configured" | ||
if memory is None: | ||
raise KeyError("Memory tool not configured") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think this is right either |
||
|
||
bank_ids.extend(c.bank_id for c in memory.memory_bank_configs) | ||
|
||
if attachments: | ||
|
@@ -813,16 +817,19 @@ async def execute_tool_call_maybe( | |
# When this changes, we can drop this assert | ||
# Whether to call tools on each message and aggregate | ||
# or aggregate and call tool once, reamins to be seen. | ||
assert len(messages) == 1, "Expected single message" | ||
if len(messages) != 1: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or this |
||
raise ValueError("Expected single message") | ||
message = messages[0] | ||
|
||
tool_call = message.tool_calls[0] | ||
name = tool_call.tool_name | ||
assert isinstance(name, BuiltinTool) | ||
|
||
if not isinstance(name, BuiltinTool): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, this needs to be an assert |
||
raise TypeError(f"Tool {name} is not an instance of BuiltinTool.") | ||
name = name.value | ||
if name not in tools_dict: | ||
raise KeyError(f"Tool {name} not found in agent config.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems correct but I might call it a ValueError |
||
|
||
assert name in tools_dict, f"Tool {name} not found" | ||
tool = tools_dict[name] | ||
result_messages = await tool.run(messages) | ||
return result_messages | ||
|
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.
this is good