wenjin272 commented on code in PR #843:
URL: https://github.com/apache/flink-agents/pull/843#discussion_r3637119047
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelConnection.java:
##########
@@ -55,4 +77,33 @@ public ResourceType getResourceType() {
*/
public abstract ChatMessage chat(
List<ChatMessage> messages, List<Tool> tools, Map<String, Object>
modelParams);
+
+ /**
+ * Process a chat request that carries an output schema, and return a chat
response.
+ *
+ * <p>{@code outputSchema} is framework-level execution metadata, kept off
{@code modelParams}
+ * so that it can never reach a provider SDK request as a generation
parameter. It is either a
+ * POJO {@link Class} or an {@link
org.apache.flink.agents.api.agents.OutputSchema} (a {@code
+ * RowTypeInfo} wrapper); the two cases are distinguished by the
connection that consumes it.
+ *
+ * <p>This default implementation <b>ignores</b> {@code outputSchema} and
delegates to {@link
+ * #chat(List, List, Map)}. Connections without a native structured-output
translation inherit
+ * it unchanged and need no edits. A connection that does translate a
schema into a native
+ * provider parameter overrides this overload, and reports its capability
via {@link
+ * #supportsNativeStructuredOutput(String)}.
+ *
+ * @param messages the input chat messages
+ * @param tools the tools can be called by the model
+ * @param modelParams the additional arguments passed to the model
+ * @param outputSchema the schema the response should conform to, or null
for an unconstrained
+ * response
+ * @return the chat response containing model outputs
+ */
+ public ChatMessage chat(
Review Comment:
It appears that the `BaseChatModelSetup` has never invoked this `chat`
interface. Since `ChatModelAction` actually calls `BaseChatModelSetup#chat`,
should we also add the corresponding interface to `BaseChatModelSetup`?
##########
python/flink_agents/api/chat_models/chat_model.py:
##########
@@ -153,6 +254,14 @@ class BaseChatModelSetup(Resource):
skill_discovery_prompt: str | None = None
allowed_commands: List[str] = Field(default_factory=list)
allowed_script_dirs: List[str] = Field(default_factory=list)
+ structured_output_strategy: StructuredOutputStrategy = Field(
Review Comment:
There seems to be a Java/Python parity issue for an explicitly configured
`structured_output_strategy: null`: Java treats it as `AUTO`, while Python
rejects `None` with a validation error. I think we should either normalize
`null` to `AUTO` in both languages or reject it consistently.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelConnection.java:
##########
@@ -55,4 +77,33 @@ public ResourceType getResourceType() {
*/
public abstract ChatMessage chat(
List<ChatMessage> messages, List<Tool> tools, Map<String, Object>
modelParams);
+
+ /**
+ * Process a chat request that carries an output schema, and return a chat
response.
+ *
+ * <p>{@code outputSchema} is framework-level execution metadata, kept off
{@code modelParams}
+ * so that it can never reach a provider SDK request as a generation
parameter. It is either a
+ * POJO {@link Class} or an {@link
org.apache.flink.agents.api.agents.OutputSchema} (a {@code
+ * RowTypeInfo} wrapper); the two cases are distinguished by the
connection that consumes it.
+ *
+ * <p>This default implementation <b>ignores</b> {@code outputSchema} and
delegates to {@link
+ * #chat(List, List, Map)}. Connections without a native structured-output
translation inherit
+ * it unchanged and need no edits. A connection that does translate a
schema into a native
+ * provider parameter overrides this overload, and reports its capability
via {@link
+ * #supportsNativeStructuredOutput(String)}.
+ *
+ * @param messages the input chat messages
+ * @param tools the tools can be called by the model
+ * @param modelParams the additional arguments passed to the model
+ * @param outputSchema the schema the response should conform to, or null
for an unconstrained
+ * response
+ * @return the chat response containing model outputs
+ */
+ public ChatMessage chat(
+ List<ChatMessage> messages,
+ List<Tool> tools,
+ Map<String, Object> modelParams,
+ @Nullable Object outputSchema) {
+ return chat(messages, tools, modelParams);
Review Comment:
When `NATIVE` is configured, `resolvesToNative(false)` still returns `true`,
so the caller may invoke the four-argument `chat()` even when the connection
does not support native structured output. With the current default
implementation, `outputSchema` is silently ignored and an unconstrained
response may be returned. I think the default implementation should throw
`UnsupportedOperationException` for a non-null `outputSchema`; supporting
connections can override it, while `null` can continue delegating to the
three-argument overload.
--
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]
