wenjin272 commented on code in PR #1352:
URL: https://github.com/apache/flink-agents/pull/1352#discussion_r4488469438
##########
api/src/main/java/org/apache/flink/agents/api/event/ChatRequestEvent.java:
##########
@@ -54,20 +56,31 @@ public class ChatRequestEvent extends Event {
ChatMessage.class,
Map.class),
BuiltInAttribute.optional("prompt_args", Map.class),
- BuiltInAttribute.optionalUntyped("output_schema"));
+ BuiltInAttribute.optionalUntyped("output_schema"),
+
BuiltInAttribute.optionalUntyped(UserDefinedContext.ATTRIBUTE));
public ChatRequestEvent(
String model,
List<ChatMessage> messages,
@Nullable Map<String, Object> promptArgs,
- @Nullable Object outputSchema) {
+ @Nullable Object outputSchema,
+ @Nullable Object context) {
Review Comment:
Could we use `Map<String, Object> userContext` here, with `dict[str, Any] |
None` and `user_context` on the Python side? A map gives this application-data
field a clearer shape, while `userContext` distinguishes it from execution
context and context supplied to models. Please apply the naming and type
consistently across all eight request/response events, including accessors and
attribute validation.
##########
docs/content/docs/development/calling-llms/chat.md:
##########
@@ -107,6 +107,8 @@ InputEvent → request Action → ChatRequestEvent
A `ChatRequestEvent` can name a model router instead of a chat model. The
built-in action selects the model first and emits a `ModelRoutingEvent`; see
[Model Routing]({{< ref "docs/development/calling-llms/model_routing" >}}).
+Pass optional `context` (Python) or `withContext` (Java) when one Action sends
several chat requests and the response Action needs to tell them apart. The
value is returned unchanged on the `ChatResponseEvent`, including after
tool-call rounds, retries, model fallback, and a failed response. It is not
added to the prompt. See [Request context]({{< ref
"docs/development/create-agent/events#request-context" >}}).
Review Comment:
Could we describe this as carrying application-defined data from a request
to its corresponding response? Distinguishing responses is one use case, and
requests may come from one or multiple Actions. Also, callers pass data to the
constructor, rather than “pass `withContext`”. Please replace “returned
unchanged” with “propagated to the corresponding response” and link to the
existing serialization rules, since JSON boundaries do not preserve arbitrary
application types.
##########
api/src/main/java/org/apache/flink/agents/api/event/ChatRequestEvent.java:
##########
@@ -145,4 +158,20 @@ public Map<String, Object> getPromptArgs() {
Map<String, Object> args = (Map<String, Object>)
getAttr("prompt_args");
return args != null ? args : Collections.emptyMap();
}
+
+ /**
+ * Attaches opaque application data to echo unchanged on the matching
{@link ChatResponseEvent}.
+ * The value is not added to messages or prompt arguments. {@code null}
omits it.
+ */
+ public ChatRequestEvent withContext(@Nullable Object context) {
Review Comment:
The request already has a constructor accepting this data, so could we
remove `withContext()` from the four request events and pass it at construction
time? Keeping `withUserContext()` on response events makes sense: built-in
Actions can attach the request's data after calling the existing
success/failure factories, without adding overloads to every factory.
##########
api/src/main/java/org/apache/flink/agents/api/event/UserDefinedContext.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.event;
+
+import org.apache.flink.agents.api.Event;
+
+import javax.annotation.Nullable;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Optional opaque application data carried on a built-in request and echoed
on its response.
+ *
+ * <p>The value is not interpreted, not injected into prompts or tool
arguments, and omitted
+ * entirely when absent so existing events keep their serialized shape.
+ *
+ * <p>Maps and lists, including nested ones, are copied when attached. A later
change to the
+ * caller's object does not change an event that already holds the value.
Other values are stored as
+ * given.
+ */
+final class UserDefinedContext {
+
+ static final String ATTRIBUTE = "context";
+
+ private UserDefinedContext() {}
+
+ static void set(Event event, @Nullable Object context) {
+ if (context == null) {
+ event.getAttributes().remove(ATTRIBUTE);
+ } else {
+ event.setAttr(ATTRIBUTE, snapshot(context));
+ }
+ }
+
+ /**
+ * Returns a detached copy of {@code value}. Maps and lists are copied
recursively so the event
+ * does not share the caller's mutable structure.
+ */
+ static Object snapshot(Object value) {
+ if (value instanceof Map) {
+ Map<?, ?> map = (Map<?, ?>) value;
+ Map<Object, Object> copy = new LinkedHashMap<>();
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ copy.put(snapshot(entry.getKey()), snapshot(entry.getValue()));
+ }
+ return copy;
+ }
+ if (value instanceof List) {
+ List<?> list = (List<?>) value;
+ List<Object> copy = new ArrayList<>(list.size());
+ for (Object item : list) {
+ copy.add(snapshot(item));
+ }
+ return copy;
+ }
+ return value;
Review Comment:
Arrays still retain the caller's reference here. I reproduced this with a
context map containing a `String[]`: after creating request A, modifying the
array and creating request B also changed A's context. Java's `sendEvent` does
not detach it either. Could we recursively copy object and primitive arrays and
add a regression test with an array nested in the context map?
##########
docs/content/docs/development/create-agent/events.md:
##########
@@ -278,3 +278,53 @@ Event that your Actions can handle to continue the
workflow:
| Call tools | `ToolRequestEvent`, `ToolResponseEvent` | [Tool Use]({{< ref
"docs/development/calling-llms/tool_use#built-in-events-and-actions" >}}) |
| Retrieve context | `ContextRetrievalRequestEvent`,
`ContextRetrievalResponseEvent` | [Vector Stores]({{< ref
"docs/development/vector-search/vector_stores#built-in-events-and-actions" >}})
|
| Call a decision model | `DecisionRequestEvent`, `DecisionResponseEvent` |
[Decision Models]({{< ref "docs/development/decision_models#event-driven-calls"
>}}) |
+
+### Request context
+
+Chat, tool, context-retrieval, and decision request events accept an optional
`context`
+value: a map or other JSON-compatible object your Action uses to tell
responses apart.
+The built-in Action copies that value onto the matching response and does not
add it to
+the prompt, tool arguments, or vector-store query. Omitting it leaves the
response
+without a `context` attribute, which is what existing callers do.
+
+The value is copied when the event is created, including nested maps and
lists, so a
+later change to the object you passed in does not change the event. It
round-trips as
Review Comment:
Could we reference the existing “Serialization and Typed Values” section
instead of describing a separate round-trip contract here? `userContext` values
should follow the same rules as other application-defined Event attributes:
JSON transfer or replay may change their runtime types, and callers restore
application types when needed. We do not need additional normalization at event
construction. Copying mutable input and restoring types are separate concerns.
##########
docs/content/docs/development/create-agent/events.md:
##########
@@ -278,3 +278,53 @@ Event that your Actions can handle to continue the
workflow:
| Call tools | `ToolRequestEvent`, `ToolResponseEvent` | [Tool Use]({{< ref
"docs/development/calling-llms/tool_use#built-in-events-and-actions" >}}) |
| Retrieve context | `ContextRetrievalRequestEvent`,
`ContextRetrievalResponseEvent` | [Vector Stores]({{< ref
"docs/development/vector-search/vector_stores#built-in-events-and-actions" >}})
|
| Call a decision model | `DecisionRequestEvent`, `DecisionResponseEvent` |
[Decision Models]({{< ref "docs/development/decision_models#event-driven-calls"
>}}) |
+
+### Request context
+
+Chat, tool, context-retrieval, and decision request events accept an optional
`context`
+value: a map or other JSON-compatible object your Action uses to tell
responses apart.
+The built-in Action copies that value onto the matching response and does not
add it to
+the prompt, tool arguments, or vector-store query. Omitting it leaves the
response
+without a `context` attribute, which is what existing callers do.
Review Comment:
Could we remove “which is what existing callers do” and the paragraph
comparing this feature with attachments? The former describes migration history
rather than the current user contract, and the latter introduces an unrelated
mechanism into this explanation. Please also broaden the opening beyond “tell
responses apart”: carrying application-defined data is the general capability,
with response identification as an example.
--
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]
