Conversation
hardikjshah
left a comment
There was a problem hiding this comment.
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.
Does the turn just terminate after this point?
There was a problem hiding this comment.
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.
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.
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
ReActAgentwrapper.Test Plan
see test in llamastack/llama-stack-apps#166
Sources
Please link relevant resources if necessary.
Before submitting
Pull Request section?