pltbkd commented on PR #926: URL: https://github.com/apache/flink-agents/pull/926#issuecomment-5166600871
Thanks @da-daken for proposing this. The thorough code review from @weiqingy already covers a lot — thanks for that. On top of it, I'd like to raise a few concerns of my own. 1. DurableCall vs Java's DurableCallable — not aligned The new Python DurableCall and Java's DurableCallable are not aligned: Java uses the caller-declared getId() for recovery matching, while Python never consumes any caller-supplied identity and derives it from func + args instead. This seems to conflict with the requirement that new APIs stay semantically aligned across languages (and the class names differ too). That said, this is essentially a legacy gap, and this PR actually partly closes it. From a semantic-alignment standpoint it's fine to keep the current state (including the removal of the id field) for v1; I'd just suggest we track it as a tech-debt follow-up to bring Python's durable identity back in line with Java's getId() contract — including aligning the class names. 2. Timeout — confirm reconcile impact before merge; and subagents will need this too The timeout here is a mechanism on durable_execute, but the framework has no way to actually cancel a running callable. If the callable has side effects, then after a timeout the framework records the state as failed while the side effects may still happen — leaving flink-agents state inconsistent with the actual side effects. The exception path is also long, and the exception type is lost on recovery replay (TimeoutException becomes a bare RuntimeException). I'm not asking to remove the timeout here. But before merging, I'd like to confirm the interaction with reconciler: reconciler only fires on PENDING slots, and timeout finalizes the slot to FAILED — so after a timeout, the reconcile channel looks closed. Is that the intended behavior? Also worth noting: subagents are expected to use the durableExecuteAllAsync API too. If the mechanism can't properly support side-effecting callables (cancel + post-timeout state semantics), that gap will need to be filled soon — not just for tool calls. 3. Do we really need two thread pools? The motivation in the discussion for a dedicated pool was to avoid one ToolRequestEvent exhausting the global pool and affecting other keys. But if every record carries multiple parallel tool calls (which is the typical case), the tool pool gets saturated just the same — the problem simply moves to the other pool. Meanwhile, the default thread count is based on CPU cores, a sizing heuristic meant for compute-bound work; tool calls are I/O bound, so adding another pool of the same size only increases CPU and memory pressure. I think max-parallelism (a single shared pool + a cap on how many threads one record/batch can occupy) may be more suitable. It can be a follow-up optimization. But if that's the direction, I'd suggest avoiding introducing a separate tool-call.num-async-threads config now, since it would likely need to be removed or reworked when we move to that model. 4. Config surface — potential redundancy given the above Following from point 3: if max-parallelism turns out to be the more suitable direction, tool-call.num-async-threads looks redundant — the single pool is already sized by num-async-threads. At the same time, the new tool-call.parallel boolean switch could simply be folded into parallelism = 1 (serial behavior). So the current three-option surface looks like it carries config we'd likely revisit or collapse shortly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
