Skip to content

Add support for Responses API #541

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

Open
wants to merge 91 commits into
base: main
Choose a base branch
from
Open

Conversation

momostafa
Copy link

@momostafa momostafa commented Mar 22, 2025

What:

Adds the new feature Responses API, supporting:

  • Create Response
  • List Input Items (from a Response)
  • Streaming Create Response
  • Retrieve Response
  • Delete Response

OpenAI's most advanced interface for generating model responses. Supports text and image inputs, and text outputs. Create stateful interactions with the model, using the output of previous responses as input. Extend the model's capabilities with built-in tools for file search, web search, computer use, and more. Allow the model access to external systems and data using function calling.

Nerdy Breakdown:

This endpoint was massively larger than any other OpenAI endpoint to date, rivaling the complexity of the Threads feature. This meant as the typed objects expanded we ran into issues with PHPStan due to sealed/unsealed arrays not supported alongside massive unions breaking type checking.

This module introduced @phpstan-type and @phpstan-import-type which allowed the base Type to be defined on a child object, which was imported by the parent to either use/expand. This continued all the way up to the base Response class.

Due to the name being Responses. It collided with some of the automatic logic for discovering fixtures. A few changes were made to allow this class to exist.

Finally, the streaming logic changed once again with no exact common format between Chat, Threads and now Responses. This iteration had no event, but the type of response attached in the data payload. This explains the adaption to the root parser to allow this extraction logic to work without breaking Chat & Threads.


Authored via: @momostafa & @iBotPeaches

Fixes: #535
Laravel PR: openai-php/laravel#147

Copy link
Collaborator

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started going through this, but it doesn't really follow the pattern at all :/

Some top level comments

  • Place the Contract into src/Contracts/Resources
  • Drop the Transporter/Payload iterations - you can reuse that logic.
  • Move the Response API into src/Resources
  • Split the Response/Resource into src/Resources & src/Responses
  • Introduce some heavy tests.

@momostafa
Copy link
Author

Thanks for reviewing the PR, I will go over it again and let you know

@iBotPeaches
Copy link
Collaborator

Can you run some of the tooling locally? Without tests and the bar having to be at 100% and some linting errors - this has a bit further to go.

composer test:lint
composer test:types
composer test:unit
composer test:type-coverage min=100

I put them in order of complexity.

@momostafa
Copy link
Author

Can you run some of the tooling locally? Without tests and the bar having to be at 100% and some linting errors - this has a bit further to go.

composer test:lint
composer test:types
composer test:unit
composer test:type-coverage min=100

I put them in order of complexity.

Please find attached test results, sorry I don't have time to fix all errors. Phpstan already gave me a lot of headaches.

test:unit.txt
test:types.txt
test:type-coverage min=100.txt
test:lint.txt

@momostafa
Copy link
Author

momostafa commented Mar 31, 2025

@iBotPeaches Please check my last commit 412f35c

with above commit I tested live all models and all are working.
sorry but I don't have time to go further if there are anything needed to be changed etc...
I hope someone else can carry on from where i finished.

Thank you for your understanding

@iBotPeaches
Copy link
Collaborator

@momostafa - thanks for your work so far. I triggered the pipeline so you can see the failures right now. I'll look for some time if you've tapped out to take this further.

@momostafa
Copy link
Author

@momostafa - thanks for your work so far. I triggered the pipeline so you can see the failures right now. I'll look for some time if you've tapped out to take this further.

You are welcome and thank you for your time looking into my PR. I am seriously overloaded but since there only 2 fails I will work on it tonight and I will get back to you.

@momostafa
Copy link
Author

@momostafa - thanks for your work so far. I triggered the pipeline so you can see the failures right now. I'll look for some time if you've tapped out to take this further.

Hi, Now test:lint pass on my local machine please check momostafa@29436e8
Thank you

@momostafa
Copy link
Author

@momostafa - thanks for your work so far. I triggered the pipeline so you can see the failures right now. I'll look for some time if you've tapped out to take this further.

Sorry... I will check the other failing

