-
Notifications
You must be signed in to change notification settings - Fork 857
fix: forward OpenAI Responses parse params #2341
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -742,6 +742,29 @@ async def test_structured_output(openai_client, model, test_output_model_cls, al | |
| assert tru_result == exp_result | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_structured_output_forwards_formatted_request_params( | ||
| openai_client, model_id, test_output_model_cls, alist | ||
| ): | ||
| model = OpenAIResponsesModel( | ||
| model_id=model_id, | ||
| params={"max_output_tokens": 1000, "reasoning": {"effort": "high"}}, | ||
| ) | ||
| messages = [{"role": "user", "content": [{"text": "Generate a person"}]}] | ||
| mock_response = unittest.mock.Mock(output_parsed=test_output_model_cls(name="John", age=30)) | ||
|
|
||
| openai_client.responses.parse = unittest.mock.AsyncMock(return_value=mock_response) | ||
|
|
||
| await alist(model.structured_output(test_output_model_cls, messages, system_prompt="Follow the schema")) | ||
|
|
||
| _, kwargs = openai_client.responses.parse.call_args | ||
| assert kwargs["max_output_tokens"] == 1000 | ||
| assert kwargs["reasoning"] == {"effort": "high"} | ||
| assert kwargs["instructions"] == "Follow the schema" | ||
| assert kwargs["store"] is False | ||
| assert "stream" not in kwargs | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: The test asserts individual fields but doesn't verify there are no unexpected extra keys being forwarded. If a future change to Suggestion: Add an assertion on the complete set of forwarded keys to catch unintended leakage: expected_keys = {"model", "input", "text_format", "max_output_tokens", "reasoning", "instructions", "store"}
assert set(kwargs.keys()) == expected_keys |
||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_stream_context_overflow_exception(openai_client, model, messages): | ||
| """Test that OpenAI context overflow errors are properly converted to ContextWindowOverflowException.""" | ||
|
|
||
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.
Issue: The exclusion-list approach (
key not in {"model", "input", "stream"}) is fragile. If_format_requestis later extended to add more keys that shouldn't reachresponses.parse()(or if users put keys liketoolsinparamsfor built-in tool use), those will be forwarded silently.Suggestion: Consider using an allowlist of known safe parameters to forward, or additionally exclude keys that are only relevant to streaming/stateful flows (e.g.,
previous_response_id,tools,tool_choice):This would prevent built-in tools configured via
params(e.g.,web_search) from leaking into structured output calls where they're unlikely to be intended.