Conversation
|
Hi @Laurianti, Thank you for your contribution and for taking the time to submit this pull request. Our team is currently reviewing your changes and we will reach out if we need any further information. Thank you. |
|
Thanks for the port and the clear write-up. We cannot take this as-is on 1.x. adk-java 1.x does not ship behavior changes, and this changes the output of templates that work today:
None of these throw an error, so callers only notice when values stop reaching the model. adk-python made this change in 2.10.0 and listed it under behavior changes. For Java, we want this opt-in on 1.x, default in 2.0. If you need this change, could you rework the PR to put the new parsing behind a flag? Here is what we need:
NOTE: The listed changes might not be the only changes needed to make it work. If you decide to follow-up, make sure the impact of your change is fully covered. |
fd023bd to
4cb1a5e
Compare
|
Thanks for the table and the precise list. Reworked in 9928b62, still one commit:
On impact: |
…written
`InstructionUtils.injectSessionState` resolves a placeholder right after `$` or
`\` like any other and keeps the character in front: `Price: ${price}` becomes
`Price: $9.99`, and `${expression}` fails the request when `expression` is not
in the state. adk-python leaves such placeholders as written since 2.10.0
(google/adk-python@0caecd2).
`LlmAgent.Builder.preserveEscapedPlaceholders(boolean)`, also available as
`preserve_escaped_placeholders` in YAML agent configs, lets an agent opt into the
same rule. The agent's flag applies to its instruction and the root agent's flag
to the global instruction; `GlobalInstructionPlugin` takes it as a constructor
argument. The default is unchanged.
4cb1a5e to
9928b62
Compare
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Ports the adk-python templating rule of google/adk-python@0caecd2 (google/adk-python#5706) as an opt-in: adk-python made it the default in 2.10.0 and listed it under behavior changes, while adk-java 1.x keeps today's output unless an agent opts in.
Problem:
InstructionUtils.injectSessionStateresolves a placeholder right after$or\like any other and keeps the character in front, so instructions that show a template syntax cannot reach the model as written:The formatString supports interpolation via ${expression} syntax.IllegalArgumentException: Context variable not foundWorkflow syntax: ${{expression}}(expression= foo)Workflow syntax: $fooPrice: ${price}(price= 9.99)Price: $9.99A ${var?} B(varmissing)A $ B\{user_name}(user_name= Foo)\FooSolution:
LlmAgent.Builder.preserveEscapedPlaceholders(boolean), defaultfalse, lets an agent opt into the adk-python rule: a placeholder right after$or\is left as written, backslash included, so each template above reaches the model unchanged. Other placeholders, including{{key}}and optional ones, resolve as before.LlmAgent: builder method,preserveEscapedPlaceholders()accessor, andpreserve_escaped_placeholdersinLlmAgentConfigfor YAML agents.InstructionUtils: the current pattern stays the default. The newinjectSessionState(InvocationContext, String, boolean)selects the adk-python pattern; the existing two-argument method takes the flag fromcontext.agent()when it is anLlmAgent, andfalseotherwise.Instructions: the agent's flag applies to its instruction, the root agent's flag to the global instruction.GlobalInstructionPlugin: a newGlobalInstructionPlugin(String, String, boolean)constructor. The existing constructors keep today's output whatever the current agent's flag.injectSessionState, in the templating paragraph ofInstruction, on the builder method and in theGlobalInstructionPluginclass Javadoc.An
Instruction.Provideris not templated, so the flag does not affect it.Testing Plan
Unit Tests:
I have added or updated unit tests for my change.
All unit tests pass locally.
InstructionUtilsTest: the four adk-python cases with the flag on, the escaped case as\{user_name}with state set, plusPrice: ${price},A ${var?} B,\{{user_name}}, and unescaped{key},{{key}}and{key?}still resolving with the flag on. With the flag off, today's output is pinned:${expression}throws,${{expression}}renders$foo,Price: ${price}renders$9.99,A ${var?} BrendersA $ B,\{user_name}renders\Foo. The two-argument method follows an opted-inLlmAgent, anLlmAgentwith defaults, and a non-LlmAgent.InstructionsTest: agent instruction with the flag on and off; global instruction following the root agent's flag, in both directions against the sub-agent's flag; a provider instruction left untouched.GlobalInstructionPluginTest: the new constructor with the flag on and off and its name; both existing string constructors keeping today's output when the current agent opts in.LlmAgentTest: default and opted-in builder, and a run of the agent checking the instruction sent to the model.ConfigAgentUtilsTest: the YAML key set and absent.AgentToolTest: an opted-in and a default agent wrapped inAgentTool.Each production change fails at least one of these tests when reverted on its own (twelve single-change reverts checked).
mvn -pl core -am test: 2051 tests, 0 failures, 0 errors, 24 skipped.Manual End-to-End (E2E) Tests:
Covered by the
LlmAgentTestandAgentToolTestruns above, which check the system instruction a test model receives.Checklist