@momostafa
Copy link
Author

dd263ac
@iBotPeaches I have ran all tests several times and all passed 100% please check.
Thank you

Copy link
Collaborator

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer! A few things here and there, but the major aspect I see missing is the tests. You produced some fixtures, but nothing to assert any of the work you did - works.

  • You'll need a Resource test, ie like Assistants
  • You'll need a Response test for all types of responses, ie like Assistants
  • Finally, a simple test to assert the mocking/pass-through all works with a more full body Response test - ie w/ Assistants again

@momostafa
Copy link
Author

Getting closer! A few things here and there, but the major aspect I see missing is the tests. You produced some fixtures, but nothing to assert any of the work you did - works.

  • You'll need a Resource test, ie like Assistants
  • You'll need a Response test for all types of responses, ie like Assistants
  • Finally, a simple test to assert the mocking/pass-through all works with a more full body Response test - ie w/ Assistants again

Yeah getting closer : ) thank you for your patience and detailed inputs on what's needs to be fixed. I will try to resolve it today

@iBotPeaches iBotPeaches marked this pull request as draft April 8, 2025 22:32
@iBotPeaches
Copy link
Collaborator

Yeah getting closer : ) thank you for your patience and detailed inputs on what's needs to be fixed. I will try to resolve it today

No worries, I'm excited to get this as well. Thanks for continuing to work on it. I know this is a big new endpoint, which I'll probably migrate all my Chat endpoints towards once completed.

@elnurvl
Copy link

elnurvl commented Apr 10, 2025

@momostafa , thank you for this great contribution! I am also looking forward to see this merged.

Question: Does the documentation in the description reflect the current state of this PR?

In your readme file this can be found:

// Example: Stream a response
$stream = $responses->createStreamed([
    'model' => 'gpt-4o',
    'input' => [ // Changed to input to match implementation, assuming input is not a valid parameter
        [
            'content' => 'The quick brown fox jumps over the lazy fox.',
        ],
    ],
]);

According to the Responses API specification, content receives an array of inputs(e.g. text, file), not a plain string:

curl "https://api.openai.com/v1/responses" \
    -H "Content-Type: application/json" \
    -H "Authorization: Bearer $OPENAI_API_KEY" \
    -d '{
        "model": "gpt-4o",
        "input": [
            {
                "role": "user",
                "content": [
                    {
                        "type": "input_file",
                        "filename": "draconomicon.pdf",
                        "file_data": "...base64 encoded PDF bytes here..."
                    },
                    {
                        "type": "input_text",
                        "text": "What is the first dragon in the book?"
                    }
                ]
            }
        ]
    }'

Also, it is also not mentioned, what would be the implied role if it is not given in input. user?

Updating the repository README file as part of this PR would be very helpful.

I had to modify OpenAI\Testing\Responses\Concerns\Fakeable
as    $class = str_replace('Responses\\', 'Testing\\Responses\\Fixtures\\', static::class).'Fixture';
was conflicting with newly added Responses folder and added docblock explaining the modification and tested against all files.

Updated readme can be found at README-RESPONSES.md

Added dedicated ClientFake for Responses tests/Testing/ClientFakeResponses.php
@momostafa momostafa requested a review from iBotPeaches April 12, 2025 00:01
@momostafa
Copy link
Author

@momostafa , thank you for this great contribution! I am also looking forward to see this merged.

Question: Does the documentation in the description reflect the current state of this PR?

In your readme file this can be found:

// Example: Stream a response
$stream = $responses->createStreamed([
    'model' => 'gpt-4o',
    'input' => [ // Changed to input to match implementation, assuming input is not a valid parameter
        [
            'content' => 'The quick brown fox jumps over the lazy fox.',
        ],
    ],
]);

According to the Responses API specification, content receives an array of inputs(e.g. text, file), not a plain string:

