Introduce OpenAIThrowable

[VincentLanglet]

What:

• Bug Fix
• New Feature

Description:

Hi @gehrisandro

I'm trying to move #287 forward by splitting it in 3 smaller PRs:

• First PR to introduce OpenAIThrowable
• Second PR to introduce UnreadableResponse exception
• Third PR with only Phpdoc update

The purpose of this PR is to provide an easy way to try/catch method of this lib:

• It avoids catching \Exception which catch everything
• It avoids catching OpenAI\Exceptions\ErrorException|OpenAI\Exceptions\InvalidArgumentException|... which is complicated and can miss an exception if a new one is introduced later.

Related:

#287

[VincentLanglet]

Friendly ping @nunomaduro @gehrisandro,

Is something missing for a review ?
Since #287 got interest but no reaction,
I'm trying to moving it forward with smaller PR. Please tell me if I can do it better :)

Thanks

[bytestream]

I agree that this is a useful and beneficial improvement. I see no reason why this cannot be merged @nunomaduro

[VincentLanglet]

Anyway to move forward this PR @gehrisandro @nunomaduro ?

Thanks a lot

[iBotPeaches]

Sorry for delay. I took a look at this and outside of the linting failing - I'm not really following this implementation.

• It appears to introduce a custom interface (OpenAIThrowable) that extends Throwable
• All classes then implement that interface.

Could you give any background to help me learn why you took that route instead of say like a base class of like OpenAiException that all these others extended from, which then extended Exception.

It seems like on a face value there is just a bit of workarounds here to avoid this callout.

PHP classes cannot implement the Throwable interface directly, and must instead extend Exception.

I want to be sure I'm understanding any trade-offs properly.

[VincentLanglet]

Hi @iBotPeaches, thanks for your review.

My main goal is

• to update the phpdoc from the method of this repository to add @throws
• Regroup all the exceptions with a base class/interface in order to be able to catch (BaseClassOrInterface $e) {}

I mostly saw in such situation the following strategy:

• Introducing an interface (or many) like https://github.com/symfony/symfony/tree/7.3/src/Symfony/Contracts/HttpClient/Exception
• then, implementing this interface https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/HttpClient/Exception/ClientException.php

Introducing an Interface offers more flexibility because I can write

- MyLogicException extends \LogicException implements CustomThrowable
- MyRuntimeException extends \RuntimeException implements CustomThrowable

For instance, and would allow to

• Just catch CustomThrowable
• Just catch RuntimeException instead

Notice I can also implements multiple interfaces when I cannot extends multiple classes. So it offer more flexibility.

But I know some repository prefer to introduce a BaseException (or used this strategy because it was introduced before the Throwable interface exists in PHP). If it's the preferred choice from OpenAI, I won't go against it if it the preferred choice.

#287 didn't get any review in 2 years and this PR is waiting for 1 year so I'd like to move on this in order to "fix" the phpdoc and adding the @throws annotation :)

[VincentLanglet]

@iBotPeaches I rebased so you can approval the workflow run.

Also I'll wait for your decision if you prefer a baseException or an Interface

[iBotPeaches]

I kicked off the workflows and I don't have a strong opinion on base vs interface. Let me do some research though to double check if I'm missing anything.

[iBotPeaches]

I took a look at your game plan and have a few thoughts I'm looking for clarity on.

• I grepped codebase and I see @throws 3 times, attached right to the agnostic HTTP client implementation for all the ways this can fail. The classes after this change would have the interface and for folks that want to just to catch 1 thing - they can still do that. So maybe you add the interface for documentation into the tag, but that seems weird right? You'd have the base interface and then the concrete implementations which extends that base, which still feels weird. You wouldn't add throw tags with the unified interface to all methods, because it would be invalid. Like the stream methods can only throw a subset of the errors that the non-streaming can.

• If you are just changing your code to capture any and all OpenAI Exception (this new thing) - you are probably the same type of engineer to just capture any exception during the execution of this library.

• UnreadableResponse exception - what is use-case for this? I'd argue the issues we have right now is the code is actually crashing when its not json. I'd argue that its "UnserializableException", but we can't even throw it because we crash out (null, string, etc) before getting there.

I wanted to give this PR the time it deserved after waiting a solid few years, but with the bugs and missing parity with new OpenAI APIs - unfortunately at the moment re-hauling the exceptions/throws which can have a direct impact on strict tooling for user-land code does not seem like something I should focus at the moment.

[VincentLanglet]

