wenjin272 commented on code in PR #964:
URL: https://github.com/apache/flink-agents/pull/964#discussion_r3790854354
##########
plan/src/main/java/org/apache/flink/agents/plan/actions/ChatModelAction.java:
##########
@@ -67,6 +105,121 @@ public class ChatModelAction {
private static final ObjectMapper mapper = new ObjectMapper();
+ private static final class RoutingSelection {
Review Comment:
`ChatModelAction` has grown to roughly 1,000 lines and now owns several
distinct responsibilities: route resolution, concrete-model invocation/retry,
fallback orchestration, routing metadata propagation, and tool-loop state
management.
Could we extract the routing-specific pieces into a package-private
component such as `ModelRoutingResolver` / `ResolvedModelRoute`?
`RoutingSelection`, `resolveRouter`, candidate ordering, routed durable-call
IDs, and routing metadata construction form a cohesive unit. Similarly,
`chatWithRetries` together with `ChatAttemptResult` / `ChatAttemptFailed` could
become a concrete-model invoker. This would leave `ChatModelAction` responsible
primarily for event orchestration while keeping the routing implementation
easier to review and extend.
##########
plan/src/main/java/org/apache/flink/agents/plan/actions/ChatModelAction.java:
##########
@@ -503,7 +971,14 @@ private static void processToolResponse(ToolResponseEvent
event, RunnerContext c
Collections.emptyList(),
toolResponseMessages);
- chat(initialRequestId, model, messages, promptArgs, outputSchema, ctx);
+ // Tool rounds reuse the already-selected concrete model (no
re-routing); if the initial
+ // request was routed, carry its routing metadata onto the eventual
final response.
+ Map<String, Object> routingMetadata = (Map<String, Object>)
context.get(ROUTING);
Review Comment:
Do we need to carry `routingMetadata` through every tool-request context?
The concrete model used for subsequent tool rounds is already stored separately
in `MODEL`; `routingMetadata` does not participate in model selection and is
only used to attach `model_routing` to the eventual `ChatResponseEvent`.
Since its lifetime is the whole ReAct loop, could we store it once in an
initial-request context keyed by `initialRequestId`, then retrieve it only when
producing the final response? This would avoid repeatedly copying it across
tool rounds, remove the special `RoutingSelection.carried(...)` state, and keep
observability metadata out of intermediate `ChatMessage.extraArgs`. The context
could then be cleaned up when the loop completes.
--
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]
