[FEATURE REQ] Different exception for different errors
verdie-g opened this issue · 14 comments
Describe the feature or improvement you are requesting
Today, the client only throws a single type of exception System.ClientModel.ClientResultException and fits everything in the message property so it's hard to handle them properly. It would be great to have one exception type by error type like it's done in the python library.
Additional context
No response
Hi @verdie-g. Thank you for your reaching out and for your feedback. The error patterns in the library are intentional and based on general .NET design guidelines for exceptions and standard C# error patterns used by framework types such as HttpClient. We have no intention on creating a custom convention specific to the OpenAI library.
Guidance for System.ClientModel error handling is discussed in the Handling Exceptions section of the System.ClientModel-based client service method samples.
These guidelines don't seem to prevent having a child exception of ClientResultException.
To give a concrete example, when a model is not found, this exception is thrown
Status: 404
Message: HTTP 404 (: DeploymentNotFound)
The API deployment for this resource does not exist. If you created the deployment within the last 5 minutes, please wait a moment and try again.
Having the error type in its own field and the status code out of the message would help dealing with them. Today the exception is not exploitable.
Per design guidelines, we create new exception types only if there is a programmatic way of handling the error. @verdie-g, do you think there is a way to handle the "model/deployment not found" error? I cannot think of what such handler would do.
we create new exception types only if there is a programmatic way of handling the error
Fair enough.
do you think there is a way to handle the "model/deployment not found" error?
Maybe it's not the best example, but could be anything honestly. If we get this error, there might be something very wrong with our setup and we would want to raise the issue in a different way.
I don't have a better example for now. To talk about more about my specific problem, I would like to display the error in a better formatting, that is, the message without the "HTTP 404 (: DeploymentNotFound)" but still being able to get the error type (DeploymentNotFound).
If it's about formatting a custom message, can you use the ErrorCode property?
No, I mean ErrorCode property on the exception
I don't know this exception. Let me check again tomorrow I thought the client only threw ClientResultException.
Ah, sorry. I mixed things up. You are indeed getting ClientResultExceptions, not RequestFailedExceptions. and ClientResultException does not have this property. Let me dig into why.
Ok, here is how to access the code:
try {
...
}
catch (ClientResultException e){
PipelineResponse? response = e.GetRawResponse();
string? error = response?.ReasonPhrase;
}Thanks, and is there a way to get the message without the "HTTP 404 (: DeploymentNotFound)" just "The API deployment for this resource does not exist..."?
Not sure, but thry this:
try {
...
}
catch (ClientResultException e){
PipelineResponse? response = e.GetRawResponse();
string? message = response?.Content.ToString();
}Weird way to answer :D
.GetRawResponse()!.Content.ToString()
returns
{
"error" : {
"code" : "DeploymentNotFound",
"message" : "The API deployment for this resource does not exist. If you created the deployment within the last 5 minutes, please wait a moment and try again."
}
}
I'm not sure I would be comfortable parsing a JSON to get an error message.
I'm not sure I would be comfortable parsing a JSON to get an error message.
Why not? What's makes you not comfortable here?
The above shows how to implement the scenario. We cannot add exception types for all the reason codes as this would complicate the library surface area for a relatively corner case scenario while the scenario can be implemented using the solution discussed above.