Add a new VertexAI error type #8240
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: d71e71d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
dlarocque
force-pushed
the
dlarocque/vertexaierror
branch
from
7594331
to
5fb071b
Compare
hsubox76
reviewed
May 10, 2024
packages/vertexai/src/types/error.ts
Outdated
| * | ||
| * @public | ||
| */ | ||
| export class VertexAIError extends FirebaseError { |
There was a problem hiding this comment.
We shouldn't have any code in types - for style purposes and it also causes some obscure bugs during build sometimes. Let's move this to src/errors.ts.
There was a problem hiding this comment.
We still want users to be able to import VertexAIError, so I moved the class implementation to src/errors.ts, but am still exporting it from types. Does this approach still cause some bugs during build times?
There was a problem hiding this comment.
Can't we export it out of api.ts or index.ts? I guess I'd prefer api.ts since there's a few class exports there already.
There was a problem hiding this comment.
We don't currently export any classes from api.ts- we only have getVertexAI(), and getGenerativeModel(). I feel like it's a bit odd to export an error alongside those. Could we add export './errors' to index.ts?
packages/vertexai/src/types/error.ts
Outdated
| * @param errorDetails - Optional additional details about the error. | ||
| */ | ||
| constructor( | ||
| readonly code: VertexAIErrorCode, |
There was a problem hiding this comment.
Realized the constructor is already kind of long and only going to get longer as we think of more properties to add. (We already have to add "response" as mentioned in another comment.) Maybe we can put all the optional properties into an object, so it's constructor(code, message, customErrorData) where customErrorData is an object with all the optional properties.
There was a problem hiding this comment.
Agreed! since status, statusText, and errorDetails are used only when throwing on HTTP responses, I moved them into a HTTPErrorDetails interface, and accept that as a parameter instead.
New constructor:
constructor(
readonly code: VertexAIErrorCode,
readonly message: string,
readonly httpErrorDetails?: HTTPErrorDetails,
readonly generateContentResponse?: GenerateContentResponse
)New error declaration with http details:
throw new VertexAIError(
VertexAIErrorCode.FETCH_ERROR,
`Error fetching from ${url}: [${response.status} ${response.statusText}] ${message}`,
{
status: response.status,
statusText: response.statusText,
errorDetails
}
);There was a problem hiding this comment.
I think we should future proof it by making it not HTTPError specific. We might put other details in here in the future not related to HTTP, I don't think we should then create a separate arg for that. I think response should go in here too.
There was a problem hiding this comment.
Sounds good! I renamed httpErrorData to customErrorData, made all the properties optional, and moved generateContentResponse into it.
Since we're making all these properties optional and we expect to add more in the future, I am a little worried our error declarations will start looking like this:
throw new VertexAIError(
VertexAIErrorCode.FETCH_ERROR,
`Error fetching from ${url}: [${response.status} ${response.statusText}] ${message}`,
{
undefined, // unused `status`. must be undefined to "skip over" to the field we want to include
undefined, // unused `statusText`
undefined, // unused `response`
undefined, // unused `customErrorData`
newErrorField
}
);
dlarocque
force-pushed
the
dlarocque/vertexaierror
branch
from
3b4301a
to
c3a3037
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
VertexAIErrorso that users caninstanceofthe errors from the VertexAI SDK.status,statusText,errorDetails) (mirroring google-gemini/generative-ai-js@111e970)ErrorFactorytowards our new error typeVertexAIErrorCode.ERRORto align with the generic error in the Google AI SDKGoogleGenerativeAIErrorTesting
I tested this in a React application in Chrome and Safari, and in a Node project.
instanceofand the stack trace works in these environments.Error output in different environments
Safari
Chrome
Node