Dedicated exceptions for specific events

[kikaragyozov]

I see there's some code that's been added that populates the headers for rate limiting, but they're unusable as our current implementations require a successful status code response before we set the response data. If the response is not a success status code (200-299) we throw an exception.

Currently the BaseResponse looks the following way.

I suggest if we do decide to tackle this, to create a custom OpenAIClientRateLimitException that end-users can inspect and access all those related headers, which should also end up cleaning the BaseResponse a bit. The cleaning part though would introduce breaking changes if we do decide to follow through with it.

Alternative but much more destructive, would be to move all of the properties related to rate limiting from BaseResponse in their own class and remove their nullability - if the object exists, then they too exist - and possibly make BaseResponse generic to encapsulate the actual response received from Open AI, along with anything else we deem worthy.

This makes using the API a bit more cumbersome and entirely changes how users interact with it. You'd need to access 1 more property to get to what you're most interested it. The benefits are that it removes the need for throwing exceptions for normal execution flow, as well as users writing try/catches for normal execution flow. There's nothing uncommon or exceptional about requests with a status code of 400-499. A 500+ status code is an exceptional event though, as it indicates an unexpected server-side error and the client can do nothing to change its outcome.

First approach is much more manageable for implementation. It all depends on how much we value performance, versus simplicity and in which direction we should focus our time and effort on.

P.S: I'm willing to help out with anything you guys decide to do on this.

[StephenHodgson]

There's nothing uncommon or exceptional about requests with a status code of 400-499. A 500+ status code is an exceptional event though, as it indicates an unexpected server-side error and the client can do nothing to change its outcome.

any exception is exceptional. Idk personally I like try/catch as it gives me more control about how I want to handle any situation where I need to make a choice about how to handle it. Gives more power to the consumer of the library.

[StephenHodgson]

It all depends on how much we value performance, versus simplicity

It's a remote resource call, not sure we need the performance imho

[StephenHodgson]

tbh we need to have a more dedicated exception for the library itself that contains the data about the errors, data, etc when certain calls don't finish successfully. Low priority imho.

[StephenHodgson]

I vote no on the Alternative, just to be clear

[kikaragyozov]

I vote no on the Alternative, just to be clear

I'm not convinced the exception route is good in the long run. Handling 404 not found as an exception in a REST HTTP API world by a HTTP API library rather than returning a null object as a response to the consumers, "translating" if you may say the HTTP REST world to the .NET world, is much more appealing and manageable both for us and them.

Would you be okay if I tackled support for 429 via an exception while also addressing the 404 responses by returning a null object instead of throwing? I know it might look weird to start doing it both ways, but this way we're striking a good balance between ease of use and not overly complicated API design.

Do you agree? It's just very strange for me to get an exception when requesting a given entity, but that entity is no longer there, i.e it was deleted by someone else. I'd expect as a consumer to simply receive a null object as a response and then I'd know what's up and deal with it appropriately, as null is exactly for that - nothing was found, which is what 404 signifies in the HTTP world.

[StephenHodgson]

I don't understand why try/catch is a overcomplicated use of a native language feature.

[StephenHodgson]

I'm sorry but you're not going to convince me to change it to be this way 😅

[kikaragyozov]

I don't understand why try/catch is a overcomplicated use of a native language feature.

If network streams were throwing exceptions every time they encountered a null byte, code would look terrible for those working with them.

What is the point of a .NET library whose whole purpose is to communicate with an HTTP REST API?
To translate those calls into .NET managed objects, correct?

Microsoft has done their due with HttpClient. How does OpenAIClient look compared?

In my opinion, the library does a lot of good things but only handles a small segment of the possibilities Open AI can throw at them. Currently, it handles the absolute happy-path - 2XX responses. Everything else is an "exceptional event". Does this look like a proper HTTP translation to you? It's not even close! It covers a very small spectrum of what REST is, It's as if Microsoft made HttpClient handle only POST requests.

To further bring emphasis on this: In the context of HTTP and RESTful APIs, a status code of 404 (Not Found) conventionally implies the absence of a resource. It is a standard and expected part of web communication, not an exceptional or error condition. Thus, returning a null object in response to a 404 status code aligns with typical HTTP client expectations, and is a proper translation to a .NET model.

There are other status codes that might warrant integration, such as 403 and 401, but I digress. The most basic thing we can put our efforts into properly translating, would be a 404.

[StephenHodgson]

I don't think your analogy is exactly 1:1.

I understand where you're coming from but at the same time I want this library to be as slim as possible, call giving developers options on how they want to handle the return codes.

[kikaragyozov]

I don't think your analogy is exactly 1:1.

I understand where you're coming from but at the same time I want this library to be as slim as possible, call giving developers options on how they want to handle the return codes.

If OpenAI did not conform to the REST world, they'd just return you a null application/json response for the resource you're requesting (if it did not exist) with a 200 status code. Would you still throw an exception then?

[kikaragyozov]

I'm sorry if some of my points came across the wrong way. I'm trying to give my thoughts as I'd love to see this library grow and become amazing and hopefully get brought over to OpenAI's foundation (which I'm sure they'll appreciate if we do a proper translation without cutting corners). If you disagree with me that's completely fine. Let me know if you still plan on handling the rate limiting via an exception or not, seeing as you closed the issue completely @StephenHodgson .

[StephenHodgson]

Accidentally closed the issue

[StephenHodgson]

I think OpenAI gives us a nice class to wrap most errors around. Gonna stick to that one.