-
Notifications
You must be signed in to change notification settings - Fork 28
Modify the toText
and toTexts
methods of the GenerativeAiResult
class to only return the content channel
#92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…at has text. This ensures reasoning_content isn't returned
…have text. This ensures reasoning_content isn't returned
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkotter Thank you for fixing this! The PR changes a bit more than fixing the bug, so I think that needs to be simplified.
The other part is that this exact same bug can also occur for all the other to*()
methods in GenerativeAiResult
, so I think we should fix it there too to only return content bits. Granted, it's likely impossible as of today for any provider to include things like images etc in "thought" or "reasoning", but as per the DTO it's in principle possible. So ideally we adjust all these similar methods to only consider "content" parts.
…andidate. Simplify how we check the channel. Update tests
I've updated the
There's also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkotter Looks great! Just making some minor doc nit-pick updates.
You're completely right regarding toMessage()
and toMessages()
, these should maintain the messages as is.
As described in #89, if you use a reasoning model, the content returned contains both
reasoning_content
andcontent
items. Thereasoning_content
is always first in the array and so whentoText
is returned,reasoning_content
will always be returned instead ofcontent
.This PR adjusts the logic of those methods to only return items that are in the
content
channel.May be worth a discussion on if we:
content
. Right now if you want thereasoning_content
, you'd have to parse that out yourself from the response instead of using the built-in helper methodscontent
channel? For instance, if an LLM for some reason only returns the reasoning result, this new approach will return nothing (or will throw an exception). I debated modifying these methods to fallback to returning the first result if we don't find any result that matches thecontent
channel but wasn't sure if we actually wanted it to function that way