curl "https://api.openai.com/v1/responses" \
    -H "Content-Type: application/json" \
    -H "Authorization: Bearer $OPENAI_API_KEY" \
    -d '{
        "model": "gpt-4o",
        "input": [
            {
                "role": "user",
                "content": [
                    {
                        "type": "input_file",
                        "filename": "draconomicon.pdf",
                        "file_data": "...base64 encoded PDF bytes here..."
                    },
                    {
                        "type": "input_text",
                        "text": "What is the first dragon in the book?"
                    }
                ]
            }
        ]
    }'

Also, it is also not mentioned, what would be the implied role if it is not given in input. user?

Updating the repository README file as part of this PR would be very helpful.

You are most welcome, I am glad to be able to make a small contribution to the community. Sorry for the delay since last update as it was quite a challenge to pass Pest tests as finally I found that fake function at Fakeable trait.
$class = str_replace('Responses\', 'Testing\Responses\Fixtures\', static::class).'Fixture';
was conflicting with the newly created folder Responses and I had to modify the function please see full details at the docblock on top of fake function OpenAI\Testing\Responses\Concerns\Fakeable::fake

I have submitted a review and I hope it will pass this time.

Thank you
I

@momostafa
Copy link
Author

@iBotPeaches Hi, Please check last commit caf4413

  • Test:lint passed
  • Test:types passed
  • Test:type-coverage min=100 passed
  • Test:unit 12 failed that I can't solve

Please review and I appreciate if you can fix what is left so we can all benefit of using the Response API.

Thank you for your understanding and guidance through the process.

@iBotPeaches
Copy link
Collaborator

Streaming should be fully typed now. I can take over whatever you have left @momostafa if you push. I have a few hours tonight that I think can finish this up.

@momostafa
Copy link
Author

Sorry for the long delay I got a little bit sick I just pushed

a86b365

@iBotPeaches
Copy link
Collaborator

Sorry for the long delay I got a little bit sick I just pushed

Ah sorry you aren't feeling well. I'll take it from here. Hopefully will get this solved today/tomorrow.

@momostafa
Copy link
Author

Sorry for the long delay I got a little bit sick I just pushed

Ah sorry you aren't feeling well. I'll take it from here. Hopefully will get this solved today/tomorrow.

Thanks, I feel better now. Anything I can help with now to speed up the process

@iBotPeaches
Copy link
Collaborator

Okay, I finished all classes. Pint/PHPStan should be 100%. Just working on 100% coverage and tests now.

@momostafa
Copy link
Author

Okay, I finished all classes. Pint/PHPStan should be 100%. Just working on 100% coverage and tests now.

Sounds great! good luck with the rest

@iBotPeaches
Copy link
Collaborator

Everything is done 💯

  • Just want to code review it again to be sure. This is a large PR, so will ping another maintainer (Nuno) when I'm done reviewing to double check.
  • Looks like a merge this weekend!

@iBotPeaches iBotPeaches marked this pull request as ready for review April 25, 2025 21:21
Copy link
Collaborator

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for jump starting this @momostafa - I'm happy with this now between our combined effort. Nearly 100 files touched and ready to roll.

I'm +1.

@momostafa
Copy link
Author

Thank you for jump starting this @momostafa - I'm happy with this now between our combined effort. Nearly 100 files touched and ready to roll.

I'm +1.

You are most welcome. Big thanks to you for doing the heavy lifting parts that I wouldn't be able to accomplish without you jumping in. It's Amazing to come that far! I am exited for the merge, I have a large project awaiting...

@SnakeO
Copy link

SnakeO commented Apr 26, 2025

Thank you both 🙏🏻 Been following your progress

@nunomaduro nunomaduro requested a review from gehrisandro April 27, 2025 12:16
@iBotPeaches
Copy link
Collaborator

Just waiting on another maintainer now. With the power outages across chunks of Europe though - tough to tell who was affected. Looking for a +1 or some feedback @gehrisandro when you have a moment.

@momostafa
Copy link
Author

Just waiting on another maintainer now. With the power outages across chunks of Europe though - tough to tell who was affected. Looking for a +1 or some feedback @gehrisandro when you have a moment.

Thanks for the feedback. I hope everyone is safe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Responses API
4 participants