weiqingy commented on PR #930:
URL: https://github.com/apache/flink-agents/pull/930#issuecomment-5100579123
## Implementation Description
As-built description of the final code on this branch, derived by walking
`git diff origin/main...HEAD` hunk by hunk.
### 1. Runtime flow
Both languages follow the same order. The connection decides everything
locally; nothing upstream is consulted.
**Python, `AzureOpenAIChatModelConnection.chat`:**
1. Tools are converted to OpenAI tool specs, unchanged from before.
2. `model` is popped from `kwargs` as the Azure deployment name and required
to be non-empty.
3. `model_of_azure_deployment` is popped. This is the model backing the
deployment.
4. `additional_kwargs` is popped, and its keys are checked against the
reserved typed fields, unchanged from before.
5. The native gate is evaluated as three conditions in order: the schema is
not `None`, `supports_native_structured_output(model_of_azure_deployment)` is
true, and `_api_version_supports_structured_output()` is true.
6. If the gate passes, `_native_response_format(output_schema)` is called.
It returns `None` for anything that is not a `BaseModel` subclass, which is the
fourth condition in practice.
7. If it returned a format, the collision check runs: if `response_format`
is present in `kwargs` or in `additional_kwargs`, `ValueError` is raised.
Otherwise `kwargs["response_format"]` is set.
8. `client.chat.completions.create(...)` is called with the deployment name
as `model`, and the response is converted exactly as before.
**Java, `AzureOpenAIChatModelConnection`:**
1. The 3-arg `chat` now delegates to `doChat(..., null)`. The new 4-arg
`chat` delegates to `doChat(..., outputSchema)`.
2. `doChat` calls `buildRequest`, issues
`client.chat().completions().create(params)`, and passes the completion to
`toResponse`.
3. `buildRequest` copies the caller's `modelParams` into a mutable map, then
removes `model` (required non-blank) and `model_of_azure_deployment` from the
copy.
4. The native gate is evaluated as `outputSchema instanceof Class`, then
`supportsNativeStructuredOutput(modelOfAzureDeployment)`, then
`apiVersionSupportsStructuredOutput()`. If all hold,
`toNativeResponseFormat(schemaClass)` is attached to the builder and the
schema's simple name is recorded in a local `nativeSchemaName`.
5. `temperature`, `max_tokens`, and `logprobs` are read after that,
unchanged from before.
6. `additional_kwargs` is removed and checked for reserved typed fields,
unchanged. Then, only if `nativeSchemaName` is non-null, the presence of
`response_format` in `additional_kwargs` raises `IllegalArgumentException`.
Otherwise every entry is written through as an additional body property,
unchanged.
7. `toResponse` converts the first choice's message, then reads
`model_of_azure_deployment` from the caller's original map with `get` rather
than `remove`, and attaches `model_name`, `promptTokens`, and
`completionTokens` only when that value is non-blank and `completion.usage()`
is present.
The api-version value is read from a new `private final String apiVersion`
field. Before this change the constructor consumed the value into
`AzureOpenAIServiceVersion.fromString(...)` and discarded it.
### 2. Behavioral contracts
Each is a checkable claim about the final code.
**C1.** Native structured output is applied only when all four hold: the
schema is non-null, the schema translates (a `BaseModel` subclass in Python, a
`Class` in Java), the backing model is in the allowlist, and the api-version is
at or above `2024-08-01`. If any fails, the request is built exactly as it
would have been without a schema.
**C2.** Capability is decided from `model_of_azure_deployment`, never from
`model`. The request still targets `model`, the deployment name, as before.
**C3.** An absent, empty, or unrecognized `model_of_azure_deployment`
reports not-capable, so the request carries no `response_format`.
**C4.** A bare `gpt-4o` reports not-capable, because Azure documents
`gpt-4o` as supported only at versions `2024-08-06` and `2024-11-20` while
`2024-05-13` is unsupported, and a model name carries no version.
**C5.** An api-version whose leading 10 characters sort below `2024-08-01`
suppresses the native path. An empty api-version does the same in Python. In
Java a null or blank api-version cannot reach the check, because the
constructor already rejects it.
**C6.** A `RowTypeInfo` schema in Python, and any non-`Class` schema in
Java, suppresses the native path and keeps the prompt-engineering fallback.
**C7.** A null schema leaves behavior identical to before this change.
**C8.** Bound tools do not suppress the native path. A request may carry
both tools and `response_format`.
**C9.** When the native path applies and the caller also supplied
`response_format`, the call raises rather than overwriting or duplicating it.
Python checks both `kwargs` and `additional_kwargs`. Java checks
`additional_kwargs`, which is its only caller-facing channel, because
unrecognized top-level `modelParams` keys are discarded and never reach the
request.
**C10.** On every path where the native format is not applied, a
caller-supplied `response_format` reaches the provider untouched. In Python the
identical object is forwarded, not a copy.
**C11.** The capability predicate reads no instance state. It is answerable
on an instance that was never initialized, where field access would raise.
**C12.** Response handling does not consume the caller's `modelParams`. Java
reads `model_of_azure_deployment` with `get`, so a caller reusing the same map
across calls still sees it.
**C13.** Token metrics (`model_name`, `promptTokens`, `completionTokens`)
are attached only when the backing model is non-blank and the completion
carries usage. Neither alone is sufficient.
**C14.** The Java and Python allowlists and floor constant are identical,
including ordering of the allowlist entries.
**C15.** A connection that translates schemas natively is exempt from the
cross-connection walk that requires every connection to reject a schema it
cannot translate. The exemption is derived from whether
`supports_native_structured_output` is overridden, not from a name list.
### 3. Key decisions
**Capability is keyed on `model_of_azure_deployment`, not on `model`.** On
Azure, `model` is the deployment name, an arbitrary string chosen by the user.
Matching an allowlist against it is wrong in both directions: a deployment
named `prod-chat` backed by a capable model would report not-capable, and a
deployment named `gpt-4o` backed by anything would report capable. The rejected
alternative was to reuse the OpenAI connection's rule unchanged, which is
exactly that mistake. The cost of this choice is that leaving the optional
`model_of_azure_deployment` unset keeps even a capable deployment on the
fallback. That direction of error is the safe one.
**Matching is exact, never by prefix.** The sibling OpenAI connection
matches one model family by prefix. Azure reports a model name and a model
version as separate properties, so a name has no version suffix to discriminate
on, and prefix matching would classify unsupported versions as capable.
**The api-version floor is enforced in the request path, not in the
capability predicate.** The predicate's argument is a model name and carries no
api-version, and the predicate must stay free of instance state (C11). Putting
the floor check in the predicate would break that.
**The floor is enforced conservatively because the failure mode below it is
not documented.** Azure documents `2024-08-01-preview` as the first supporting
version but does not document whether an older version rejects
`response_format` or silently ignores it. Neither SDK does client-side gating,
so this could not be settled without a live Azure resource. Never sending
`response_format` below the floor is safe under either behavior: if the service
would reject, no such call is made; if it would ignore, the native path is
never taken so the prompt fallback still produces a parseable result. The
rejected alternative was to send it and let the service decide, which is unsafe
precisely in the silent-ignore case, since the caller would receive
unconstrained text believing it was schema-constrained.
**The comparison is a date-prefix comparison, not a validator.** It assumes
the documented zero-padded `YYYY-MM-DD` form, optionally suffixed `-preview`.
Over that form, comparing the leading date lexicographically is exact. The GA
`v1` literal sorts above the floor, which matches Azure documenting `v1` as
supporting structured outputs. Values of other shapes are not classified
reliably, and that is accepted rather than fixed: the service rejects an
api-version it does not recognize, so the failure is immediate and loud, and
duplicating Azure's version validation here would rot against it.
**`response_format` was deliberately not added to the reserved-key set.**
Passing `response_format` without a schema is a working path today, and the
reserved-key check would break it. That check also produces a message about
reserved typed fields settable via the Setup, which does not describe this
situation.
**The collision check is scoped inside the branch that actually applied the
schema.** Hoisting it above the translation result would make it fire on a
`RowTypeInfo` schema combined with a caller `response_format`, which is a
working path.
**Java needed a request-building seam before any of this was testable.** The
previous `chat` built the request inline with no way to observe it, and this
module's test convention is a package-private seam with a real object rather
than a mocking framework. `buildRequest` and `toResponse` exist for that
reason. `toResponse` also has a functional consequence: splitting the method
turned one read of `model_of_azure_deployment` into two independent reads that
must agree, so it is read from the caller's map without consuming it.
**Java derives the response format through a throwaway typed builder.**
`StructuredOutputsKt.responseFormatFromClass` is a Kotlin facade that is not
callable from Java. `toNativeResponseFormat` therefore builds a minimal
`ChatCompletionCreateParams` with the typed `responseFormat(Class,
JsonSchemaLocalValidation)` overload, extracts the generated format from
`rawParams()`, and reattaches it to the real builder. This produces the SDK's
own strict draft-2020-12 schema rather than a hand-rolled one.
**Python imports a private SDK helper.** `to_strict_json_schema` comes from
`openai.lib._pydantic`, which has a leading underscore. The openai client
itself uses this helper to build the strict `json_schema`, and there is no
public re-export. It has existed at that path since structured-output support
landed in openai 1.66.3, which is the pinned minimum. A future openai bump that
moves it fails loudly at import rather than silently degrading.
**Java's `JsonSchemaLocalValidation.NO` is passed.** Local validation is
left to the provider rather than performed client-side.
### 4. Failure behavior
**Missing deployment name.** `model` absent or blank raises `ValueError` in
Python and `IllegalArgumentException` in Java, before any request is issued.
Unchanged from before.
**Null or blank api-version, Java.** The constructor raises
`IllegalArgumentException`. The connection cannot be built, so no request path
is reachable. Unchanged from before.
**Reserved typed field inside `additional_kwargs`.** Raises `ValueError` /
`IllegalArgumentException` before the request. Unchanged from before.
**Caller-supplied `response_format` colliding with an applied native
schema.** Raises `ValueError` / `IllegalArgumentException` before the request.
The message names the schema and the deployment and states both remedies. This
is new; before this change the combination was unreachable.
**Unsupported configuration that is not an error.** An unrecognized or
absent backing model, an api-version below the floor, and a schema that does
not translate all fall back silently to an unconstrained request. Nothing is
logged and nothing raises. This is deliberate: the prompt-engineering fallback
is expected to govern the response in those cases. It is also the most likely
surprise for a user, since a capable deployment with
`model_of_azure_deployment` unset takes this path.
**An error returned by the service.** In Java, `doChat` catches
`IllegalArgumentException` and rethrows it unwrapped, and wraps every other
exception in `RuntimeException("Failed to call Azure OpenAI chat completions
API.", e)`. This is unchanged from before, but note the seam moved:
`buildRequest` is called inside the same `try`, so an argument error raised
while building still propagates unwrapped. In Python there is no try/except in
this path, so an SDK exception (including a 400 from the provider) propagates
to the caller unchanged. That asymmetry predates this change and is not
introduced by it.
**A response that does not satisfy the requested schema.** This connection
does not validate the response against the schema. With `strict: true`, the
provider is responsible for conformance, and a refusal or a length-truncated
completion is returned as ordinary message content. The message is converted
and returned to the caller unchanged. Parsing the content into the schema type
happens above this layer, so a non-conforming body surfaces there as a parse
failure, not here. There is no retry and no repair in this connection.
**The SDK fails to produce a response format, Java.**
`toNativeResponseFormat` raises `IllegalStateException` naming the schema class
if `rawParams().responseFormat()` is empty. This is a guard against an SDK
behavior change, not an expected path.
**Schema generation rejects the type.** A `BaseModel` or POJO that the SDK's
strict schema generator cannot express raises from the generator, before the
request. Azure additionally enforces limits the generator does not check, for
example nesting depth and open map fields becoming `additionalProperties:
true`, which `strict` rejects. Those surface as a provider error on the first
call. This behavior is inherited from the sibling OpenAI connection rather than
introduced here.
**Import-time failure, Python.** If a future openai release moves
`openai.lib._pydantic.to_strict_json_schema`, importing this module raises
`ImportError` immediately rather than degrading at runtime.
### 5. Compatibility impact
**No public type, field, or method signature changes.** Java adds an
override of a 4-arg `chat` that already exists on the base class, plus two
package-private methods. Python's `output_schema` parameter already existed on
this connection. No configuration field is added, removed, or made required.
**No dependency changes.** No `pom.xml` and no `pyproject.toml` edit. The
Java path uses the openai-java SDK already present in this module.
**A caller that passes no output schema sees identical behavior.** The
request is built by the same statements in the same order, and the response is
converted identically.
**One behavior does change for existing callers.** Passing a non-null
`output_schema` to this connection previously raised, since the connection had
no native translation: `NotImplementedError` in Python via the base guard, and
`UnsupportedOperationException` in Java via the base 4-arg default. It now
either applies the native path or proceeds unconstrained. Nothing in the
framework calls the 4-arg path today, so this is reachable only by a direct
caller.
**`model_of_azure_deployment` gains a second role.** It previously labeled
token-usage metrics only. It now also decides native capability. Its type,
default, and absent-behavior are unchanged, and the Setup Javadoc was updated
to say so. Callers who never set it are unaffected apart from staying on the
fallback.
**The cross-connection walk test changes population, not strictness.** It
exempts natively-translating connections. On this branch the exemption matches
exactly one class and leaves the others held to the rejection contract. Its
pre-existing non-empty assertion still runs on the filtered list, so an
exemption that grew to cover everything would fail.
### 6. Tests to contracts
Python tests are in
`azure/tests/test_azure_openai_native_structured_output.py`, a new
non-integration file, because the existing Azure test module is
integration-marked and would be skipped in the unit run. Java tests extend the
existing `AzureOpenAIChatModelConnectionTest`; its five pre-existing tests are
unmodified.
| Test | Language | Contract pinned |
|---|---|---|
| `test_native_applied_for_capable_deployment_model` /
`testNativeAppliedForCapableDeploymentModel` | both | C1 |
| `test_capable_native_request_still_targets_the_deployment` /
`testCapableNativeRequestStillTargetsTheDeployment` | both | C2 |
| `test_native_not_applied_when_deployment_model_absent` /
`testNativeNotAppliedWhenDeploymentModelAbsent` | both | C3 |
| `test_native_not_applied_for_unknown_deployment_model` /
`testNativeNotAppliedForUnknownDeploymentModel` | both | C3 |
| `test_native_not_applied_for_bare_gpt_4o` /
`testNativeNotAppliedForBareGpt4o` | both | C4 |
| `test_native_applied_for_other_api_versions_above_floor` /
`testNativeAppliedForOtherApiVersionsAboveFloor` (`2024-10-21`, `v1`) | both |
C5, upper side |
| `test_native_not_applied_when_api_version_below_floor` /
`testNativeNotAppliedWhenApiVersionBelowFloor` | both | C5 |
| `test_native_not_applied_when_api_version_empty` | Python only | C5, empty
case. Java has no equivalent because its constructor rejects blank, which the
pre-existing `testConstructorMissingApiVersion` pins |
| `test_native_not_applied_when_schema_none` /
`testNativeNotAppliedWhenSchemaNull` | both | C7 |
| `test_native_not_applied_for_row_type_info` /
`testNativeNotAppliedForNonPojoSchema` | both | C6 |
| `test_native_applied_even_when_tools_bound` /
`testNativeAppliedEvenWhenToolsBound` | both | C8 |
| `test_caller_response_format_conflicts_with_native_schema` (parametrized
over both channels) / `testCallerResponseFormatConflictsWithNativeSchema` |
both | C9. Python covers two channels, Java one, matching the channel counts
stated in C9 |
| `test_caller_response_format_survives_when_native_is_skipped`
(parametrized over 4 non-native paths x 2 channels) /
`testCallerResponseFormatSurvivesWhenNativeIsSkipped` (`nonNativePaths`) | both
| C10 |
| `test_capability_predicate_accepts_capable_models` /
`testCapabilityPredicateAcceptsCapableModels` | both | C1 capability half, and
C14 by enumerating every allowlist entry |
| `test_capability_predicate_rejects_incapable_models` /
`testCapabilityPredicateRejectsIncapableModels` | both | C3, C4 |
| `test_capability_predicate_reads_no_instance_state` | Python only | C11 |
| `testResponseCarriesBackingModelTokenMetrics` | Java only | C13, positive
case |
| `testResponseHandlingDoesNotConsumeCallerModelParams` | Java only | C12 |
| `testNoTokenMetricsWhenMetricsInputsAreIncomplete` (parametrized) | Java
only | C13, negative cases: backing model unset, and usage absent |
| `test_every_connection_rejects_an_output_schema_it_cannot_translate` |
Python only | C15 |
**Contracts with no direct test, stated rather than omitted:**
- **C11** has no Java equivalent. Java's capability predicate is `protected`
and reads no instance state by inspection, but no Java test constructs an
uninitialized instance to prove it. The Python test exists because Python's
cross-connection walk actually calls the predicate on `cls.__new__(cls)`.
- **C12 and C13** have no Python equivalent. Python's response handling was
not restructured by this change, so its single read of
`model_of_azure_deployment` is the pre-existing one and the
two-reads-must-agree risk that motivates these tests does not exist there.
- **C14** is pinned only in the sense that both capability tests enumerate
the same model list independently. No test compares the two languages'
constants directly, since no cross-language test harness covers this pair.
- The failure paths in section 4 that are inherited rather than introduced
have no new tests: service errors, provider schema-limit rejections, and
non-conforming responses. Existing coverage for the unchanged request and
response paths applies.
--
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]
