WIP: Attempt to fix reasoning models support#6
Open
raghubetina wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 60024a4 in 1 minute and 41 seconds. Click for details.
- Reviewed
90lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft 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. CHANGELOG.md:3
- Draft comment:
The changelog update describes the new reasoning parameter functionality. Ensure that the changelog reflects all changes consistently (e.g., see additional notes in version 0.0.7). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that the changelog is updated, which falls under the rule of not asking the author to update the PR description or similar documentation. It doesn't provide a specific code suggestion or point out a potential issue in the code itself.
2. lib/ai/chat.rb:128
- Draft comment:
Good refactoring of the reasoning parameters in 'assistant!'. Always passing 'summary=auto' and conditionally adding 'effort' correctly meets the new API requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. lib/ai/chat.rb:215
- Draft comment:
The 'reasoning' method correctly retrieves reasoning data from assistant messages. Verify its usage in the context of the overall API response processing. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. lib/ai/chat.rb:8
- Draft comment:
The attr_reader for :reasoning_output is declared but never assigned. If you intend to expose separate reasoning output, consider setting @reasoning_output when extracting reasoning data, else remove the attribute. - Reason this comment was not posted:
Marked as duplicate.
5. lib/ai/chat.rb:195
- Draft comment:
The extraction of the output text uses a safe navigation with a ternary operator which might be less clear. Consider refactoring for clarity, e.g. explicitly checking if output_text_item is not nil and has a 'text' key. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. lib/ai/chat.rb:215
- Draft comment:
The 'reasoning' method searches the messages array each time for the last assistant message with reasoning. If this becomes a performance issue, consider caching the reasoning output upon extraction. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_IVPzTxwBXXnSN0Ay
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| class Chat | ||
| attr_accessor :messages, :schema, :model | ||
| attr_reader :reasoning_effort | ||
| attr_reader :reasoning_effort, :reasoning_output |
There was a problem hiding this comment.
The new attr_reader 'reasoning_output' is declared but never used in the class. Consider removing or implementing its usage.
Suggested change
| attr_reader :reasoning_effort, :reasoning_output | |
| attr_reader :reasoning_effort |
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.
Summary
reasoningparameter withsummary=autoto OpenAI API for all callsCurrent status
This is still not working correctly but saving the work-in-progress for reference.