weiqingy commented on code in PR #1215:
URL: https://github.com/apache/flink-agents/pull/1215#discussion_r4233486355
##########
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:
Done in e7834650. The setup now has `chat(messages, tools, modelParams,
outputSchema)`, which sends messages and tools as given. The existing `chat`
prepares the request and delegates to it. `chatStructured`, the history rebuild
and `withoutToolTraffic` are gone. Python can't overload, and a new keyword on
`chat` would reach the provider through `**kwargs` in subclasses that override
it, so the Python twin is a separate `chat_explicit(messages, tools,
output_schema=None, **kwargs)`.
I changed one thing in the shape. The request is a single user message, the
directive followed by the answer text, instead of `[assistant(answer),
user(directive)]`. On Bedrock an empty answer becomes an assistant message with
no content, and some vLLM/Ollama chat templates may refuse a conversation that
opens with an assistant turn. Does that trade-off work for you?
One thing I'm unsure about on the ReAct side. On the native path the loop
call no longer carries the schema instruction (613b2759, 7e650972), and the
conversion call now sees only the answer. So nothing tells the model which
fields to fill, and a missing field either fails to parse or gets made up.
Should we keep the schema instruction on the native path, or at least its field
list? Or do you see a better place for that hint?
##########
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:
Fixed in 445374f7. When the call carries a schema and no tools, it drops
`tool_choice`, `tool_choice_option` and `parallel_tool_calls`, at the top level
and inside `additional_kwargs`. A plain `chat` call is unchanged. Regression
tests added in Java and Python.
##########
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:
Done in d6a22a3d. A null schema still returns false. Otherwise the gate now
requires a connection and fails with a message that names `open()` and suggests
overriding the method. I noted the compatibility impact in the PR description.
--
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]
