feat: add OpenAI Responses API model implementation #975
Conversation
notgitika
requested a deployment
to
manual-approval
GitHub Actions
Waiting
| logger.debug("config=<%s> | initializing", self.config) | ||
|
|
||
| @override | ||
| def update_config(self, **model_config: Unpack[OpenAIResponsesConfig]) -> None: # type: ignore[override] |
There was a problem hiding this comment.
Just wondering why we need it here: # type: ignore[override]
There was a problem hiding this comment.
good question! the base class accepts **model_config: Any but here we use the more specific Unpack[OpenAIResponsesConfig] type which raises the mypy error.
openai.py also has this: https://github.com/strands-agents/sdk-python/blob/main/src/strands/models/openai.py#L73
| } | ||
|
|
||
| @classmethod | ||
| def _format_request_tool_message(cls, tool_result: ToolResult) -> dict[str, Any]: |
There was a problem hiding this comment.
do we not handle images in tool results? Seems like these get dropped?
| self, | ||
| messages: Messages, | ||
| tool_specs: Optional[list[ToolSpec]] = None, | ||
| system_prompt: Optional[str] = None, |
There was a problem hiding this comment.
is the concept of tool_choice similar to how we have it in the existing provider?
| if message.get("content") or message.get("type") in ["function_call", "function_call_output"] | ||
| ] | ||
|
|
||
| def _format_request( |
There was a problem hiding this comment.
the ordering here is strange to me, it doesn't read like a story as much. I'd expect this to be something like the following top to bottom. Introducing a private method before it is used I'm not a fan of
stream
_format_request
_format_request_message_content
_format_request_message_tool_call
_format_request_tool_message
| """ | ||
| match event["chunk_type"]: | ||
| case "message_start": | ||
| return {"messageStart": {"role": "assistant"}} |
There was a problem hiding this comment.
reach out to @zastrowm , but I believe we can make this more readable by now returning the typed StreamEvents rather than the dictionaries
|
|
||
| for message in messages: | ||
| role = message["role"] | ||
| if role == "system": |
There was a problem hiding this comment.
can you explain and add a comment why we are skipping these? And then higher up can in the description can you add a comment briefly describing opeai responses and how it differs fundamentally from the existing stateless models
| async with openai.AsyncOpenAI(**self.client_args) as client: | ||
| try: | ||
| response = await client.responses.create(**request) | ||
| except openai.APIError as e: |
There was a problem hiding this comment.
in some places we are catching RateLimitError and others it looks like we are just looking at the code. Same for badrequest+context overflow
can you explain why we have different error handling and unify if possibly
3 participants
Description
This PR implements OpenAI Responses API as a separate model provider supporting streaming, structured output and tool calling.
Note: this commit does not include the extra capabilities that the Responses API supports such as the built-in tools and the stateful conversation runs.
Related Issues
#253
Documentation PR
TODO; coming next: will be adding a section for the Responses API within the existing openai model page
Type of Change
New feature
Testing
Added a unit test file similar to the existing
test_openai.pymodel provider. Reusing the integ tests in the same filetest_model_openai.pyusingpytest.parameterizewithOpenAIResponsesmodel so that I could test that the funcitonality is the same between the two models.I ran everything in the CONTRIBUTING.md file.
hatch run integ-test================== 81 passed, 68 skipped, 49 warnings in 106.56s (0:01:46) ==========
hatch run test-integ tests_integ/models/test_model_openai.py -v============= 18 passed, 2 skipped in 13.44s =============
pre-commit run --all-fileshatch run prepare--> yes, all tests passChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.