weiqingy commented on code in PR #1129:
URL: https://github.com/apache/flink-agents/pull/1129#discussion_r4037675717
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelConnection.java:
##########
@@ -99,6 +99,48 @@ protected String effectiveModelFor(@Nullable Map<String,
Object> modelParams) {
return modelParams == null ? null : (String) modelParams.get("model");
}
+ /**
+ * Whether this connection could apply {@code outputSchema} natively to a
request built from
+ * these tools and parameters, leaving the effective model's capability
out of the answer.
+ *
+ * <p>Feasibility, not capability: the answer covers everything this
connection's native branch
+ * requires of a request apart from the effective model, and says nothing
about whether the
+ * model the request names would honor a native schema, which is the
separate question {@link
+ * #supportsNativeStructuredOutput(String)} answers. Neither answer bounds
the other, in either
+ * direction. A POJO on a model the connection does not classify as
capable is feasible here and
+ * not capable there; a {@code RowTypeInfo} on a connection whose
capability predicate is
+ * unconditionally true is capable there and not feasible here.
+ *
+ * <p>An override must answer from the same logic its own request builder
uses to decide the
+ * native branch, so that the answer cannot drift from what the request
ends up carrying.
+ *
+ * <p>A {@code false} answer is not an error: it reports that the request
would carry no native
+ * schema, so the caller keeps the prompt-engineering fallback rather than
losing the schema.
+ *
+ * <p>The default {@code false} is safe only for a connection that
translates no schema at all.
+ * A connection whose request builder has a native branch but which leaves
this unoverridden
+ * reports every request infeasible: a caller that degrades to the
prompt-engineering fallback
+ * then silently never reaches that branch, and one that refuses an
unapplicable schema instead
+ * fails on a request the connection could in fact have applied.
+ *
+ * <p>Answers about the request rather than validating it. A null {@code
outputSchema} is an
+ * unconstrained request, a null {@code tools} is a request binding no
tools, and a null {@code
+ * modelParams} is accepted; none of the three may raise. The parameters
must be read without
+ * being consumed, so that the same map still builds the request the
answer was about.
+ *
+ * @param outputSchema the schema the request would carry, or null for an
unconstrained request
+ * @param tools the tools the request would bind, may be null or empty for
none
+ * @param modelParams the parameters the request would be built from, may
be null
+ * @return true if these inputs satisfy every condition the native branch
imposes apart from the
+ * effective model's capability
+ */
+ protected boolean canApplyNativeStructuredOutput(
Review Comment:
You're right that nothing consumes the two independently today, and the
combining happens inside each connection rather than in the framework:
`canApplyNativeStructuredOutput(...) && supportsNativeStructuredOutput(...)` at
`OpenAICompletionsConnection.java:303-304`, and the same pair in the Anthropic,
Bedrock, Gemini, Ollama, Azure and watsonx connections.
The caller that would read capability on its own is
`StructuredOutputStrategy.resolvesToNative`
(`StructuredOutputStrategy.java:65-75`), and it is not wired to anything yet.
The setup parses and stores the strategy without reading it
(`BaseChatModelSetup.java:71-74`), and it still calls the three-argument `chat`
(`:200`), so no schema reaches a connection through the framework at all on
this branch. The independence is a designed contract here rather than observed
behavior.
I would still keep them separate, and the reason that already shows up in
code is provider inheritance. `VLLMChatModelConnection` extends
`OpenAICompletionsConnection` and overrides capability only
(`VLLMChatModelConnection.java:80-83`), because the inherited allowlist rejects
served models such as `Qwen/Qwen2.5-7B-Instruct`, while it inherits feasibility
unchanged. Merge the two and `super` stops being usable, since the parent's
answer has already applied the allowlist the subclass exists to escape. vLLM
would have to restate the parent's feasibility rule locally: one line in Java,
and in Python also a reach into a module-private helper in the parent module
(`vllm_chat_model.py:77-85`).
Your point holds for the other providers though. Ollama and watsonx answer
capability with an unconditional `true` (`OllamaChatModelConnection.java:203`,
`WatsonxChatModelConnection.java:265`), so under a merged predicate their
override collapses into the feasibility test and disappears.
The contract does state that the two are independent
(`BaseChatModelConnection.java:106-112`), but not why they are separate hooks
rather than one, which is a fair gap. I will state the asymmetry at the
declaration: capability is advisory, so a policy is allowed to override it,
while feasibility bounds what the connection can encode at all.
If you would rather have one hook, the shape that keeps both behaviors is a
three-valued answer rather than a boolean, so that "cannot encode this" and
"this model probably will not honor it" stay distinguishable at a single call
site. That is more API surface than the split rather than less, which is why I
have not taken it, but it is a real option if the single call site is worth it.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelConnection.java:
##########
@@ -99,6 +99,48 @@ protected String effectiveModelFor(@Nullable Map<String,
Object> modelParams) {
return modelParams == null ? null : (String) modelParams.get("model");
}
+ /**
+ * Whether this connection could apply {@code outputSchema} natively to a
request built from
+ * these tools and parameters, leaving the effective model's capability
out of the answer.
+ *
+ * <p>Feasibility, not capability: the answer covers everything this
connection's native branch
+ * requires of a request apart from the effective model, and says nothing
about whether the
+ * model the request names would honor a native schema, which is the
separate question {@link
+ * #supportsNativeStructuredOutput(String)} answers. Neither answer bounds
the other, in either
+ * direction. A POJO on a model the connection does not classify as
capable is feasible here and
+ * not capable there; a {@code RowTypeInfo} on a connection whose
capability predicate is
+ * unconditionally true is capable there and not feasible here.
Review Comment:
Agreed that a flat `RowTypeInfo` is renderable without a runtime POJO or
Pydantic class, since `OutputSchema` already restricts the fields to basic
types, and the response still parses back into a `Row`. Nested rows are the
part that would need a decision, so leaving those unsupported initially sounds
right.
Worth doing as its own change rather than widening this one.
--
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]
