feat: tool outputs metadata #1155
Conversation
facebook-github-bot
added
the
CLA Signed
ehhuang
force-pushed
the
pr1153
branch
5 times, most recently
from
593efac to
7beaf65
Compare
ehhuang
requested review from
ashwinb,
yanxi0830,
hardikjshah,
dltn,
raghotham,
dineshyv,
vladimirivic,
sixianyi0721 and
terrytangyuan
as code owners
| @@ -916,12 +925,12 @@ async def execute_tool_call_maybe( | |||
| messages: List[CompletionMessage], | |||
| toolgroup_args: Dict[str, Dict[str, Any]], | |||
| tool_to_group: Dict[str, str], | |||
| ) -> List[ToolResponseMessage]: | |||
| ) -> Tuple[ToolCall, ToolInvocationResult]: | |||
There was a problem hiding this comment.
not related to the PR and up for discussion:
but since we now have both client side agent loop (https://github.com/meta-llama/llama-stack-client-python/blob/ad6ffc63df658674f275267b1befc2b7046dbf33/src/llama_stack_client/lib/agents/agent.py#L179-L198) + server side agent loop.
Perhaps we should unify the behaviour/interface for the agent loop.
There was a problem hiding this comment.
Yea we'll need to make a similar change on the client side to support ClientTool outputting metadata. I've been putting off unifying since this is a relatively simple change.
llama_stack/providers/inline/agents/meta_reference/agent_instance.py
Outdated
Show resolved
Hide resolved
ehhuang
force-pushed
the
pr1153
branch
2 times, most recently
from
6a28777 to
fde4e86
Compare
| @@ -165,6 +165,7 @@ class ToolResponse(BaseModel): | |||
| call_id: str | |||
| tool_name: Union[BuiltinTool, str] | |||
| content: InterleavedContent | |||
| metadata: Optional[Dict[str, Any]] = None | |||
There was a problem hiding this comment.
Should we also add metadata to ToolResponseMessage. This is s.t. we can continue the turn with new_messages (e.g. https://github.com/meta-llama/llama-stack/pull/1178/files)
There was a problem hiding this comment.
metadata is meant as auxiliary data for the user, so I don't think we need to pass it along to continue the conversation?
There was a problem hiding this comment.
we need consistent metadata whether the tool is executed server side or client side. So, it will be good to include it in ToolResponseMessage.
There was a problem hiding this comment.
@raghotham I don't think this is related to server/client side. ToolMessage is only used in the output, i.e. ToolExecutionStep, where adding the metadata makes sense as it is extra information about the execution, akin to log.info. ToolResponseMessage are inputs to inference, which doesn't make use of the added metadata.
There was a problem hiding this comment.
My understanding is that the ClientTool will output a ToolResponseMessage. This ToolResponseMessage can be sent back to resume the Turn.
Having metadata in ToolResponseMessage is useful for:
(1) Support ClientTool outputting metadata
(1) Easy to allow metadata info to be tracked/logged on server when we resume_turn by passing back the ToolResponseMessage.
There was a problem hiding this comment.
Ah! Yea I was thinking that we should change ClientTool to output ToolInvocationResult instead, same as tool_call on the server side (this PR's change). Then we don't need metadata in ToolResponseMessage.
This PR doesn't support ClientTool metadata anyway (which will be made easier to support with your resume change). We can punt on this until the PR to support ClientTool ouputting metadata?
Summary: Allows tools to output metadata. This is useful for evaluating tool outputs, e.g. RAG tool will output document IDs, which can be used to score recall. Will need to make a similar change on the client side to support ClientTool outputting metadata. Test Plan: LLAMA_STACK_CONFIG=fireworks pytest -s -v tests/client-sdk/agents/test_agents.py
Summary:
Allows tools to output metadata. This is useful for evaluating tool outputs, e.g. RAG tool will output document IDs, which can be used to score recall.
Will need to make a similar change on the client side to support ClientTool outputting metadata.
Test Plan:
LLAMA_STACK_CONFIG=fireworks pytest -s -v tests/client-sdk/agents/test_agents.py