xintongsong commented on PR #548: URL: https://github.com/apache/flink-agents/pull/548#issuecomment-4053617193
> 1. I kept both in plan. They only depend on plan/api types (ResourceProvider, PythonResourceAdapter, etc.) with zero runtime imports. ResourceCache is also used by 6 plan test files — moving it would require either circular test dependencies or relocating tests that fundamentally test AgentPlan behavior. Being consumed by runtime doesn't mean it should be owned by runtime. I tend to disagree. I think the key question is which module does the responsibility of the class conceptually belongs to, not the dependency. - The responsibility of the plan module is to provide a common representation of agents, which are programmed with potentially different sets of apis, so that they can be executed with a unified runtime. In other words, an agent plan is a translation of user program, which determines the behavior of the agent. - On the other hand, the responsibility of the runtime module is to actually execute the agent, performing the behaviors described by the given plan. From that perspective, I think both `ResourceProvider` and `PythonResourceAdapter` belongs to "how to execute the agent", thus should be moved to `runtime`. As for the testing dependencies, first of all, it should be the production codes that affect/decide the testing codes, not the other way around. I briefly check the codes that referenced `ResourceCache` in the plan module, and find that they either should also be move to `runtime` (e.g., `PythonMCPResourceDiscovery` is only used by `ActionExecutionOperator`, `AgentPlanTest` should be `ResourceCacheTest` after decoupling `ResourceCache` from the original `AgentPlan`), or should not call `ResourceCache` at all (`AgentPlanDeclareToolFieldTest` should get the provider from the plan and call the provider directly). > 2. Relationship between PythonResourceBridge and PythonResourceAdapter? They serve different roles — PythonResourceAdapter is a general-purpose Java-Python interop interface in api, while PythonResourceBridge is a one-time MCP server discovery utility. Combining them doesn't make sense since they're at different abstraction levels. I renamed PythonResourceBridge → PythonMCPResourceDiscovery to make the distinction obvious from the name. That's much clearer. Thanks. > 3. Cross-language test stuck after retry. Fixed per @wenjin272's finding — moved resourceCache.close() to the top of ActionExecutionOperator.close(), before pythonInterpreter.close(). Cached resources may hold Python object references that need the interpreter alive during cleanup. Added a comment explaining the ordering constraint. Sounds good. -- 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]