• UnreadableResponse exception - what is use-case for this? I'd argue the issues we have right now is the code is actually crashing when its not json. I'd argue that its "UnserializableException", but we can't even throw it because we crash out (null, string, etc) before getting there.

It was made to catch

$response->getBody()->getContents();

https://github.com/openai-php/client/pull/287/files#diff-0344a87ebdb10e227d2ba6b93d934fe88e0641c95e5d443f90436e3c8972cab9R54-R58

If we introduce an interface or a baseException, to allows people catching it rather than

catch (ExceptionA|ExceptionB|ExceptionC|...) {}

people will still need to be aware of the implementation of OpenAIThrowable, cause it might throw a RuntimeException.

So instead it's recommended to throw a Domain-related Exception.

If you are just changing your code to capture any and all OpenAI Exception (this new thing) - you are probably the same type of engineer to just capture any exception during the execution of this library. I'm sure a

"I'm sure a", is it missing the end of the sentence @iBotPeaches ?

I grepped codebase and I see @throws 3 times, attached right to the agnostic HTTP client implementation for all the ways this can fail. The classes after this change would have the interface and for folks that want to just to catch 1 thing - they can still do that. So maybe you add the interface for documentation into the tag, but that seems weird right? You'd have the base interface and then the concrete implementations which extends that base, which still feels weird. You wouldn't add throw tags with the unified interface to all methods, because it would be invalid. Like the stream methods can only throw a subset of the errors that the non-streaming can.

If you prefer I can add specific @throws tag everywhere without introducing the interface. My Main goal is to have a well documented @throws tag and it's way easier when you use an interface.

But if you prefer having

@throws ErrorException
@throws InvalidArgumentException
@throws TransporterException
@throws UnserializableResponse
@throws RuntimeException

almost everywhere rather than @throws OpenAiThrowable I can do the PR.

[bytestream]

you are probably the same type of engineer to just capture any exception during the execution of this library.

They're very different things catching OpenAIThrowable and \Throwable or \Exception. The interface is most beneficial when there's a method that's going to throw multiple exceptions defined in this library. It's excessive to define each individually in a catch, when realistically the catch behaviour is most likely the same in all scenarios that this lib throws.

[iBotPeaches]

"I'm sure a", is it missing the end of the sentence @iBotPeaches ?

hmm lol. Not sure what thought I had going there. Removed it

[VincentLanglet]

• Like the stream methods can only throw a subset of the errors that the non-streaming can.

But are you sure it's really true ?

When looking at the interface it currently say that the mehtod only throw ErrorException, which is indeed a subset of
ErrorException|UnserializableResponse|TransporterException

client/src/Contracts/TransporterContract.php

Line 38 in dadf819

* @throws ErrorException

But when looking at the implementation, sendRequest is called

client/src/Transporters/HttpTransporter.php

Line 92 in dadf819

$response = $this->sendRequest(fn () => ($this->streamHandler)($request));

and this method is throwing TransporterException, then throwIfJsonError is called

client/src/Transporters/HttpTransporter.php

Line 94 in dadf819

$this->throwIfJsonError($response, $response);

and this method does throw ErrorException or UnserializableResponse.

So

• All the method from transporter are throwing ErrorException|UnserializableResponse|TransporterException
• And then all the method from resources are doing it too

Throwing an Interface/BaseException is also a way to simplify the maintenance of what is exactly thrown.
But at least it will avoids interface like

client/src/Contracts/Resources/VectorStoresContract.php

Line 9 in dadf819

interface VectorStoresContract

where we can think (and the static analysis tool too) that no exception are thrown.

[VincentLanglet]

If preferred @iBotPeaches, I can open a PR with just all the @throws added (and validated by PHPStan)
https://github.com/openai-php/client/compare/main...VincentLanglet:client:phpdoc?expand=1

[iBotPeaches]

At moment - no need @VincentLanglet. I am supposed to start small as a collaborator and touching ~30 files with throws when this PR wasn't really visited by the other maintainers. I want to start smaller before tackling exceptions.

[bytestream]

Am I missing something? The PR changes 6 files and effects no one from a BC perspective, it literally just adds an interface.

The primary sticking point seems to be interface vs base class, and I don't think there's a particularly hard opinion either way.

[iBotPeaches]

I guess my confidence is the only thing. For all the other PRs thus far I can sit down and replicate a bug / test a feature. Here I am traveling through more of a design pattern than a bug/feature in my understanding.

Thus with a discussion of patterns (throwable/exception) and preference (interface/base exception) both which are a jumping point for further changes I wanted to type it out a bit to try and understand why this PR sat.