fix: Bedrock OpenAI models response parsing (reasoning before text)#1860
Merged
jxnl merged 1 commit into567-labs:mainfrom Oct 24, 2025
Merged
fix: Bedrock OpenAI models response parsing (reasoning before text)#1860jxnl merged 1 commit into567-labs:mainfrom
jxnl merged 1 commit into567-labs:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to aea9365 in 2 minutes and 3 seconds. Click for details.
- Reviewed
141lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/processing/function_calls.py:406
- Draft comment:
Directly indexing keys (e.g. completion["output"]["message"]["content"]) may raise KeyError if the expected structure is missing. Consider safer access or explicit error checks if malformed inputs are possible. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 85% The comment makes a valid point about defensive programming. Direct dictionary access can raise KeyErrors if keys are missing. The suggestion to use .get() with default values would make the code more robust. However, this is part of an internal parsing function that expects a specific format, and the code already has error handling with a clear error message if the content is not found. The comment may be overly defensive - if the input format is wrong, failing fast with a KeyError might be preferable to silently continuing with default values. The existing error handling may be sufficient. While failing fast has merits, using .get() would provide more graceful error handling and clearer error messages, which is valuable for an API that processes external inputs. The comment should be kept as it suggests a valid improvement to make the code more robust when handling potentially malformed inputs, with clearer error messages.
2. instructor/processing/function_calls.py:407
- Draft comment:
The use of next((c for c in content if "text" in c), None) selects the first dictionary with a 'text' key. Confirm that this is the intended behavior when multiple valid text items exist. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold85%The comment is asking the author to confirm their intention, which violates the rule against asking for confirmation. However, it does point out a potential issue with the logic that could be useful for the author to consider. The comment could be rephrased to suggest a code change or improvement instead of asking for confirmation.
3. instructor/processing/function_calls.py:411
- Draft comment:
The regex extraction and subsequent re.sub call remove patterns like newline characters. Verify that removing '\n' does not unintentionally break valid JSON formatting in some responses. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
4. tests/test_json_extraction.py:289
- Draft comment:
The new tests for parse_bedrock_json cover many scenarios. Consider adding a test case for non-dict completions (where the branch using completion.text is exercised) to fully cover all logic paths. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
Workflow ID: wflow_XncyN7p3ApzUHexF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Fixes Bedrock OpenAI model response parsing to handle reasoning text before actual text content and adds tests for validation.
parse_bedrock_jsoninfunction_calls.pyto handle cases wherereasoningTextprecedestextin Bedrock OpenAI model responses.ValueErrorif no text content is found.TestBedrockJSONParsingintest_json_extraction.pyto cover scenarios with reasoning text, code blocks, and multiple text contents.This description was created by
for aea9365. You can customize this summary. It will automatically update as commits are pushed.