weiqingy commented on PR #930: URL: https://github.com/apache/flink-agents/pull/930#issuecomment-5139830975
## Spec This is the second artifact for the #894 experiment, alongside the [Implementation Description](https://github.com/apache/flink-agents/pull/930#issuecomment-5100699771). The description says what the code does. This says what it was supposed to do, written before any of it was written. One caveat that matters more than the document. This is being posted after the code, so you have no way to check that it was not reshaped to fit what shipped. It was not, but you cannot verify that, and I would rather name the hole than let it sit. From the next PR in this stack I will post the spec as a comment before pushing any code, which is the only version of this that actually tests anything. Read this one for shape, not as a pre-registration. The last section is the part I think is worth your time. A spec that matches the code proves nothing. Where it was wrong is where the review value is. ### Problem The Azure OpenAI connections reject a non-null output schema, so every caller with a schema stays on the prompt-engineering fallback even when the deployment's backing model supports native structured output. Azure OpenAI exposes the same `response_format` json-schema mechanism the OpenAI connection uses, so this is a gap in our support, not a provider limitation. ### Proposed solution Mirror the OpenAI native path in both Azure OpenAI connections: declare a capability predicate, translate a POJO/`BaseModel` schema into `response_format`, drop the rejection guard. Two deviations from the OpenAI connection, both forced by the platform. Capability resolves against `model_of_azure_deployment`, not `model`. On Azure `model` is the deployment name, an arbitrary string the user chose, so matching an allowlist against it is wrong in both directions. A second conjunct gates on the configured `api_version` against Azure's documented `2024-08-01-preview` floor. Whether an older version rejects `response_format` or silently ignores it could not be determined without a live Azure resource, and never sending it below the floor is safe under either behavior. ### API surface No new public type in either language. No config field added, removed, or made required. No signature change to an existing method. | Language | Symbol | Change | |---|---|---| | Python | `AzureOpenAIChatModelConnection.chat` | `output_schema` accepted instead of rejected | | Python | `AzureOpenAIChatModelConnection.supports_native_structured_output` | override added, was inherited `False` | | Java | `AzureOpenAIChatModelConnection.chat` | 4-arg overload override added, 3-arg behavior unchanged | | Java | `AzureOpenAIChatModelConnection.supportsNativeStructuredOutput` | override added, was inherited `false` | ### Behavior | Input | Result | |---|---| | capable model, POJO/`BaseModel` schema, api-version at or above the floor | native `response_format` applied | | `RowTypeInfo` schema | not applied, prompt fallback | | no output schema | not applied, unchanged behavior for every existing caller | | `model_of_azure_deployment` absent or not in the allowlist | not applied, prompt fallback | | `model_of_azure_deployment` is a bare `gpt-4o` | not applied, the build version is unknowable on Azure | | api-version below the floor or absent | not applied, prompt fallback | | every native condition met and the caller also supplied `response_format` | raises, naming the schema and the conflicting parameter | | caller supplies `response_format` on any non-native path | passed through unchanged | ### Files expected to change Two connection sources, two test files, plus the cross-connection walk test if the sibling OpenAI PR had not merged first. ### Out of scope Azure AI Inference native output, verified impossible at the pinned SDK and recorded on #280. Ollama, whose capability is server-version-gated rather than model-gated and needs its own design. Threading a schema down from an action or an agent, which the foundation deliberately left out. `RowTypeInfo` native support. Pre-validating a schema against Azure's structured-output limits, which fail loudly at the provider and are inherited from the OpenAI connection rather than introduced here. ### What changed during implementation Four divergences, all verified against the shipped code. **A file the spec did not list.** `AzureOpenAIChatModelSetup.java` needed a javadoc change, because `model_of_azure_deployment` picked up a second job: it used to label token-usage metrics, and it now also decides whether a request can carry a schema. A config field quietly acquiring a second meaning is a reviewer-relevant fact and the spec missed it entirely. **The Java split grew a fourth member.** The spec called for `chat` / `doChat` / `buildRequest`. The shipped code also has a package-private `toResponse` (`AzureOpenAIChatModelConnection.java:289`). The split turns one read of `model_of_azure_deployment` into two that must agree, and there was no way to observe the second one without that seam. **`toNativeResponseFormat` is absent from the spec.** The spec said "apply via `responseFormat(Class, JsonSchemaLocalValidation)`" as though it were a call. It is not. The SDK's Kotlin facade is not callable from Java, so the shipped code (`AzureOpenAIChatModelConnection.java:403-417`) builds a throwaway params object with a dummy model and a dummy user message, extracts the response format, and reattaches it. It carries an `IllegalStateException` guard that no test exercises. That is the piece of this PR I would most want a second opinion on. **The Java and Python refusal paths diverge.** Neither the spec nor the plan mentions provider refusals at all. That behavior lives one call away from this diff, in a file the diff does not touch, so a document scoped to the change could not see it. Found while writing the Implementation Description, filed as #936. The first three share a shape: a plan-shaped document specifies the mechanism and under-specifies the seam the code needs to be testable. That is the honest limit of a pre-written spec, and it is worth weighing against the limit on the other side, which is that an as-built description can only describe what the author already noticed. ### What would be useful to hear back Not which document is better in the abstract, since that invites a taste answer. Two narrower things. Which one did you open first, and did you end up going back to the other. And did either one let you skip opening a file you would otherwise have read. The second is the only measure of review effort here that is not self-reported. Worth saying out loud: this PR now carries three documents for a 1,232-line change. If the answer is that the pair costs more attention than it saves, that is a real result and I would rather record it than talk around it. -- 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]
