-
Couldn't load subscription status.
- Fork 14
AAP-54071: Fix raw tool_call in the message responses #108
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
base: main
Are you sure you want to change the base?
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.
I'm not an expert in the chatbot but this approach seems reasonable to me
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.
Hey @de1987 ,
thanks, looking good, although let me share a few comments:
-
I use to wrap code blocks, as commented below, and preferable use of MarkDown syntax in general.
-
Which models have you tested? We need to be sure that it works with Gemini and Granite, if possible, and we don't care about OpenAI at this point
-
The files you changed, in fact, are not being used on the deployments in our clusters. Those files you changed are only applicable when running locally the chatbot. So it is fine doing this change, but if we want this change applied to our deployed chatbots, you'll have to also update our -ops repo, such as this PR does
-
As a final comment, we have an evaluation tool for the chatbot, that evaluates queries around AAP. I tihnk it should be good running it, once changing the sys. prompt, just to be sure we're not breaking anything
ansible-chatbot-deploy.yaml
Outdated
| For single tool needed: Reply with <tool_call> followed by one-item JSON list containing the tool | ||
| Multiple tools example: | ||
| Input: "How do I configure AAP authentication?" | ||
| Response: <tool_call>[{"name": "knowledge_search", "arguments": {"query": "AAP authentication configuration"}}, {"name": "knowledge_search", "arguments": {"query": "AAP LDAP setup"}}]</tool_call> |
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 think it's better to wrap those kind of JSON blocks by using code block (an example here), so the LLM knows exactly when the block starts and end.
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'm using Granite. I addressed all your suggestions and run the evaluation tool:
...
Processing query 84: What is subscription usage?...
✅ Success for query 84
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.
@romartin PR to deploy in our clusters https://github.com/ansible/ansible-wisdom-ops/pull/1596
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.
Although I am not sure how much efforts we want to put on Granite LLM specific changes, I think the changes are clear and looks good to me.
9847227 to
93f51cd
Compare
93f51cd to
d01dec5
Compare
Jira Issue: AAP-54071
Description
As reported, there were some prompts that the responses contains raw
tool_call.By adjusting the system prompt instructions the issue was fixed for Granite models.
Testing
Steps to test
make buildmake runScenarios tested
Verified with at least 20 prompts asking questions about AAP, create Ansible playbooks...
Production deployment