[integrations][openai] Preserve provider refusals in the Python message converter - #952
[integrations][openai] Preserve provider refusals in the Python message converter#952weiqingy wants to merge 1 commit into
Conversation
…ge converter When a provider refuses a request it returns an assistant message with no content and the reason in `refusal`. Java carried that reason into `extraArgs["refusal"]`, while the Python converter dropped it, so a refusal reached the caller as an empty assistant message indistinguishable from a genuinely empty completion. Mirror the Java write in `convert_from_openai_message`, which both the OpenAI and Azure OpenAI connections share. The guard is `is not None` rather than a truthiness test so that an empty refusal reason is preserved, matching Java's `Optional.ifPresent`. Add unit tests on both sides: Python for the new behavior, Java to lock the existing behavior that had no coverage.
|
Hi @wenjin272 , could you take a look at this PR when you get a chance? A second sample for the Implementation Description experiment on #894, the Python refusal parity fix that came out of writing the #930 description. It is much smaller than #930, 123 lines across 4 files, so it may be an easier one to start with if #930 is a lot to get through. Two things changed in how it is written, both because #930's description turned out to be unreadable at 24k characters. The description is the PR body now rather than a separate comment. Four of the six fields you listed already have homes in the current template, so only behavioral contracts and failure behavior needed a new heading. The whole body is about 6k characters. The tests table maps one row per contract instead of one row per test, which is what made the #930 version long. I have not touched the PR template itself. That seemed worth holding until you have actually reviewed one of these and can say whether it makes the review easier or just adds reading. |
Linked issue: #936
Addresses case 1 only. Cases 2 and 3 are design questions and are left as follow-ups, as recorded on the issue.
Purpose of change
Java and Python disagree on what a caller receives when a provider refuses a request. Java treats the refusal as recoverable information and Python discards it, so this is a parity fix with Java as the specification.
On a refusal the provider returns an assistant message with no content and the reason in
refusal.OpenAIChatCompletionsUtils.convertFromOpenAIMessagerecords it asextraArgs["refusal"]. The Python converter never read the field, so the reason was lost andcontentbecame"". A Python caller could not tell a refusal apart from a genuinely empty completion.The gap predates the native structured-output work, but
strict: truemakes the refusal path materially more reachable, because a model refuses rather than emitting non-conforming JSON.Runtime flow
Unchanged except for one step. A connection calls
chat.completions.create, builds a localextra_argsholdingmodel_nameand token counts when usage is present, and passes it withresponse.choices[0].messagetoconvert_from_openai_message. The converter builds the tool-call list, then, whenmessage.refusal is not None, rebindsextra_argsto a new dict carrying the reason before constructing theChatMessage. Pydantic copies that dict during validation, so the write must precede construction to be visible.Both the OpenAI and the Azure OpenAI connections call this helper, so one change covers both.
Key decisions
The guard is
is not Nonerather than a truthiness test, so an empty refusal reason is still recorded. Java usesOptional.ifPresent, which keeps an empty string, and a truthiness test would have left a new divergence in the function meant to remove one.There is no
isinstancecheck. The inbound Java path has none. Java'sinstanceof Stringguards the outbound direction, whereextra_argsholds arbitrary caller data, which is a different situation.The write rebinds rather than mutating the argument. Both callers pass a fresh local dict and pydantic copies it, so this is hygiene and not a guarantee. No test pins it, deliberately.
Java gets tests but no production change. Its behavior had no coverage, so a refactor could have removed it and reopened the gap from the other side.
Implementation Description
Behavioral contracts
extra_args["refusal"]on the returnedChatMessage.refusalkey is added.extra_argspassed by the caller survive, including the token metrics both connections put there.contentas"". The reason is never written intocontent.Failure behavior
Nothing in this change raises, falls back, or retries. It adds one conditional dict write with no error path. Invalid configuration does not apply, since the converter takes none.
An error from the provider is unaffected and still propagates out of the Python connection unwrapped, which is case 3 on the linked issue and is deliberately not touched.
A response violating the expected shape is rejected before the converter sees it, since
refusalis declaredOptional[str]on the SDK's pydantic model. The converter does no type checking of its own, so a caller bypassing that model and supplying another type would have it stored as-is.A message lacking the attribute would raise
AttributeError. The field exists at the floor of theopenai>=1.66.3pin, so no version guard is used.Tests
There was no non-integration coverage of the response-conversion path before this, in either language.
test_refusal_is_preserved_in_extra_args, Python.testRefusalPreservedInExtraArgs, Javatest_refusal_is_preserved_in_extra_args, the""parametertest_no_refusal_key_when_refusal_absent, Python.testNoRefusalKeyWhenAbsent, Javatest_refusal_is_preserved_in_extra_args, which passes a non-emptyextra_argsand asserts it survivestest_refusal_is_preserved_in_extra_argsEach test was run against a deliberately broken implementation to confirm it fails when the behavior it covers breaks. A truthiness guard is caught only by the empty-string parameter, and replacing the merge with a plain assignment only by the surviving-keys assertion.
One existing fixture in
test_openai_native_structured_output.pymocked the SDK message with onlyrole,contentandtool_calls. It now setsrefusaltoo, so it does not hand back an auto-generated attribute.Java runs 22 tests in the openai integration module, up from 20. The Python unit suite runs 657.
Azure is covered through the shared helper rather than by a test of its own, because every Azure test in the repo is integration-marked.
API
No signature change, no new class, no new dependency.
extra_argsis an existing field onChatMessage, andrefusalis a key Java already writes, so the cross-language contract is unchanged. Python starts honoring it.For a caller who does nothing differently, one thing changes: on a refused response,
extra_argsnow carries an extra key. Responses that were not refused are unaffected and gain no key.There is one effect beyond the changed files.
convert_to_openai_messagemergesextra_argsinto the outbound assistant message, so aChatMessagethat carries a refusal and is later sent back to the provider will now includerefusalin that request. It is a declared field there, so this is valid and matches Java.Documentation
doc-neededdoc-not-neededdoc-included