weiqingy commented on code in PR #843:
URL: https://github.com/apache/flink-agents/pull/843#discussion_r3647346898
##########
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:
Good question, and yes, nothing calls the four-argument `chat` in this PR.
That is the scope here: the mechanism, with no framework path passing a
schema yet. The setup-level overload is the point where the policy on the setup
meets the connection-side capability, and where that resolution should live is
still open. #912 lists it as an open question: "Where should `auto` resolve
policy into a concrete strategy, given capability is a connection-side
predicate over the effective model at request-build time?"
#912 is also the consumer. It issues a dedicated structured call at
`ChatModelAction`'s no-tool-calls branch, so it is the change that both needs
the setup-level entry point and can actually exercise it. Adding the overload
here would settle that open question with nothing able to validate the choice,
and it has to move together with normalizing effective-model resolution across
Java and Python.
If you would rather see the complete path in one PR, I am happy to pull it
forward here instead.
##########
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:
Agreed, fixed in `aead976`.
The default overload now throws `UnsupportedOperationException` for a
non-null `outputSchema` and still delegates to the three-argument overload for
null, so a connection that translates a schema overrides it and nothing changes
for schemaless calls.
Python needed a different shape for the same contract. `chat` is abstract
there, so there is no inherited body that could refuse a schema. A guard on the
connection base raises `NotImplementedError`, and every connection without a
native translation calls it as the first statement of `chat`, before any client
access. The connection subclass walk in `test_output_schema_param_declared.py`
now asserts the rejection as well, so a connection added later is held to the
same contract.
One thing worth flagging since it is not visible from the diff: the throw
has no production caller in either language yet, so it stays dormant until the
native path lands. Java's only connection call is the three-argument one in
`BaseChatModelSetup`, and on the Python side `chat_model_action.py` calls
`chat` without a schema and applies the prompt fallback separately.
##########
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:
Good catch, fixed in `e537e4d`.
Python now normalizes an explicit null to `AUTO`, matching Java. I went that
direction rather than rejecting in both because Java cannot reject it:
`getArgument` is a plain map lookup (`ResourceDescriptor.java:93-96`), so an
absent key and an explicit null reach `fromArgument` identically, and rejecting
the one would reject the other.
Only null normalizes. An empty string, `0`, `False` and an unrecognized name
all still raise, and the case-insensitive resolution of both the lowercase
values and the Java enum names is unchanged.
I also added the Java test for an explicitly null argument. The neighbouring
strategy tests build their descriptors with `Map.of`, which rejects a null
value, so that case could not be expressed there and the behavior this parity
fix relies on was not pinned on the Java side.
--
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]
