weiqingy commented on code in PR #1129:
URL: https://github.com/apache/flink-agents/pull/1129#discussion_r4136680371
##########
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:
Yes, one method on the base connection makes sense to me.
`effectiveModelFor`, capability and feasibility can become provider-internal
helpers. vLLM can still override capability alone through a protected helper on
`OpenAICompletionsConnection`, so my inheritance concern goes away. None of
this has shipped, so renaming is free.
One question on NATIVE though. In the follow-up wiring, the schema is only
sent on a final call after the tool rounds. If NATIVE skips the check and lets
the builder fail, the failure shows up after every tool has already run. It
would also go through the generic retry catch (`ChatModelInvoker.java:211`), so
a failure that can never succeed gets retried. LangChain doesn't hit this
because it binds the schema on every call, so it fails on the first one.
What do you think about having the method return three answers instead of a
boolean? Something like `nativeStructuredOutputSupport(schema, tools,
modelParams)` returning `INFEASIBLE`, `FEASIBLE` or `NATIVE_RECOMMENDED`. AUTO
goes native only on `NATIVE_RECOMMENDED`, NATIVE fails up front on
`INFEASIBLE`, and PROMPT stays as is. It also means the builders keep skipping
a schema they can't apply, instead of all 13 of them starting to throw. It
needs a new name either way, since Python can't overload
`supports_native_structured_output`.
If that shape works for you, I'll make the change on this PR in place of the
two predicates.
--
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]
