-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[gpt-oss][2/N] Support input_messages in responsesRequest #26962
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
340b276 to
21815e5
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Andrew Xia <[email protected]>
21815e5 to
ec420f3
Compare
Signed-off-by: Andrew Xia <[email protected]>
Signed-off-by: Andrew Xia <[email protected]>
Signed-off-by: Andrew Xia <[email protected]>
Signed-off-by: Andrew Xia <[email protected]>
| get_system_message, | ||
| parse_chat_input, | ||
| parse_chat_output, | ||
| parse_input_to_harmony_message, |
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.
no functional changes in this file, just renamed the function and formatted code
Signed-off-by: Andrew Xia <[email protected]>
Signed-off-by: Andrew Xia <[email protected]>
|
ready for review, |
|
/gemini review |
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.
Code Review
This pull request adds support for previous_input_messages in ResponsesRequest to enable multi-turn conversations, which is a valuable addition. The implementation is mostly sound and is accompanied by new tests. However, I've identified a couple of high-severity issues. One is a bug in the new test case where the tool name is incorrect, potentially masking issues. The other is a significant code duplication in the message processing logic, which impacts maintainability. Addressing these points will improve the robustness and quality of the code.
Signed-off-by: Andrew Xia <[email protected]>
Signed-off-by: Andrew Xia <[email protected]>
04fad7a to
b1d2b02
Compare
qandrew
left a comment
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.
@yeqcharlotte @alecsolder thanks for the comments! should be ready for another review
| message_role = message.author.role | ||
| # To match OpenAI, instructions, reasoning and tools are | ||
| # always taken from the most recent Responses API request | ||
| # not carried over from previous requests | ||
| if ( | ||
| message_role == OpenAIHarmonyRole.SYSTEM | ||
| or message_role == OpenAIHarmonyRole.DEVELOPER | ||
| ): | ||
| continue |
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.
move these logic to harmony util?
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.
@yeqcharlotte thanks for the catch, done
Signed-off-by: Andrew Xia <[email protected]>
yeqcharlotte
left a comment
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.
thanks for addressing all the updates. it looks much cleaner now!
…ct#26962) Signed-off-by: Andrew Xia <[email protected]> Co-authored-by: Andrew Xia <[email protected]>
…ct#26962) Signed-off-by: Andrew Xia <[email protected]> Co-authored-by: Andrew Xia <[email protected]>
…ct#26962) Signed-off-by: Andrew Xia <[email protected]> Co-authored-by: Andrew Xia <[email protected]>
…ct#26962) Signed-off-by: Andrew Xia <[email protected]> Co-authored-by: Andrew Xia <[email protected]>
Purpose
in the RepsonsesResponse object, we already have input_messages and output_messages. We should be able to do multi_turl by allowing ResponsesRequest to have previous_input_messages as payload.
Test Plan
I tested following OpenAI's example: https://platform.openai.com/docs/guides/function-calling#function-tool-example
Added the following as a test also.
Turn 1
Turn 1 output
Turn 2