alnzng commented on code in PR #938: URL: https://github.com/apache/flink-agents/pull/938#discussion_r3751465238
########## api/src/main/java/org/apache/flink/agents/api/subagent/Subagent.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.agents.api.subagent; + +import org.apache.flink.agents.api.context.RunnerContext; + +/** + * Caller-facing interface for all sub-agents (external and internal). + * + * <p>An invocation is identified by a {@code (sessionId, callId)} pair; the session groups a + * conversation across invocations. Callers do not manage ids: the short forms below leave the Review Comment: >Callers do not manage ids: the short forms below leave the missing ids to the implementation, Looks like this is not consistent with the method definition below, it still requires the session id passed in: ``` SubagentFuture submit(RunnerContext ctx, Object prompt, String sessionId) throws Exception; ``` ########## api/src/main/java/org/apache/flink/agents/api/subagent/Subagent.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.agents.api.subagent; + +import org.apache.flink.agents.api.context.RunnerContext; + +/** + * Caller-facing interface for all sub-agents (external and internal). + * + * <p>An invocation is identified by a {@code (sessionId, callId)} pair; the session groups a + * conversation across invocations. Callers do not manage ids: the short forms below leave the + * missing ids to the implementation, which assigns them (runtime setups typically through a + * deterministic id allocator, stable across failover replays) or rejects the call. + * + * <p>The full form taking the complete {@code (sessionId, callId)} identity is the + * implementation-side contract, declared by {@link SubagentSetup}; resolving a returned handle is + * {@code await}. + */ +public interface Subagent { Review Comment: In the existing framework, an Agent is the thing a user authors. A user builds it by adding actions and resources (chat models, tools, and so on), and it runs in the event driven model. So today "agent" means an authored, event driven unit. The new Subagent interface is different in nature. It declares only submit, which is a caller side capability: it is how you invoke a remote agent and get a Result back, not something a user authors. Putting these together, we now have two public types with "agent" in the name that mean quite different things: Agent (authored, event driven) and Subagent (invoked, request response), and the two have no relationship in the type graph. I worry this is confusing users to read, since it is hard to tell whether Subagent is a kind of agent or the handle used to call one. So my question is whether we should keep a public Subagent interface at all, or express submit on a caller or handle abstraction whose name reflects that it is a way to invoke rather than a kind of agent. ########## api/src/main/java/org/apache/flink/agents/api/subagent/Result.java: ########## @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.agents.api.subagent; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.Serializable; + +/** + * Outcome of a {@link Subagent} call. + * + * <p>Sub-agent implementations should capture internal failures into a {@code Result} (via {@link + * #error}) instead of throwing, so callers can inspect {@link #isSuccess()} without try/catch. + * + * <p>The failure cause is carried as a serializable {@code errorMessage} — the exception's type and + * message — rather than a live exception, so that a {@code Result} can be persisted through durable + * execution. The full stack trace is logged when the failure is captured, not persisted. + */ +public class Result implements Serializable { Review Comment: Looks like all other API names have `Subagent` as prefix, maybe we should follow similar pattern for this class - `SubagentResult`? -- 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]
