Added regression test for openai/harmony/issues/78#29830
Added regression test for openai/harmony/issues/78#29830jacobthebanana wants to merge 4 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a regression test for an issue where response messages can contain empty content. The test correctly reproduces the bug and is well-documented. I have one suggestion to make the test more robust against potential TypeError exceptions.
| # Regression test for github.com/openai/harmony/issues/78 (empty content) | ||
| is_query_returned: bool = False | ||
| for _message in [*response.input_messages, *response.output_messages]: | ||
| for _item in _message.get("content"): |
There was a problem hiding this comment.
The content of a message can be None, for example in an assistant message that only contains tool calls. If _message.get("content") returns None, iterating over it will raise a TypeError. To make the test more robust, you should handle the None case, for example by iterating over an empty list if content is None.
| for _item in _message.get("content"): | |
| for _item in _message.get("content") or []: |
There was a problem hiding this comment.
Updated to use _message["content"] instead of _message.get("content") since "content" is required per OAI Harmony specs- better to raise KeyError if that is missing in vLLM output
Signed-off-by: Jacob-Junqi Tian <jacob@banana.abay.cf>
23de85f to
f44dda6
Compare
💡 Codex Reviewhttps://github.com/vllm-project/vllm/blob/23de85f3697b84c698c465fd6d3d911adb091bbf/tests/entrypoints/openai/test_response_api_with_harmony.py#L869-L872 The new loop asserts every content element in both ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Harmony specs Signed-off-by: Jacob-Junqi Tian <jacob@banana.abay.cf>
Signed-off-by: Jacob-Junqi Tian <jacob@banana.abay.cf>
|
Can you make the pre-commit pass? |
Signed-off-by: Jacob-Junqi Tian <jacob@banana.abay.cf>
553706f to
a3ea500
Compare
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
Purpose
Add regression tests for openai/harmony#78, particularly the FastAPI
vllm servepath which wasn't fully addressed in #26185.Test Plan
Modify the existing
test_output_messages_enabledtest in test_response_api_with_harmony.py to add the following validations:full code details
Test Result
FAILED tests/entrypoints/openai/test_response_api_with_harmony.py::test_output_messages_enabled[openai/gpt-oss-20b] - AssertionError:
{'author': {'name': None, 'role': 'system'}, 'channel': None, 'content': [{}], 'content_type': None, ...}Full test output
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.