-
Notifications
You must be signed in to change notification settings - Fork 95
[RFC] Client Agent SDK OutputParser #121
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
Conversation
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.
Thanks for this change, a couple of comments but looks good (Y)
react_output = ReActOutput.model_validate_json(response_text) | ||
except ValidationError as e: | ||
print(f"Error parsing action: {e}") | ||
return output_message |
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 the turn just terminate after this point?
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.
Yes, the turn will terminate after this point as there's no tool calls in Agent
. We can override orchestration in ReActAgent
to continue the loop and think again if tool call is not being correctly parsed until "answer" is reached.
# by default, we stop after the first turn | ||
stop = True | ||
for chunk in response: | ||
self._process_chunk(chunk) |
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 wonder if it's cleaner to only have the override as get_tool_call(chunk)
instead of a generic output parser. This way:
- It's clearer what the user is supposed to override
- We can actually simplify the logic below as:
# the default tool_call_getter just return `chunk...tool_calls`
tool_call = self.tool_call_getter.get_tool_call(chunk)
if not tool_call:
yield chunk
return
else:
# run tool
- Bonus: we can also be more functional and not overwrite chunk with the parsed tool calls.
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.
@ehhuang +100 especially the (3) bonus
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.
Some update in: #130
However, we still need to overwrite chunk with parsed tool calls, as ClientTool.run takes in a message history and expect the ToolCall detail in the last message.
# What does this PR do? - address comments in #121 ## Test Plan - see llamastack/llama-stack-apps#166 ## Sources Please link relevant resources if necessary. ## Before submitting - [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case). - [ ] Ran pre-commit to handle lint / formatting issues. - [ ] Read the [contributor guideline](https://github.com/meta-llama/llama-stack/blob/main/CONTRIBUTING.md), Pull Request section? - [ ] Updated relevant documentation. - [ ] Wrote necessary unit or integration tests.
# What does this PR do? - See discussion in #121 (comment) ## Test Plan test with llamastack/llama-stack-apps#166 ``` LLAMA_STACK_BASE_URL=http://localhost:8321 pytest -v tests/client-sdk/agents/test_agents.py::test_override_system_message_behavior --inference-model "meta-llama/Llama-3.3-70B-Instruct" ``` <img width="1697" alt="image" src="https://github.com/user-attachments/assets/c036cbf6-9fc1-4064-82af-fa1984300653" /> ## Sources Please link relevant resources if necessary. ## Before submitting - [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case). - [ ] Ran pre-commit to handle lint / formatting issues. - [ ] Read the [contributor guideline](https://github.com/meta-llama/llama-stack/blob/main/CONTRIBUTING.md), Pull Request section? - [ ] Updated relevant documentation. - [ ] Wrote necessary unit or integration tests.
What does this PR do?
Changes
✅ Bugfix ToolResponseMessage role
✅ Add ReACT default prompt + default output parser
✅ Add ReACTAgent wrapper
🚧 Remove ClientTool and simplify it as a decorator (separate PR, including llama-stack-apps)
✅ Make agent able to return structured outputs
ReActAgent
wrapper.Test Plan
see test in llamastack/llama-stack-apps#166
Sources
Please link relevant resources if necessary.
Before submitting
Pull Request section?