Prompt refactor for planner agent (ml-commons)#4822
Conversation
yyfamazon
requested review from
HenryL27,
Zhangxunmt,
akolarkunnu,
austintlee,
b4sjoo,
dhrubo-os,
jngz-es,
mingshl,
model-collapse,
pyek-bot,
rbhavna,
sam-herman,
xinyual,
ylwu-amzn and
zane-neo
as code owners
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 2ef150a.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
PR Reviewer Guide 🔍(Review updated until commit 2ef150a)Here are some key observations to aid the review process:
|
yyfamazon
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
yyfamazon
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
yyfamazon
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
yyfamazon
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
PR Code Suggestions ✨Latest suggestions up to 2ef150a Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 7c27359
Suggestions up to commit 8c98473
Suggestions up to commit 8c98473
Suggestions up to commit 6873396
Suggestions up to commit 6873396
|
yyfamazon
force-pushed
the
main
branch
2 times, most recently
from
407f605 to
30c8100
Compare
|
Persistent review updated to latest commit 30c8100 |
1 similar comment
|
Persistent review updated to latest commit 30c8100 |
| if (injectDate) { | ||
| plannerSystemPrompt = String.format("%s\n\n%s", plannerSystemPrompt, currentDateTime); | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
| stringBuilder.append(plannerSystemPromptPrefix).append("\n"); |
There was a problem hiding this comment.
plannerSystemPromptPrefix can be null here (since params.get() returns null when the key is absent). StringBuilder.append(null) will append the literal string "null" to the prompt. Should use params.getOrDefault(PLANNER_SYSTEM_PROMPT_PREFIX, DEFAULT_PLANNER_SYSTEM_PROMPT_PREFIX) instead.
| stringBuilder.append(plannerSystemPromptPrefix).append("\n"); | ||
| stringBuilder.append(getCorePlanningInstructions()).append("\n"); | ||
| stringBuilder.append(getPlanExecuteReflectResponseFormat(resultExpendOverride, importantRulesExpend)); | ||
| String finalPlannerPrompt = stringBuilder.toString(); |
There was a problem hiding this comment.
This path is missing FINAL_RESULT_RESPONSE_INSTRUCTIONS which is present in DEFAULT_PLANNER_SYSTEM_PROMPT. When a caller does not provide clientBusinessPrompt, the assembled prompt will be missing the final-result reporting instructions, changing behavior vs. the default.
| String dateFormat = params.get(DATETIME_FORMAT_FIELD); | ||
| String currentDateTime = injectDate ? getCurrentDateTime(dateFormat) : ""; | ||
| String clientBusinessPrompt = params.get(SYSTEM_PROMPT_FIELD); | ||
| String resultExpendOverride = params.get(RESULT_EXPAND_OVERRIDE); |
There was a problem hiding this comment.
Nit: the variable names use "Expend" (to use up) but the intent is "Expand" (to extend). The field constant names (RESULT_EXPAND_OVERRIDE, IMPORTANT_RULES_EXPAND) are correct, but the local variables and method parameters are inconsistent — e.g. resultExpendOverride should be resultExpandOverride, importantRulesExpend → importantRulesExpand.
| testParams.put(MLPlanExecuteAndReflectAgentRunner.QUESTION_FIELD, "test question"); | ||
| testParams.put(MLPlanExecuteAndReflectAgentRunner.INJECT_DATETIME_FIELD, "true"); | ||
| testParams.put(MLPlanExecuteAndReflectAgentRunner.SYSTEM_PROMPT_FIELD, ""); | ||
|
|
There was a problem hiding this comment.
This test sets INJECT_DATETIME_FIELD = "true" in the params map, but then calls setupPromptParameters(testParams, false, "") — passing injectDate=false. Since the refactored method now uses the boolean parameter (not the map value), this test no longer exercises the date injection path. To actually test it, call with injectDate=true and a non-empty currentDateTime, then assert the executor system prompt contains the datetime string.
|
Persistent review updated to latest commit 6873396 |
1 similar comment
|
Persistent review updated to latest commit 6873396 |
Signed-off-by: yyfamazon <yyf@amazon.com>
|
Persistent review updated to latest commit 8c98473 |
1 similar comment
|
Persistent review updated to latest commit 8c98473 |
|
Persistent review updated to latest commit 7c27359 |
|
Persistent review updated to latest commit 2ef150a |
2 participants
Description
Prompt refactor for planner agent (ml-commons)
Currently, there are two sets of system prompt, one in investigation and one in ml-commons. The change is ONLY keep the common part of these two sets of system prompt into ml-commons, and keep the investigation-only system prompt in investigation, then pass investigation-only system prompt to ml-common to combine with/override/expend the common system prompt.
Another change is to move the dynamic field(timeScope, currentTime, initial goal, new question) from system prompt to context to make the system prompt static.
This PR should work with opensearch-project/dashboards-investigation#309
investigation system prompt before and after diff:
re-investigation system prompt before and after diff:
this pr should work with opensearch-project/dashboards-investigation#372
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.