purushah commented on code in PR #1129:
URL: https://github.com/apache/flink-agents/pull/1129#discussion_r4054289843
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/StructuredOutputStrategy.java:
##########
@@ -25,8 +25,8 @@
*
* <p>This expresses <b>policy</b> only. Whether a connection <i>can</i> apply
the provider's native
* structured-output API is a separate, model-dependent <b>capability</b>
question answered by
- * {@link BaseChatModelConnection#supportsNativeStructuredOutput(String)}.
Policy and capability are
- * combined at request-build time.
+ * {@link BaseChatModelConnection#supportsNativeStructuredOutput(String)}.
{@link
+ * #resolvesToNative(boolean)} combines the two.
Review Comment:
Could we retain one TODO for #912 here noting that strategy resolution is
not wired into production yet? When it is wired, the native branches need to
honor the resolved policy without independently vetoing `NATIVE` through
another capability check. Non-blocking for this extraction, it's unfinished
#912 integration rather than something this PR changed.
```suggestion
* #resolvesToNative(boolean)} combines the two.
*
* TODO(#912): strategy resolution is not wired into production yet. Once it
is, the
* native branches must honor the resolved policy rather than vetoing NATIVE
through
* their own capability check.
```
##########
python/flink_agents/integrations/chat_models/openai/openai_chat_model.py:
##########
@@ -82,6 +82,26 @@
)
+def _native_output_model(output_schema: Any) -> type[BaseModel] | None:
Review Comment:
This is now the same function in anthropic, ollama and openai, and azure,
tongyi and watsonx have their own versions too. If it lived next to
`render_output_schema` in `types.py`, the schema-form check would have one home
when the flat `RowTypeInfo` follow-up comes. Optional, fine as a follow-up.
##########
integrations/chat-models/ollama/src/test/java/org/apache/flink/agents/integrations/chatmodels/ollama/OllamaChatModelConnectionTest.java:
##########
@@ -207,6 +217,91 @@ void buildRequestLeavesFormatUnsetForRowTypeInfo() {
assertThat(request.getFormat()).isNull();
}
+ /** A connection that records what the feasibility query answered on each
request it built. */
+ private static OllamaChatModelConnection recordingConnection(
+ AtomicReference<Boolean> answered) {
+ ResourceDescriptor desc =
+
ResourceDescriptor.Builder.newBuilder(OllamaChatModelConnection.class.getName())
+ .addInitialArgument("endpoint",
"http://localhost:11434";)
+ .build();
+ return new OllamaChatModelConnection(desc, NOOP) {
+ @Override
+ protected boolean canApplyNativeStructuredOutput(
+ Object outputSchema, List<Tool> tools, Map<String, Object>
modelParams) {
+ boolean answer =
+ super.canApplyNativeStructuredOutput(outputSchema,
tools, modelParams);
+ answered.set(answer);
+ return answer;
+ }
+ };
+ }
+
+ @Test
+ @DisplayName("The feasibility query answers exactly what the native branch
decides")
+ void feasibilityQueryAgreesWithTheNativeBranch() {
Review Comment:
Ollama's capability check is always true, so this test can't tell if someone
folds capability into the override later, both sides move together and it still
passes. The Python test gets around that with an `_IncapableConnection`
subclass that returns false for capability and checks the hook still says yes
for a Pydantic model. Worth doing the same here with a POJO.
##########
integrations/chat-models/watsonx/src/test/java/org/apache/flink/agents/integrations/chatmodels/watsonx/WatsonxChatModelConnectionTest.java:
##########
@@ -812,6 +818,120 @@ void buildPayloadOmitsResponseFormatForRowTypeInfo() {
assertThat(payload.has("response_format")).isFalse();
}
+ /** Minimal tool carrying only metadata; never invoked in these tests. */
+ private static final class SchemaOnlyTool extends Tool {
+ SchemaOnlyTool() {
+ super(new ToolMetadata("add", "Add two numbers.",
"{\"type\":\"object\"}"));
+ }
+
+ @Override
+ public ToolType getToolType() {
+ return ToolType.FUNCTION;
+ }
+
+ @Override
+ public ToolResponse call(ToolParameters parameters) {
+ throw new UnsupportedOperationException("not invoked in this
test");
+ }
+ }
+
+ /** A connection that records what the feasibility query answered on each
payload it built. */
+ private static WatsonxChatModelConnection recordingConnection(
+ AtomicReference<Boolean> answered) {
+ return new WatsonxChatModelConnection(
+ descriptor("https://us-south.ml.cloud.ibm.com";, "test-key",
"test-project"),
+ NOOP,
+ NO_ENVIRONMENT) {
+ @Override
+ protected boolean canApplyNativeStructuredOutput(
+ Object outputSchema, List<Tool> tools, Map<String, Object>
modelParams) {
+ boolean answer =
+ super.canApplyNativeStructuredOutput(outputSchema,
tools, modelParams);
+ answered.set(answer);
+ return answer;
+ }
+ };
+ }
+
+ @Test
+ @DisplayName("The feasibility query answers exactly what the native branch
decides")
+ void feasibilityQueryAgreesWithTheNativeBranch() {
Review Comment:
Same as Ollama, capability is always true here so there's no way to test
that it isn't folded into the hook. These two are the ones the "dedicated test
per connection" line in the description doesn't cover yet.
##########
python/flink_agents/integrations/chat_models/openai/openai_chat_model.py:
##########
@@ -262,6 +316,12 @@ def chat(
Model response message. When the response carries a finish reason,
it is available as ``extra_args["finish_reason"]``.
"""
+ # Snapshotted before the native branch below, so the feasibility query
is
+ # asked with the parameters as they arrived. This path strips nothing
today,
+ # and the snapshot is what keeps the query's view of them accurate if
it ever
+ # does, rather than leaving that to whoever adds the first pop.
+ raw_kwargs = dict(kwargs)
Review Comment:
Nothing gets popped before the hook on this path, so this copy is the same
map as `kwargs`. The other connections do strip keys before an overridable
hook, so their snapshots make sense as a contract for subclasses. Here it could
just pass `kwargs`. Tiny, non-blocking.
##########
python/flink_agents/integrations/chat_models/tongyi_chat_model.py:
##########
@@ -247,34 +296,29 @@ def chat(
# popped on the line above, so a kwargs lookup would yield None on
every call
# and report every model incapable.
#
- # TODO(#912): the requested strategy is not visible here, so this check
- # cannot tell an explicit NATIVE request apart from one that merely
- # resolved to native. A caller asking for NATIVE on a model this
predicate
- # rejects therefore gets an unconstrained response instead of an error.
- # Once strategy resolution is wired up, NATIVE must either bypass this
- # capability check or fail explicitly.
- if output_schema is not None and
self.supports_native_structured_output(
- model_name
- ):
- # Resolved before the conflict test, so a payload with no native
- # translation does not raise over a response_format this branch was
- # never going to write. Tested before the schema is rendered,
because a
- # caller who supplies both a schema and a response_format has a
conflict
- # to resolve whatever the schema turns out to render to, and
reporting a
- # render failure instead would describe the wrong problem. The
name is
- # read off the model class, so this needs no rendered document.
+ # The feasibility half is asked rather than restated, so a caller
asking the
+ # same question gets the answer this branch acts on. A payload with no
native
+ # translation is reported infeasible there, so it never reaches the
conflict
+ # test below and cannot raise over a response_format this branch was
never
+ # going to write.
+ if self.can_apply_native_structured_output(
Review Comment:
Small thing on the description. Tongyi and Azure use plain set membership
for capability, so a non-string model like `123` was already returning `False`
quietly before this change, only unhashable values raised. OpenAI did raise, at
its containment check, and Anthropic at `.startswith`. Might be worth softening
"four connections raised" and adding one test for the new fallback so it's
pinned. Not a real risk in practice since the setup declares `model: str`.
--
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]
