fix: guard against empty choices and null message in LLM responses#1695
fix: guard against empty choices and null message in LLM responses#1695qizwiz wants to merge 2 commits into
Conversation
Accessing response.choices[0].message.content without first checking that choices is non-empty and message is not None can crash with: - IndexError: empty choices list (network error, quota exhaustion) - AttributeError: None message (Gemini PROHIBITED_CONTENT returns HTTP 200 with null message instead of an error status) Six locations fixed across five files: - open_instruct/rubrics/run_utils.py (2 locations) Return empty string when response is empty, matching existing error-path behaviour. - open_instruct/ground_truth_utils.py (1 location) Raise ValueError inside existing try/except; error is logged and reasoning/score return their zero-values. - open_instruct/rejection_sampling/synthetic_preference_dataset.py (1) Raise ValueError inside retry loop's try/except. - scripts/does_prompt_make_sense.py (1 location) Raise ValueError inside retry loop's try/except. - scripts/data/rlvr_code/the_algorithms.py (1 location) Raise ValueError; caught by outer except → returns error dict. - scripts/data/rlvr_code/sft_to_rlvr_azure.py (1 location) Guard before content access; also simplifies redundant double access on the same line.
There was a problem hiding this comment.
Code Review
This pull request implements safety checks across several files to handle empty or filtered LLM responses, preventing potential errors when accessing response content. The reviewer suggests further hardening these guards by also checking if the message content is null, which can occur with certain LLM providers and would lead to runtime errors or type hint violations.
| if not completion.choices or completion.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The guard should also check if completion.choices[0].message.content is None. Some LLM providers (like Gemini) may return a valid message object with null content when a response is filtered, which would cause a TypeError in the subsequent re.sub call.
| if not completion.choices or completion.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not completion.choices or completion.choices[0].message is None or completion.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The guard should also check if response.choices[0].message.content is None. If content is None, the subsequent r.split call will raise an AttributeError.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The guard should also check if response.choices[0].message.content is None. If content is None, the subsequent response_content.startswith call (line 156) will raise an AttributeError.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The guard should also check if response.choices[0].message.content is None. If content is None, the returned dictionary will contain None for openai_response, which might cause issues for consumers expecting a string.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The guard should also check if response.choices[0].message.content is None. If content is None, the subsequent r.split call (line 79) will raise an AttributeError.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| return "" |
There was a problem hiding this comment.
The guard should also check if response.choices[0].message.content is None. If content is None, this function will return None, which violates the str return type hint and may cause crashes in downstream code expecting a string.
| if not response.choices or response.choices[0].message is None: | |
| return "" | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| return "" |
| if not response.choices or response.choices[0].message is None: | ||
| return "" |
There was a problem hiding this comment.
The guard should also check if response.choices[0].message.content is None. If content is None, this function will return None, which violates the str return type hint and may cause crashes in downstream code expecting a string.
| if not response.choices or response.choices[0].message is None: | |
| return "" | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| return "" |
Extend all six guards to additionally check that message.content is not None, covering providers that return a non-None message object but set content to None (e.g. on max-token truncation).
|
Good catch @gemini-code-assist — extended all six guards to additionally check |
|
Thank you for the update, @qizwiz. That looks correct and ensures robust handling for providers that return a valid message object but omit the content field. This change significantly improves the reliability of the LLM response processing across all identified paths. |
|
Thanks for the review @gemini-code-assist. Pushed 9f3f166 extending all six guards to also check |
|
Thank you for the update, @qizwiz. The addition of the |
Summary
Six locations across five files access
response.choices[0].message.contentwithout first verifying thatchoicesis non-empty andmessageis notNone.Two crash vectors that bypass existing try/except blocks:
choiceslist (quota exhaustion, network failure, provider-side filtering)choices[0].messageisNone(documented GeminiPROHIBITED_CONTENTbehaviour: HTTP 200 response with null message object rather than an error status)Files changed
open_instruct/rubrics/run_utils.py""(matches existing except-path behaviour)open_instruct/ground_truth_utils.pyValueError→ caught byexcept Exception→ returns zero-valuesopen_instruct/rejection_sampling/synthetic_preference_dataset.pyValueError→ caught by retry-loopexcept Exceptionscripts/does_prompt_make_sense.pyValueError→ caught by retry-loopexcept Exceptionscripts/data/rlvr_code/the_algorithms.pyValueError→ caught by outerexcept→ returns error dictscripts/data/rlvr_code/sft_to_rlvr_azure.pyPattern
References
llm_response_unguardedrule)