feat: add OpenAI Responses API model implementation#975
feat: add OpenAI Responses API model implementation#975notgitika wants to merge 8 commits intostrands-agents:mainfrom
Conversation
|
Hey Team, this is one of the feature my customer request and concerned as blocker from adopting Strands Agent. They mentioned Strands currently only support deprecated chatcompletion API and not responses API. When can we expect this to be merged? and will this also enable streaming structured output? |
dbschmigelski
left a comment
There was a problem hiding this comment.
A major version bump is proposed in #1370.
Adding a comment here that we would need to consider when the user has v1 installed compared to v2.
For example
import pydantic
from packaging import version
# Detect the version once at the module level
PYDANTIC_V2 = version.parse(pydantic.VERSION) >= version.parse("2.0.0")
if PYDANTIC_V2:
from pydantic import ConfigDict
def get_model_fields(model):
return model.model_fields
else:
def get_model_fields(model):
return model.__fields__
7c05e84 to
8eea5d9
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
notowen333
left a comment
There was a problem hiding this comment.
Some refactoring is required to make this maintainable. Otherwise LGTM
…-responses-model # Conflicts: # tests_integ/models/test_model_openai.py
6a70455 to
83e2982
Compare
|
Assessment: Comment This PR adds a well-structured OpenAI Responses API model implementation with comprehensive test coverage (~91%). The core functionality is solid and the integration test reuse via parameterization is a good approach. Review Categories
The implementation demonstrates a thorough understanding of the Responses API and handles edge cases well (reasoning content, incomplete responses, tool call accumulation). The existing review comments about streaming typed events can be addressed in a follow-up iteration. |
|
/strands can you address the comments? Also can you leave a comment with a suggestion on how to fix this integ test failure: |
- Add OpenAIResponsesModel to lazy loading in strands.models.__init__.py - Remove 'Step X:' prefixes from logging to match project conventions - Fix unused variable in test fixture by removing model_name from params
Integration Test Failure FixThe error occurs because Suggested FixUpdate # Remove the top-level import:
# from strands.models.openai_responses import OpenAIResponsesModel # DELETE THIS LINE
# Change the provider definition to use lazy import:
openai_responses = ProviderInfo(
id="openai_responses",
environment_variable="OPENAI_API_KEY",
factory=lambda: __import__(
"strands.models.openai_responses", fromlist=["OpenAIResponsesModel"]
).OpenAIResponsesModel(
model_id="gpt-4o",
client_args={
"api_key": os.getenv("OPENAI_API_KEY"),
},
),
)Alternatively, for cleaner code: def _create_openai_responses_model():
from strands.models.openai_responses import OpenAIResponsesModel
return OpenAIResponsesModel(
model_id="gpt-4o",
client_args={"api_key": os.getenv("OPENAI_API_KEY")},
)
openai_responses = ProviderInfo(
id="openai_responses",
environment_variable="OPENAI_API_KEY",
factory=_create_openai_responses_model,
)This defers the import until the model is actually instantiated, avoiding the version check in environments that don't have openai >= 2.0.0. |
|
I opted for a different approach for the integ test fix. The lazy import approach suggested only fixes the issue in With this flag approach, both files can check _openai_responses_available and skip the responses model when its not installed |
|
Assessment: Approve The PR has addressed the key issues from the previous review: Changes Since Last Review
The conditional import approach for handling older openai versions is well-designed - it allows the module to gracefully skip when openai < 2.0.0 rather than crashing at import time. Minor suggestions from previous review (client injection parity, TODO comment) can be addressed in follow-up iterations. The core implementation is solid and ready for merge. |
Unshure
left a comment
There was a problem hiding this comment.
Integ test failing for unrelated reason
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.