weiqingy commented on code in PR #1098:
URL: https://github.com/apache/flink-agents/pull/1098#discussion_r4002323995
##########
integrations/chat-models/gemini/src/main/java/org/apache/flink/agents/integrations/chatmodels/gemini/GeminiChatModelConnection.java:
##########
@@ -283,9 +377,115 @@ private GenerateContentConfig buildConfig(
builder.tools(List.of(convertTools(tools)));
}
+ // 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.
+ // Nothing above writes either field this branch sets: the keys read
directly are
+ // temperature and max_output_tokens, and applyAdditionalKwargs
recognizes only top_k,
+ // top_p and stop_sequences, so there is no caller-supplied value to
collide with.
+ //
+ // 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, a model, or a tool-carrying request
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
+ && (tools == null || tools.isEmpty())
+ && supportsNativeStructuredOutput(modelName)) {
+ builder.responseMimeType("application/json");
+ builder.responseJsonSchema(toNativeJsonSchema((Class<?>)
outputSchema));
+ }
+
return builder.build();
}
+ // Derives the JSON Schema Gemini's responseJsonSchema field expects from
a POJO class. Gemini
+ // supports a subset of JSON Schema and ignores the keywords outside that
subset server-side
+ // without reporting which, so every setting below is chosen against that
published subset:
+ //
+ // - DRAFT_2020_12 is the draft whose keywords Gemini's supported list
names: $defs and
+ // prefixItems are listed, while the older drafts' definitions and
tuple-form items are not.
+ // - The PLAIN_JSON preset keeps generation to fields. A preset is
mandatory, and the
+ // generator's default one, FULL_DOCUMENTATION, surfaces getters as
properties of their
+ // own, named after the accessor call, e.g. "getSummary()".
+ // - MAP_VALUES_AS_ADDITIONAL_PROPERTIES gives a Map its value schema,
as an
+ // additionalProperties keyword carrying the declared value type.
Dropped from this recipe,
+ // the map instead takes the additionalProperties:false of the option
below and admits no
+ // entries at all.
+ // - FORBIDDEN_ADDITIONAL_PROPERTIES_BY_DEFAULT closes every object. No
Gemini document states
+ // that a schema omitting the keyword is closed, and under ordinary
JSON Schema semantics it
+ // is not, so without it a response may carry an undeclared key that
the ObjectMapper
+ // read-back then rejects. If the service honors the keyword the
object is closed; if it
+ // does not, the keyword is ignored like any other unsupported one,
which is where omitting
+ // it would have left us. It applies to the enclosing object and
leaves a Map's declared
+ // value schema alone.
+ // - Sorting fields before methods and applying no further comparison
leaves properties in
+ // declaration order, which keeps the emitted document stable rather
than alphabetized. It
+ // is not an ordering guarantee: Gemini's ordering knob is the
non-standard propertyOrdering
+ // keyword, which this generator never emits.
+ // - The required check marks every field required except an Optional
one. Gemini treats a
+ // field the schema does not list as required as optional and lets the
model skip it, while
+ // marking everything required would force the fields a caller
declared omissible.
+ // - The Jackson module makes the schema name properties the way Jackson
names them. The
+ // response is read back into the same class with an ObjectMapper, so
a property that
+ // @JsonProperty renames or @JsonIgnore drops has to be stated in the
schema under the name
+ // the mapper reads, or a response that satisfies the schema still
fails to deserialize. It
+ // is applied with no JacksonOption, so it contributes property
naming, visibility and
+ // descriptions only: the required set and the property order stay the
ones configured
+ // above.
+ //
+ // DEFINITION_FOR_MAIN_SCHEMA is deliberately absent. Without it a
recursive type emits
+ // {"$ref": "#"} at the recursion point, which is the form Google's own
recursion example uses.
+ // Enabling it instead produces a $defs entry referencing another $defs
entry, a shape no
+ // published Gemini example demonstrates.
+ private static ObjectNode toNativeJsonSchema(Class<?> schemaClass) {
+ SchemaGeneratorConfigBuilder configBuilder =
+ new SchemaGeneratorConfigBuilder(
+ SchemaVersion.DRAFT_2020_12,
OptionPreset.PLAIN_JSON)
+ .with(Option.MAP_VALUES_AS_ADDITIONAL_PROPERTIES)
+
.with(Option.FORBIDDEN_ADDITIONAL_PROPERTIES_BY_DEFAULT)
+ .with(new JacksonModule());
Review Comment:
Thanks. Both options are enabled in `d233bead`. The new test builds a
response from the enum values in the schema and reads it back with a plain
`ObjectMapper`. It fails if either option is dropped.
One gap is left. If an enum puts `@JsonProperty` on only some of its
constants, victools uses the Java names for the whole enum. That mix seems
rare, so I noted it in the comment instead of working around it. Does that
sound reasonable to you?
--
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]
