Python: fix: handle string response in _get_metadata_from_chat_response (Closes #6234)#6257
Python: fix: handle string response in _get_metadata_from_chat_response (Closes #6234)#6257kuishou68 wants to merge 1 commit into
Conversation
microsoft#6234) Co-authored-by: lingxiu58 <86288566+lingxiu58@users.noreply.github.com> Co-authored-by: pojian68 <232320289+pojian68@users.noreply.github.com> Signed-off-by: Cocoon-Break <54054995+kuishou68@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens OpenAI chat completion response parsing by adding defensive type-checking and making metadata extraction tolerant of missing fields.
Changes:
- Add a guard that raises a
ChatClientExceptionwhen the service returns astrinstead of aChatCompletion. - Make
system_fingerprintextraction resilient to SDK/service responses where the attribute is absent.
| if isinstance(response, str): | ||
| raise ChatClientException( | ||
| maybe_append_azure_endpoint_guidance( | ||
| f"{type(self)} service returned an invalid response (str instead of ChatCompletion): {response!r}", | ||
| azure_endpoint=self.azure_endpoint, | ||
| ), | ||
| ) |
| if isinstance(response, str): | ||
| raise ChatClientException( | ||
| maybe_append_azure_endpoint_guidance( | ||
| f"{type(self)} service returned an invalid response (str instead of ChatCompletion): {response!r}", |
github-actions
Bot
changed the title
| """Get metadata from a chat response.""" | ||
| return { | ||
| "system_fingerprint": response.system_fingerprint, | ||
| "system_fingerprint": getattr(response, "system_fingerprint", None), |
There was a problem hiding this comment.
Should we mirror this onto the streaming path too? The fix hardens non-streaming, but here response.system_fingerprint is still a direct deref, and _parse_response_update_from_openai has no str guard. A provider returning the same malformed payload while streaming hits the cryptic AttributeError again. The outer except Exception still wraps it into ChatClientException, so it's loud, just without the specific "str instead of ..." message this PR adds for non-streaming. Same contract on both paths?
|
|
||
| def _parse_response_from_openai(self, response: ChatCompletion, options: Mapping[str, Any]) -> ChatResponse: | ||
| """Parse a response from OpenAI into a ChatResponse.""" | ||
| if isinstance(response, str): |
There was a problem hiding this comment.
Are we covering the new branch? Couldn't find a test exercising the str -> ChatClientException path in python/packages/openai/tests/openai/test_openai_chat_completion_client.py. Existing direct-call tests make it nearly free: client._parse_response_from_openai("some string", {}) inside pytest.raises(ChatClientException), asserting substring "str instead of ChatCompletion" rather than the full message (Azure suffix is env-dependent). Pins the contract so a refactor can't quietly regress it back to the AttributeError.
|
This is a duplicate of #6270 that solves for two issues, closing this one. |
Closes #6234
Problem
When a non-conformant OpenAI-compatible backend returns a plain string instead of a
ChatCompletionobject,_parse_response_from_openai()passes it directly to_get_metadata_from_chat_response(), which crashes with:Fix
Two defensive changes:
_parse_response_from_openai(): Added anisinstance(response, str)check at the top of the method. If the response is a string, a descriptiveChatClientExceptionis raised with the invalid payload included for debugging — following the same pattern used elsewhere in this file for error handling._get_metadata_from_chat_response(): Changedresponse.system_fingerprinttogetattr(response, "system_fingerprint", None)as defense-in-depth, so even if an unexpected non-ChatCompletion object slips through, it won't crash with anAttributeError.Testing
_parse_response_from_openai(seeBadRequestErrorhandling at lines 557-571).getattrguard is consistent with howlogprobsis already handled in_get_metadata_from_chat_choice(line 810).Co-authored-by: lingxiu58 86288566+lingxiu58@users.noreply.github.com
Co-authored-by: pojian68 232320289+pojian68@users.noreply.github.com