Repository navigation
Conversation
|
Hi @Laurianti, thank you for your contribution We appreciate you taking the time to submit this pull request. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you. |
dosadczuk
left a comment
dosadczuk
left a comment
There was a problem hiding this comment.
Thanks for porting this. The direction matches Python, but the branch is behind main and has conflicts. Main now has a durable resumable path that already handles maxIterations <= 0 the way Python does, so the new guard needs to skip that path. Details inline.
| List<? extends BaseAgent> subAgents = subAgents(); | ||
| if (subAgents == null || subAgents.isEmpty()) { | ||
| // A maxIterations of zero or less runs the sub-agents no times at all. | ||
| if (subAgents == null || subAgents.isEmpty() || (maxIterations != null && maxIterations <= 0)) { |
There was a problem hiding this comment.
This branch is behind main, which now has a durable resumable path. That path already checks the cap before the first pass and records end-of-agent (see oneIteration), which is what Python's loop_agent.py does for max_iterations <= 0. Returning early here skips that, and a resumable ParallelAgent parent then never records its own end-of-agent. Could you rebase and guard only the non-resumable and legacy paths?
if (!invocationContext.isResumable() && maxIterations != null && maxIterations <= 0) {
return Flowable.empty();
}There was a problem hiding this comment.
Fixed in 5feaae4: rebased on main, and the guard now only covers invocations that are not resumable, which includes the legacy flow: if (!invocationContext.isResumable() && maxIterations != null && maxIterations <= 0). The durable resumable path is left to check the cap in oneIteration and record end-of-agent.
| .build(); | ||
|
|
||
| assertThat(runLoop(loopAgent, false)).isEmpty(); | ||
| assertThat(runLoop(loopAgent, true)).isEmpty(); |
There was a problem hiding this comment.
After the rebase, resumable(true) takes the durable path, so the expected result is a single end-of-agent event from the loop, not an empty list. The extra pass for 0 now only happens in the legacy plainTextContinuationAutoResume flow. Could you test all three: non-resumable (empty), legacy (empty), and createResumableInvocationContext (only the loop's end-of-agent)? ResumabilityConfig isn't deprecated on main, so the suppression is only needed on the legacy case. The "Problem" section of the description needs the same update.
There was a problem hiding this comment.
Fixed in 5feaae4: three tests, each for 0 and -1: non-resumable (empty), legacy with plainTextContinuationAutoResume (empty, with the deprecation suppression only on its helper), and createResumableInvocationContext (only the loop's end-of-agent). The Problem section now says the resumable path already behaves like Python and names the legacy and non-resumable cases.
| protected Flowable<Event> runAsyncImpl(InvocationContext invocationContext) { | ||
| List<? extends BaseAgent> subAgents = subAgents(); | ||
| if (subAgents == null || subAgents.isEmpty()) { | ||
| // A maxIterations of zero or less runs the sub-agents no times at all. |
There was a problem hiding this comment.
This repeats what the condition says. Python documents the behavior on the field, so could this sentence move to Javadoc on Builder.maxIterations? If you keep an inline comment, it would help more to say why the guard is needed: Flowable.repeat rejects a negative count, and the legacy flow only checks the cap after a pass.
There was a problem hiding this comment.
Fixed in 5feaae4: the behavior is now in the Javadoc on Builder.maxIterations, and the inline comment says why the guard is needed: Flowable.repeat rejects a negative count, and the legacy flow checks the cap only after a pass.
dosadczuk
added
waiting on reporter
…egative Matches adk-python: a max_iterations of zero or less runs the sub-agents no times at all. The resumable path already checks the cap before the first pass; the non-resumable path threw IllegalArgumentException for negative values and the legacy resumption flow ran one iteration.
b2536dc to
5feaae4
Compare
3 participants
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Ports the adk-python fix google/adk-python@afbcaff to
LoopAgent.Problem:
adk-python runs the sub-agents no times at all when
max_iterationsis zero or less. The resumable path ofLoopAgentalready does: it checks the cap before the first pass and records end-of-agent. The other two paths do not:maxIterations(0)or less with the legacy resumption flow (plainTextContinuationAutoResume) runs one full iteration, because that flow checks the cap only after a pass.maxIterations(-1)with a non-resumable invocation fails withIllegalArgumentException: times >= 0 required but it was -1fromFlowable.repeat.Solution:
For an invocation that is not resumable, which includes the legacy resumption flow,
runAsyncImplreturns an emptyFlowablewhenmaxIterationsis zero or less. The resumable path is left as it is, so it still records end-of-agent. The behavior is documented onBuilder.maxIterations, as adk-python documents it onmax_iterations. Positive and absent values are unchanged.Testing Plan
Unit Tests:
Three tests in
LoopAgentTestrun a loop withmaxIterations(0)andmaxIterations(-1): the non-resumable and the legacy resumption tests expect no events, the resumable test expects only the loop's end-of-agent event. Without the fix the non-resumable test fails withIllegalArgumentExceptionand the legacy test gets one iteration; with the guard applied to the resumable path too, the resumable test fails.mvn -pl core test: 2020 tests, 0 failures, 0 errors, 24 skipped.Manual End-to-End (E2E) Tests:
Not needed: the change is a guard at the start of
runAsyncImpl.Checklist