Skip to content
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

[Vertex AI] Add CustomNSError conformance to BackendError #14029

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

andrewheard
Copy link
Contributor

@andrewheard andrewheard commented Nov 5, 2024

Added CustomNSError conformance to BackendError. This improves the string returned by localizedDescription and will also improve interoperability with Crashlytics (e.g., when reporting non-fatal errors, logged errors are grouped by domain and code).

Example - calling error.localizedDescription in testGenerateContent_failure_invalidAPIKey():

  • Before: The operation couldn’t be completed. (FirebaseVertexAI.BackendError error 1.)
  • Now: "API key not valid. Please pass a valid API key. (com.google.firebase.vertexai.BackendError - HTTP 400 INVALID_ARGUMENT)"

#no-changelog

@andrewheard andrewheard added this to the 11.5.0 - M156 milestone Nov 5, 2024
@andrewheard andrewheard marked this pull request as ready for review November 5, 2024 15:20
Copy link
Member

@ncooke3 ncooke3 left a comment

Choose a reason for hiding this comment

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

Nice improvement. For future consideration, do you think more of our Swift errors should conform to NSCustomError? It seems odd that there isn't a more obvious Swift type to do this. I looked at the LocalizedError protocol, but it doesn't look like an equally good fit.

@andrewheard
Copy link
Contributor Author

Nice improvement. For future consideration, do you think more of our Swift errors should conform to NSCustomError? It seems odd that there isn't a more obvious Swift type to do this. I looked at the LocalizedError protocol, but it doesn't look like an equally good fit.

@ncooke3 That's a good question. I haven't figured out a good rule of thumb as to whether to conform to NSCustomError. In some cases like error enums the code comes from the enum case index and the domain comes from the type name (both are typically reasonable). I think Swift devs often ignore those too.

In this particular scenario, since BackendError is an internal type, I wanted to make it as easy as possible to extract the useful debugging info since it's most commonly thrown in wrapped form:

public enum GenerateContentError: Error {
/// An internal error occurred. See the underlying error for more context.
case internalError(underlying: Error)

It's also open for debate whether to implement LocalizedError (or NSLocalizedDescriptionKey in the userInfo dictionary) in cases like this where we aren't actually providing a localized error (i.e., it's purely English). My argument here was that auto-generated message The operation couldn’t be completed. wasn't very useful, even though it would be translated. I don't think devs would want to display this error (before or after) directly in their UIs.

That's my brain dump so far but would be good to figure out a set of guidelines for the future.

@andrewheard andrewheard merged commit db87a53 into main Nov 5, 2024
38 checks passed
@andrewheard andrewheard deleted the ah/vertex-backend-nserror branch November 5, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants