Skip to content

fix: Async response not instanceof ImageResponse in example #43

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

Closed
wants to merge 1 commit into from

Conversation

smnandre
Copy link
Member

Q A
Bug fix? yes
New feature? no
Docs? no
Issues Fix #...
License MIT

In examples/openai/image-output-dall-e-3.php

assert($response instanceof ImageResponse);   //  ❌

The response is an AsyncResponse that wraps a ImageResponse (and forwards methods and properties via __call and __get) but the assertion fails.

Not sure how to explain this simply in the example code.

```php
assert($response instanceof ImageResponse);   //  ❌
```

The response is an AsyncResponse that wraps a ImageResponse (and
forwards methods and properties via __call and __get) but the assertion fails.

Not sure how to show this simply in the example code.
@chr-hertel
Copy link
Contributor

Yes, that's a valid concern - and with some spoilers, it will happen over and over when working with the lib in user land. it's a bit annoying how the AsyncResponse is currently wrapped around the others. but i guess i was too lazy in php-llm/llm-chain#141

what i'm doing from time to time:

$response = $platform->request(...);
assert($response instance of AsyncResponse);
$response = $response->unwrap();
assert($response instance of ImageResponse);

which is not great DX at all

so your finding here is only the tip of the ice berg and what i listed in #41 as

Better type support for user land (.e.g. $platform->requestText(...) or $platform->request(...)->asText())

what do you think about the option to call the Platform like this to make sure to have the actual ImageResponse instead of the AsyncResponse:
a)

$response = $platform->requestImage(
    model: new DallE(name: DallE::DALL_E_3),
    input: 'A cartoon-style elephant with a long trunk and large ears.',
    options: [
        'response_format' => 'url', // Generate response as URL
    ],
);

b)

$response = $platform->request(
    model: new DallE(name: DallE::DALL_E_3),
    input: 'A cartoon-style elephant with a long trunk and large ears.',
    options: [
        'response_format' => 'url', // Generate response as URL
    ],
)->asImage();

@smnandre
Copy link
Member Author

smnandre commented Jul 1, 2025

$response = $platform->requestImage(

Readable and coherent today... but would that scale in the next few months ?

)->asImage();

Same problem in a way ?

--

I have not enough vision on all the work done here, and still some code and articles/resources to read to have any valid opinion i think..

...but as you asked 😅
my two cents would be: i like the Process api a lot, because one can explicitely call in async or sync.

So what about $platform->asyncRequest(...) / request(...)

(i do not like the "name" asyncRequest but you see the point i guess :) )

One would return an ImageResponse, the other a Promise in a way..

@chr-hertel
Copy link
Contributor

the $platform->asyncRequest(...) / request(...) wouldn't solve that i can't expect the type on user side.

but you're also right that the other options have a scaling issue ... 🤔

WDYT about php-llm/llm-chain#376

@chr-hertel
Copy link
Contributor

Closed in favor of #73 - not needed anymore

@chr-hertel chr-hertel closed this Jul 6, 2025
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.

2 participants