-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Fix/responses api harmony channel metadata #28262 #28355
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/responses api harmony channel metadata #28262 #28355
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.
Code Review
This pull request addresses an issue where channel metadata was being lost during multi-turn conversations in the Responses API. The changes correctly assign 'commentary' or 'analysis' channels to function_call_output and reasoning messages, respectively, based on the conversational context. The addition of tests to verify this behavior is a great step. I have one suggestion to simplify the conditional logic for setting the channel on reasoning messages, which will improve code clarity and maintainability.
alecsolder
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.
Thank you for this! Left a few comments.
| Author.new(Role.TOOL, f"functions.{call_response.name}"), | ||
| response_msg["output"], | ||
| ) | ||
| msg = msg.with_channel("commentary") |
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.
Do you want to add the content_type change from the issue here too? Or are you just tackling one specific part of the issue?
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.
msg = msg.with_channel("commentary")
msg = msg.with_content_type("json") -- is that enough?
|
pytest tests/entrypoints/openai/test_response_api_with_harmony.py::test_function_call_with_previous_input_messages -v pass |
|
pytest tests/entrypoints/openai/test_response_api_with_harmony.py -v -k "not test_code_interpreter" this pass test_code_interpreter dont know why out of time |
Purpose
Fix issue #28262: Restore missing channel metadata when converting Responses API output items back to Harmony Messages for multi-turn conversations.
Changes:
Set channel='commentary' for function_call_output type inputs
Set channel='analysis' or 'commentary' for reasoning type based on the following message (commentary if followed by function_call, analysis otherwise)
Add test to verify channel metadata is correctly preserved across conversation turns
Update parse_response_input() to accept optional next_msg parameter for context-aware channel assignmen
Test Plan
pytest tests/entrypoints/openai/test_response_api_with_harmony.py::test_function_call_with_previous_input_messages -v
Test Result
pass
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.