[Agentic Search] Fix model id parsing for QueryPlanningTool #4458
Conversation
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
owaiskazi19
requested review from
HenryL27,
Zhangxunmt,
austintlee,
b4sjoo,
dhrubo-os,
jngz-es,
mingshl,
model-collapse,
pyek-bot,
rbhavna,
sam-herman,
xinyual,
ylwu-amzn and
zane-neo
as code owners
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Error
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Error
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Error
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Error
owaiskazi19
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
| // Use agent's Agent model_id if tool doesn't have its own model_id | ||
| if (!params.containsKey(MODEL_ID_FIELD) && params.containsKey(AGENT_LLM_MODEL_ID)) { | ||
| params.put(MODEL_ID_FIELD, params.get(AGENT_LLM_MODEL_ID)); | ||
| } |
There was a problem hiding this comment.
When creating the Query Planner tool, during the registration of the agent, we are adding the agent's LLM Model ID as the model ID for QPT. But I believe this approach is a much cleaner way of doing things.
Can we also clean up this code in register Agent?
There was a problem hiding this comment.
@rithin-pullela-aws earlier, you had a PR that check if it's query planning tool then you passed over the model id to query planning tool and that's specific for queryPlanningTool.I wanted something more generic to tools and I like this PR's approach to handle model id for all tool execution.
Can you try find out that PR and then we can revert that commit and take this one.
There was a problem hiding this comment.
This is the PR but it has some other changes as well. I will leave that to @rithin-pullela-aws
|
@owaiskazi19 are these changes for 3.4? |
Yes, any version aware settings I need to look into? |
Looks fine to me |
5 participants
Description
During tool creation, only mlAgent.getParameters() was being passed to tools but QPT has a special case since it uses the same model as the agent llm. Fixed this by parsing agent llm in the params and checked on QPT side if there's no model_id in the params then use the agent llm as model id.
Related Issues
Resolves #4424
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.