-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix LLMJudge input handling to preserve BinaryContent as separate message part instead of stringifying #2173
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
base: main
Are you sure you want to change the base?
Fix LLMJudge input handling to preserve BinaryContent as separate message part instead of stringifying #2173
Conversation
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.
@adtyavrdhn Thanks Aditya! A few suggestions.
# For non-string inputs (e.g., BinaryContent), build a list | ||
prompt_parts: list[str | UserContent] = [] | ||
prompt_parts.append('<Input>\n') | ||
prompt_parts.append(inputs) |
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.
inputs
may itself be a list of UserContent
s, in which case we should extend
rather than `append
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.
We should probably make the signature UserContent, Sequence[UserContent] for inputs then as well? Is there a specific reason we are keeping it Any?
<Rubric> | ||
{rubric} | ||
</Rubric> | ||
""") |
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.
Similar to below, can we have build a list of strings and conditionally add the item for ExpectedOutput
, and join
them all at the end? That'll reduce the duplication a bit.
We can likely also merge the 2 isinstance(inputs, str)
branches, and only join
if isinstance(inputs, str)
, and return the entire list otherwise
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.
I've refactored it to reduce duplication
…inary_content_incorrect_parsing_fix
Fix LLMJudge input handling to preserve BinaryContent as separate message part instead of stringifying
Fixes #2089