weiqingy commented on code in PR #930:
URL: https://github.com/apache/flink-agents/pull/930#discussion_r3708239116
##########
integrations/chat-models/openai/src/main/java/org/apache/flink/agents/integrations/chatmodels/openai/AzureOpenAIChatModelConnection.java:
##########
@@ -151,84 +194,226 @@ public AzureOpenAIChatModelConnection(
this.client = clientBuilder.build();
}
+ /**
+ * Whether Azure documents json_schema strict support for {@code
effectiveModel}.
+ *
+ * <p>{@code effectiveModel} is the model backing an Azure deployment, not
the deployment name.
+ * See the allowlist above for the source of truth and for why the match
is exact. An
+ * unrecognized model reports {@code false} so it degrades to the
prompt-engineering fallback
+ * rather than failing at the provider.
+ *
+ * <p>Reads no instance state, so capability stays answerable
independently of how the
+ * connection was configured.
+ */
+ @Override
+ protected boolean supportsNativeStructuredOutput(String effectiveModel) {
+ if (effectiveModel == null || effectiveModel.isEmpty()) {
+ return false;
+ }
+ return NATIVE_STRUCTURED_OUTPUT_MODELS.contains(effectiveModel);
+ }
+
+ /**
+ * Whether the configured api-version reaches the structured-output floor.
+ *
+ * <p>Azure documents {@code 2024-08-01-preview} as the first api-version
supporting structured
+ * outputs, and whether an older version rejects {@code response_format}
or silently ignores it
+ * is not documented. The request therefore never carries {@code
response_format} below the
+ * floor, which is safe under either behavior.
+ *
+ * <p>The comparison assumes the documented api-version form, a
zero-padded {@code YYYY-MM-DD}
+ * date optionally suffixed {@code -preview}; over that form comparing the
leading date
+ * lexicographically is exact. The GA {@code v1} literal sorts above the
floor, which matches
+ * Azure documenting {@code v1} as supporting structured outputs. This is
not a validator: a
+ * value of any other shape is not classified reliably, and the service
rejects an api-version
+ * it does not recognize. The constructor rejects a null or blank
api-version, so no value of
+ * that shape reaches here.
+ */
+ private boolean apiVersionSupportsStructuredOutput() {
+ String datePrefix =
+ apiVersion.length() >
MIN_STRUCTURED_OUTPUT_API_VERSION.length()
+ ? apiVersion.substring(0,
MIN_STRUCTURED_OUTPUT_API_VERSION.length())
+ : apiVersion;
+ return datePrefix.compareTo(MIN_STRUCTURED_OUTPUT_API_VERSION) >= 0;
+ }
+
@Override
public ChatMessage chat(
List<ChatMessage> messages, List<Tool> tools, Map<String, Object>
modelParams) {
+ return doChat(messages, tools, modelParams, null);
+ }
+
+ /**
+ * Translates {@code outputSchema} into Azure's native strict {@code
response_format}
+ * json_schema when it is a POJO {@link Class}, the model backing the
deployment is one Azure
+ * documents json_schema strict support for, and the configured
api-version reaches {@code
+ * 2024-08-01-preview}. Any other combination leaves the request
unconstrained so that the
+ * prompt-engineering fallback still governs the response, rather than
failing at the provider.
+ *
+ * <p>Capability is keyed on the {@code model_of_azure_deployment} model
parameter rather than
+ * on the deployment the request targets, because a deployment name is
chosen by the user and
+ * carries no model information. Leaving that parameter unset therefore
keeps even a capable
+ * deployment on the fallback.
+ *
+ * @throws IllegalArgumentException if the schema is applied natively
while {@code
+ * additional_kwargs} also carries a {@code response_format}, since
the two would otherwise
+ * compete on the same request
+ */
+ @Override
+ public ChatMessage chat(
+ List<ChatMessage> messages,
+ List<Tool> tools,
+ Map<String, Object> modelParams,
+ Object outputSchema) {
+ return doChat(messages, tools, modelParams, outputSchema);
+ }
+
+ private ChatMessage doChat(
+ List<ChatMessage> messages,
+ List<Tool> tools,
+ Map<String, Object> modelParams,
+ Object outputSchema) {
try {
- Map<String, Object> mutableArgs =
- modelParams != null ? new HashMap<>(modelParams) : new
HashMap<>();
+ ChatCompletionCreateParams params =
+ buildRequest(messages, tools, modelParams, outputSchema);
+ return toResponse(client.chat().completions().create(params),
modelParams);
+ } catch (IllegalArgumentException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to call Azure OpenAI chat
completions API.", e);
+ }
+ }
- String azureDeployment = (String) mutableArgs.remove("model");
- if (azureDeployment == null || azureDeployment.isBlank()) {
- throw new IllegalArgumentException("model is required for
Azure OpenAI API calls");
- }
- String modelOfAzureDeployment =
- (String) mutableArgs.remove("model_of_azure_deployment");
+ // Package-private so response handling can be asserted against a
constructed completion without
+ // issuing a live API call through the final OpenAI client.
+ ChatMessage toResponse(ChatCompletion completion, Map<String, Object>
modelParams) {
+ // Read from the caller's map rather than the copy buildRequest
consumed, and read without
+ // consuming: a caller may reuse the same map across calls. The map is
assembled fresh for
+ // each call and no one retains it, so reading it once the response
has arrived yields the
+ // same value as reading it before the request was issued. Token
metrics report the model
+ // backing the deployment, which buildRequest only uses to decide
capability.
+ String modelOfAzureDeployment =
+ modelParams != null ? (String)
modelParams.get("model_of_azure_deployment") : null;
+
+ ChatMessage response =
+ OpenAIChatCompletionsUtils.convertFromOpenAIMessage(
+ completion.choices().get(0).message());
+
+ if (modelOfAzureDeployment != null
+ && !modelOfAzureDeployment.isBlank()
+ && completion.usage().isPresent()) {
+ response.getExtraArgs().put("model_name", modelOfAzureDeployment);
+ response.getExtraArgs().put("promptTokens",
completion.usage().get().promptTokens());
+ response.getExtraArgs()
+ .put("completionTokens",
completion.usage().get().completionTokens());
+ }
- ChatCompletionCreateParams.Builder builder =
- ChatCompletionCreateParams.builder()
- .model(ChatModel.of(azureDeployment))
-
.messages(OpenAIChatCompletionsUtils.convertToOpenAIMessages(messages));
+ return response;
+ }
- if (tools != null && !tools.isEmpty()) {
- builder.tools(convertTools(tools));
- }
+ // Package-private so the request body (including the native
response_format) can be asserted
+ // without issuing a live API call through the final OpenAI client.
+ ChatCompletionCreateParams buildRequest(
+ List<ChatMessage> messages,
+ List<Tool> tools,
+ Map<String, Object> rawModelParams,
+ Object outputSchema) {
+ Map<String, Object> mutableArgs =
+ rawModelParams != null ? new HashMap<>(rawModelParams) : new
HashMap<>();
+
+ String azureDeployment = (String) mutableArgs.remove("model");
+ if (azureDeployment == null || azureDeployment.isBlank()) {
+ throw new IllegalArgumentException("model is required for Azure
OpenAI API calls");
+ }
+ String modelOfAzureDeployment = (String)
mutableArgs.remove("model_of_azure_deployment");
- Object temperature = mutableArgs.remove("temperature");
- if (temperature instanceof Number) {
- builder.temperature(((Number) temperature).doubleValue());
- }
+ ChatCompletionCreateParams.Builder builder =
+ ChatCompletionCreateParams.builder()
+ .model(ChatModel.of(azureDeployment))
+
.messages(OpenAIChatCompletionsUtils.convertToOpenAIMessages(messages));
- Object maxTokens = mutableArgs.remove("max_tokens");
- if (maxTokens instanceof Number) {
- builder.maxCompletionTokens(((Number) maxTokens).longValue());
- }
+ if (tools != null && !tools.isEmpty()) {
+ builder.tools(convertTools(tools));
+ }
- Object logprobs = mutableArgs.remove("logprobs");
- if (Boolean.TRUE.equals(logprobs)) {
- builder.logprobs(true);
- }
+ // Capability belongs to the model backing the deployment, so it is
the input to the check;
+ // the deployment name is chosen by the user and carries none. Native
structured output
+ // applies only for a POJO Class schema — a RowTypeInfo (wrapped in
OutputSchema) keeps the
+ // prompt-engineering fallback, as do an incapable model and an
api-version below the floor.
+ String nativeSchemaName = null;
+ if (outputSchema instanceof Class
Review Comment:
You're right, the exception is gone. Azure previously inherited the base
4-arg `chat`, which throws on a non-null schema, and Python called
`_reject_unsupported_output_schema`. This PR removes both.
This is the same drop as #919 (r3671993321), where you agreed the fix
belongs in #912 and asked for a TODO at the capability check. That marker
shipped in e7639ed9, but this branch predates it. Added in 4d60405c for both
languages, including the two Azure-only cases: an api-version below the floor,
and `model_of_azure_deployment` unset.
I'd rather not make Azure reject on its own, though. The OpenAI connection
is merged with the opposite behavior and tests that assert it, so throwing only
here would leave the two disagreeing. Happy to pull the change forward if you
prefer, as long as it lands in both connections together.
--
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]
