wenjin272 commented on code in PR #981:
URL: https://github.com/apache/flink-agents/pull/981#discussion_r3849405131
##########
integrations/chat-models/ollama/src/main/java/org/apache/flink/agents/integrations/chatmodels/ollama/OllamaChatModelConnection.java:
##########
@@ -245,6 +276,100 @@ public ChatMessage chat(
}
}
+ // Package-private so the request body (including the native format) can
be asserted without
+ // issuing a live call through the Ollama endpoint caller.
+ OllamaChatRequest buildRequest(
+ List<ChatMessage> messages,
+ List<Tool> tools,
+ Map<String, Object> modelParams,
+ Object outputSchema) {
+ // convert think to think mode.
+ final Object think = modelParams.getOrDefault("think", true);
+ ThinkMode thinkMode = ThinkMode.ENABLED;
+ for (ThinkMode mode : ThinkMode.values()) {
+ if (mode.getValue().equals(think)) {
+ thinkMode = mode;
+ break;
+ }
+ }
+
+ final List<Tools.Tool> ollamaTools = this.convertToOllamaTools(tools);
+ final List<OllamaChatMessage> ollamaChatMessages =
+ messages.stream()
+ .map(this::convertToOllamaChatMessages)
+ .collect(Collectors.toList());
+
+ final String modelName = (String) modelParams.get("model");
+ final OllamaChatRequest chatRequest =
+ OllamaChatRequest.builder()
+ .withMessages(ollamaChatMessages)
+ .withModel(modelName)
+ .withThinking(thinkMode)
+ .withUseTools(false)
+ .build();
+
+ chatRequest.setTools(ollamaTools);
+
+ // Native structured output applies only for a POJO Class schema; any
other schema form,
+ // such as a RowTypeInfo wrapped in OutputSchema, keeps the
prompt-engineering fallback.
+ // The schema is a request field of its own rather than a sampling
option, so it is set as
+ // the request's format, which is left unset when no native
translation applies and is then
+ // omitted from the serialized body rather than serialized as null.
+ //
+ // TODO(#912): the requested strategy is not visible here, so this
re-check cannot tell an
+ // explicit NATIVE request apart from one that merely resolved to
native. A caller asking
+ // for NATIVE on a schema form this branch skips therefore gets an
unconstrained response
+ // instead of an error. Once strategy resolution is wired up, NATIVE
must either bypass
+ // this capability re-check or fail explicitly.
+ if (outputSchema instanceof Class &&
supportsNativeStructuredOutput(modelName)) {
+ chatRequest.setFormat(toNativeFormat((Class<?>) outputSchema));
+ }
+
+ return chatRequest;
+ }
+
+ // Derives the JSON schema Ollama's format field expects from a POJO
class. Every setting below
+ // addresses a concrete way the generated schema otherwise fails to
constrain generation:
+ //
+ // - DRAFT_2020_12 is the draft pydantic generates on the Python side,
so a schema derived
+ // from a Java class states the same contract in the same dialect.
+ // - The PLAIN_JSON preset keeps generation to fields. Without a preset,
getters surface as
+ // properties of their own, named after the accessor call, e.g.
"getSummary()".
+ // - MAP_VALUES_AS_ADDITIONAL_PROPERTIES gives a Map its value schema.
Without it the map
+ // admits any value, and a model does emit values that the declared
value type then fails
+ // to deserialize.
+ // - Sorting fields before methods and applying no further comparison
leaves properties in
+ // declaration order. Ollama's grammar fixes generation order to the
order the schema
+ // declares its properties, so the default alphabetical order would
condition generation on
+ // an order the class does not read in.
+ // - The required check marks every field required except an Optional
one. The default marks
+ // nothing required, which lets a model omit fields at will, while
marking everything
+ // required would force the fields a caller declared omissible.
+ //
+ // Two settings are deliberately absent:
+ //
+ // - FORBIDDEN_ADDITIONAL_PROPERTIES_BY_DEFAULT gains nothing: Ollama's
grammar already
+ // refuses a key the schema does not declare, even one a prompt
explicitly asks for, and
+ // only an explicit additionalProperties: true admits one.
+ // - DEFINITION_FOR_MAIN_SCHEMA lets a recursive type generate a schema,
but when the
+ // document root is a $ref and one $defs entry references another, the
server drops the
+ // grammar and returns a free-form object. Any nested type used twice
is extracted into
+ // $defs, so enabling it would silently unconstrain a common shape to
rescue a rare one. A
+ // recursive type instead fails loudly, with HTTP 400 from the server.
+ private static ObjectNode toNativeFormat(Class<?> schemaClass) {
+ SchemaGeneratorConfigBuilder configBuilder =
+ new SchemaGeneratorConfigBuilder(
+ SchemaVersion.DRAFT_2020_12,
OptionPreset.PLAIN_JSON)
Review Comment:
**[P1] Keep the generated schema aligned with Jackson deserialization**
`OptionPreset.PLAIN_JSON` ignores Jackson's property model and annotations,
so the generated schema can diverge from the contract used to deserialize the
response. For example, `@JsonProperty("full_name") public String name` is
emitted as `name`, while Jackson expects `full_name`; a response valid against
the generated schema can therefore fail to deserialize. The same mismatch
applies to ignored and visibility-controlled properties. Could we configure
victools with its Jackson module (or otherwise use the same `ObjectMapper`
semantics) and add a regression test covering renamed and ignored properties?
--
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]
