wenjin272 commented on code in PR #1215:
URL: https://github.com/apache/flink-agents/pull/1215#discussion_r4229343573
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -249,6 +250,134 @@ public ChatMessage chat(
return connection.chat(messages, tools, params);
}
+ /**
+ * Whether {@code outputSchema} should travel through the provider's
native structured output on
+ * a call issued through {@link #chatStructured(List, Map, Object)},
rather than be described to
+ * the model in the prompt.
+ *
+ * <p>Framework-facing: public because the caller lives in another
package. A user configures
+ * the outcome through the {@link StructuredOutputStrategy} instead of
calling this.
+ *
+ * <p>The connection is asked about the request {@link
#chatStructured(List, Map, Object)}
+ * sends: this schema, no tools, and the parameters {@link
#getParameters()} returns, resolved
+ * once so that the answer and the request concern the same parameters.
Per-call parameters
+ * passed to {@link #chatStructured(List, Map, Object)} are not seen here,
so a caller that adds
+ * parameters affecting feasibility must not rely on this answer.
+ *
+ * <p>A true answer is not a promise that the call succeeds: a connection
may still raise once
+ * its native branch applies the schema, for example on a conflicting
caller-supplied response
+ * format.
+ *
+ * <p>A setup with no bound connection, such as one that overrides {@link
#open()} and {@link
+ * #chat(List, Map, Map)} to answer by itself, answers false unless the
strategy is {@link
+ * StructuredOutputStrategy#NATIVE}.
+ *
+ * @param outputSchema the schema the call would carry, or null for an
unconstrained call
+ * @return true if the schema should be applied natively; false for a null
schema or when no
+ * connection is bound
+ * @throws IllegalArgumentException if the strategy is {@link
StructuredOutputStrategy#NATIVE}
+ * and no connection is bound, or the connection cannot apply this
schema to such a request
+ */
+ public boolean willApplyNativeStructuredOutput(@Nullable Object
outputSchema) {
+ if (outputSchema == null) {
+ return false;
+ }
+ if (connection == null) {
+ if (structuredOutputStrategy == StructuredOutputStrategy.NATIVE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Structured output strategy NATIVE was
requested, but %s has no"
+ + " connection to apply the output
schema natively.",
+ getClass().getName()));
+ }
+ return false;
+ }
+ NativeStructuredOutputSupport support =
+ connection.supportsNativeStructuredOutput(outputSchema,
List.of(), getParameters());
+ if (support == NativeStructuredOutputSupport.INFEASIBLE
+ && structuredOutputStrategy ==
StructuredOutputStrategy.NATIVE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Structured output strategy NATIVE was requested,
but %s cannot apply"
+ + " the output schema %s natively. Use
AUTO or PROMPT to"
+ + " describe the schema in the prompt
instead, or supply a"
+ + " schema this connection can translate.",
+ connection.getClass().getName(),
describeSchema(outputSchema)));
+ }
+ return structuredOutputStrategy.resolvesToNative(support);
+ }
+
+ private static String describeSchema(Object outputSchema) {
+ if (outputSchema instanceof Class) {
+ return ((Class<?>) outputSchema).getName();
+ }
+ if (outputSchema instanceof OutputSchema) {
+ // The wrapper's toString names no schema.
+ return String.valueOf(((OutputSchema) outputSchema).getSchema());
+ }
+ return outputSchema.getClass().getName();
+ }
+
+ /**
+ * Sends one schema-carrying request to the connection, for a caller that
has decided through
+ * {@link #willApplyNativeStructuredOutput(Object)} that the schema
travels natively.
+ *
+ * <p>Framework-facing: public because the caller lives in another
package. A user reaches a
+ * model through {@link #chat(List, Map, Map)}.
+ *
+ * <p>The messages are sent as given, without the bound prompt or the
skill-discovery message,
+ * because messages that already passed through {@link #chat(List, Map,
Map)} would otherwise
+ * carry them twice. No tools are bound, because a provider may drop a
native schema from a
+ * request that also binds tools.
+ *
+ * <p>Because no tools are bound, tool traffic is removed from what is
sent: some providers
+ * reject tool calls and tool results in a request that defines no tools.
Tool-role messages and
+ * assistant messages carrying tool calls are dropped whole, since keeping
a tool-calling turn's
+ * text would leave two assistant turns in a row. Turns still alternate
only when an assistant
+ * message without tool calls follows the tool traffic, which a caller
guarantees by appending
+ * the final answer. The caller's list and messages are not modified.
+ *
+ * @param messages the conversation to send, used as given apart from its
tool traffic
+ * @param modelParams parameters for this call, merged over {@link
#getParameters()} the same
+ * way {@link #chat(List, Map, Map)} merges them, may be null
+ * @param outputSchema the schema the call carries, must not be null
+ * @return the connection's response
+ * @throws NullPointerException if {@code outputSchema} is null, or if
{@link #open()} has not
+ * bound the connection yet
+ */
+ public ChatMessage chatStructured(
Review Comment:
Could we simplify the finalization path and keep its orchestration in the
chat action/invoker?
The finalization input should be just the last assistant response plus the
conversion directive, with the schema passed separately. That response already
has no tool calls. If the final answer itself is incomplete, format conversion
should not try to repair it using earlier context. This would remove the need
to reconstruct the full history, call `prepareRequestMessages` again, or filter
it through `withoutToolTraffic`.
Instead of a dedicated `chatStructured` method or a new request type, could
we add an overload along these lines?
```java
chat(messages, tools, modelParams, outputSchema)
```
This overload would use the supplied messages and tools without injecting
the setup's bound prompt or tools. The existing `chat(messages, promptArgs,
modelParams)` could prepare the normal request and delegate to it. The
action/invoker would call the explicit overload with the last response,
conversion directive, empty tools, and schema.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -249,6 +250,134 @@ public ChatMessage chat(
return connection.chat(messages, tools, params);
}
+ /**
+ * Whether {@code outputSchema} should travel through the provider's
native structured output on
+ * a call issued through {@link #chatStructured(List, Map, Object)},
rather than be described to
+ * the model in the prompt.
+ *
+ * <p>Framework-facing: public because the caller lives in another
package. A user configures
+ * the outcome through the {@link StructuredOutputStrategy} instead of
calling this.
+ *
+ * <p>The connection is asked about the request {@link
#chatStructured(List, Map, Object)}
+ * sends: this schema, no tools, and the parameters {@link
#getParameters()} returns, resolved
+ * once so that the answer and the request concern the same parameters.
Per-call parameters
+ * passed to {@link #chatStructured(List, Map, Object)} are not seen here,
so a caller that adds
+ * parameters affecting feasibility must not rely on this answer.
+ *
+ * <p>A true answer is not a promise that the call succeeds: a connection
may still raise once
+ * its native branch applies the schema, for example on a conflicting
caller-supplied response
+ * format.
+ *
+ * <p>A setup with no bound connection, such as one that overrides {@link
#open()} and {@link
+ * #chat(List, Map, Map)} to answer by itself, answers false unless the
strategy is {@link
+ * StructuredOutputStrategy#NATIVE}.
+ *
+ * @param outputSchema the schema the call would carry, or null for an
unconstrained call
+ * @return true if the schema should be applied natively; false for a null
schema or when no
+ * connection is bound
+ * @throws IllegalArgumentException if the strategy is {@link
StructuredOutputStrategy#NATIVE}
+ * and no connection is bound, or the connection cannot apply this
schema to such a request
+ */
+ public boolean willApplyNativeStructuredOutput(@Nullable Object
outputSchema) {
+ if (outputSchema == null) {
+ return false;
+ }
+ if (connection == null) {
Review Comment:
Could we remove the `connection == null` fallback from
`willApplyNativeStructuredOutput`?
The framework calls `open()` before exposing the setup for use, so a normal
setup should already have its connection initialized here. Treating a missing
connection as a reason to select the prompt path hides a lifecycle or
implementation error.
Special setups that intentionally operate without a connection can override
this method, as the bridge setup already does. The tests expecting automatic
fallback for a missing connection should be adjusted accordingly.
##########
python/flink_agents/api/chat_models/chat_model.py:
##########
@@ -576,19 +629,140 @@ def chat(
for msg in messages:
if len(msg.blocks) > 0 or msg.role == MessageRole.ASSISTANT:
prompt_messages.append(msg)
- messages = prompt_messages
+ prepared = prompt_messages
if self.skill_discovery_prompt:
# Right after the first system message, or at the head when there
is none.
- index = find_first_system_message(messages) + 1
+ index = find_first_system_message(prepared) + 1
injected = [ChatMessage.system(self.skill_discovery_prompt)]
- messages = list(messages[:index]) + injected +
list(messages[index:])
+ prepared = prepared[:index] + injected + prepared[index:]
+ return prepared
- # Call chat model connection to execute chat
+ def will_apply_native_structured_output(
+ self, output_schema: OutputSchema | None
+ ) -> bool:
+ """Whether ``output_schema`` should travel through the provider's
native
+ structured output on a call issued through ``chat_structured``, rather
than
+ be described to the model in the prompt.
+
+ Framework-facing. A user configures the outcome through
+ ``structured_output_strategy`` instead of calling this.
+
+ The connection is asked about the request ``chat_structured`` sends:
this
+ schema, no tools, and the parameters ``model_kwargs`` returns, read
once so
+ that the answer and the request concern the same parameters. Per-call
keyword
+ arguments passed to ``chat_structured`` are not seen here, so a caller
that
+ adds parameters affecting feasibility must not rely on this answer.
+
+ A ``True`` answer is not a promise that the call succeeds: a
connection may
+ still raise once its native branch applies the schema, for example on a
+ conflicting caller-supplied response format.
+
+ A setup with no resolved connection, such as one that overrides
``open`` and
+ ``chat`` to answer by itself, answers ``False`` unless the strategy is
+ ``NATIVE``.
+
+ Args:
+ output_schema: The schema the call would carry, or ``None`` for an
+ unconstrained call.
+
+ Returns:
+ ``True`` if the schema should be applied natively; ``False`` for a
+ ``None`` schema or when no connection is resolved.
+
+ Raises:
+ ValueError: If the strategy is ``NATIVE`` and no connection is
resolved,
+ or the connection cannot apply this schema to such a request.
+ """
+ if output_schema is None:
+ return False
+ connection = self._resolved_connection
+ if connection is None:
+ if self.structured_output_strategy ==
StructuredOutputStrategy.NATIVE:
+ setup_cls = type(self)
+ msg = (
+ f"Structured output strategy NATIVE was requested, but "
+ f"{setup_cls.__module__}.{setup_cls.__qualname__} has no "
+ "connection to apply the output schema natively."
+ )
+ raise ValueError(msg)
+ return False
+ support = connection.supports_native_structured_output(
+ output_schema, [], self.model_kwargs
+ )
+ if (
+ support == NativeStructuredOutputSupport.INFEASIBLE
+ and self.structured_output_strategy ==
StructuredOutputStrategy.NATIVE
+ ):
+ cls = type(connection)
+ msg = (
+ f"Structured output strategy NATIVE was requested, but "
+ f"{cls.__module__}.{cls.__qualname__} cannot apply the output
schema "
+ f"{_describe_output_schema(output_schema)} natively. Use AUTO
or "
+ "PROMPT to describe the schema in the prompt instead, or
supply a "
+ "schema this connection can translate."
+ )
+ raise ValueError(msg)
+ return self.structured_output_strategy.resolves_to_native(support)
+
+ def chat_structured(
+ self,
+ messages: Sequence[ChatMessage],
+ output_schema: OutputSchema,
+ **kwargs: Any,
+ ) -> ChatMessage:
+ """Send one schema-carrying request to the connection, for a caller
that has
+ decided through ``will_apply_native_structured_output`` that the schema
+ travels natively.
+
+ Framework-facing. A user reaches a model through ``chat``.
+
+ The messages are sent without the bound prompt or the skill-discovery
message,
+ because messages that already passed through ``chat`` would otherwise
carry
+ them twice. No tools are bound, because a provider may drop a native
schema
+ from a request that also binds tools. Tool traffic is removed as well,
since
+ some providers reject tool calls and tool results in a request that
defines
+ no tools: tool messages and every assistant message carrying tool
calls are
+ dropped whole, and every other message is sent as the same object. The
+ caller's list and messages are left unchanged.
+
+ User and assistant turns still alternate only when an assistant message
+ without tool calls follows the tool traffic, which a caller guarantees
by
+ including the final answer.
+
+ The caller passes messages already prepared by
``prepare_request_messages``;
+ they are not prepared again here.
+
+ Args:
+ messages: The conversation to send.
+ output_schema: The schema the call carries; must not be ``None``.
+ **kwargs: Model parameters for this call, merged over
``model_kwargs``
+ the same way ``chat`` merges them. Prompt arguments are not
accepted,
+ since no prompt is rendered: a ``prompt_args`` passed here
would
+ reach the provider as a model parameter.
+
+ Returns:
+ The connection's response.
+
+ Raises:
+ TypeError: If ``open()`` has not resolved the connection yet, or if
+ ``output_schema`` is ``None``.
+ """
+ connection = self._get_connection()
+ if output_schema is None:
+ msg = (
+ "chat_structured() requires an output schema. Call chat() for
an "
+ "unconstrained request."
+ )
+ raise TypeError(msg)
merged_kwargs = self.model_kwargs.copy()
merged_kwargs.update(kwargs)
- connection = self._get_connection()
- return connection.chat(messages, tools=self._get_tools(),
**merged_kwargs)
+ return connection.chat(
+ _without_tool_traffic(messages),
+ tools=[],
Review Comment:
The finalization call removes tools but still inherits `tool_choice`. With
vLLM and `additional_kwargs={"tool_choice": "auto"}`, this produces an invalid
request: `When using tool_choice, tools must be set.`
Could we clear the tool-specific parameters for finalization and add a
regression test?
--
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]
