weiqingy commented on PR #930:
URL: https://github.com/apache/flink-agents/pull/930#issuecomment-5100699771
Revision 2. Section 4 previously said a refusal is returned as ordinary
message content, which is wrong: a refusal arrives with empty content and is
carried separately, and in Python it is dropped. Corrected below, with a new
contract in section 2, a compatibility note in section 5, and the missing test
coverage recorded in section 6. Revision 1 stays above unchanged.
## 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`. The response is converted by `convert_from_openai_message` at
`azure_openai_chat_model.py:319`, unchanged from 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 through
`OpenAIChatCompletionsUtils.convertFromOpenAIMessage`, 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.
Neither language inspects `finish_reason` at any point on this path.
### 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.
**C16 (new in this revision).** A model refusal is not represented as
message content, and the two languages differ in what survives it. The provider
returns an empty `content` with the explanation in a separate `refusal` field.
Java preserves it: `OpenAIChatCompletionsUtils.convertFromOpenAIMessage` sets
`content` to `message.content().orElse("")` at line 114 and copies the refusal
into `extraArgs["refusal"]` at line 117, so a caller sees an empty assistant
message plus a recoverable `refusal` entry. Python discards it:
`convert_from_openai_message` in `openai_utils.py:217-222` sets
`content=message.content or ""` and has no refusal handling, and the string
`refusal` does not appear anywhere under `python/flink_agents/`, so nothing
downstream recovers it. A Python caller receives an empty assistant message
with no indication that a refusal occurred. This is a Java and Python
divergence in observable behavior. It predates this change, as established in
section 5.
### 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.
**Response handling was deliberately left unchanged.** Both connections
delegate to the shared conversion helper that already existed. That keeps this
change scoped to the request side, and it is why the refusal representation
described in C16 is inherited rather than addressed here.
### 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 model refusal.** Corrected in this revision. Under `strict: true` a
model that will not answer refuses rather than emitting non-conforming JSON, so
this is the primary non-happy path that native structured output introduces.
The provider returns an empty `content` and puts the explanation in a separate
`refusal` field. Neither connection raises, retries, or falls back. Java
preserves the explanation in `extraArgs["refusal"]`
(`OpenAIChatCompletionsUtils.convertFromOpenAIMessage:114-117`), so a caller
can detect and read it, though only by inspecting `extraArgs`; an empty
`content` on its own is indistinguishable from a genuinely empty completion.
Python discards it (`openai_utils.py:217-222`, no refusal handling, and no
occurrence of `refusal` anywhere under `python/flink_agents/`), so the refusal
is silently absorbed and the caller receives an empty assistant message with
the reason unavailable. Downstream parsing of that empty content into the
schema type fails above this la
yer, with no indication that a refusal was the cause.
**A length-truncated completion.** `finish_reason` is not inspected in
either language on this path. A truncated completion is returned as ordinary
partial content, silently, and fails downstream at parse time. This part of the
previous revision's statement was accurate; only the refusal half was wrong.
**A response that otherwise 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. 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 refusal handling in C16 predates this change, but this change makes it
easier to reach.** `openai_utils.py` and `OpenAIChatCompletionsUtils.java` are
not in this diff, and `git log -S"refusal" -- python/flink_agents/` returns no
commit, so the string has never existed on the Python side. A refusal was
already possible for any Azure caller, since the provider populates that field
for safety refusals generally. What changes is frequency: `strict: true` is the
mode in which a model refuses instead of emitting non-conforming JSON, and
before this change an Azure caller with a schema always took the prompt
fallback and never sent `strict: true`. The gap is therefore inherited rather
than introduced, and fixing it means changing shared response-conversion code
used by the OpenAI connection as well, which is outside this change's
request-side scope. It belongs in a separate change covering both connections.
**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:**
- **C16 has no test in either language.** Nothing constructs a completion
carrying a `refusal` and asserts what the returned `ChatMessage` contains. This
is the gap the correction in this revision exposes, and it is the least covered
path of the ones this change makes reachable. The Java seam added here,
`toResponse`, would make such a test straightforward, since a `ChatCompletion`
with a refusal can be built offline from the SDK builders. Python has no
equivalent seam today.
- **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, truncated
completions, 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]
