-
Notifications
You must be signed in to change notification settings - Fork 747
cleanup vertex api.ts #1350
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
base: main
Are you sure you want to change the base?
cleanup vertex api.ts #1350
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
export class GatewayError extends Error { | ||
constructor( | ||
message: string, | ||
public status: number = 500, | ||
public cause?: Error | ||
) { | ||
super(message); | ||
this.name = 'GatewayError'; | ||
this.status = status; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -809,7 +809,7 @@ export async function tryTargetsRecursively( | |||||
message: errorMessage, | ||||||
}), | ||||||
{ | ||||||
status: 500, | ||||||
status: error instanceof GatewayError ? error.status : 500, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Hardcoded error status code
Suggested change
|
||||||
headers: { | ||||||
'content-type': 'application/json', | ||||||
// Add this header so that the fallback loop can be interrupted if its an exception. | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,8 +1,9 @@ | ||||||||||||||
import { GatewayError } from '../../errors/GatewayError'; | ||||||||||||||
import { Options } from '../../types/requestBody'; | ||||||||||||||
import { endpointStrings, ProviderAPIConfig } from '../types'; | ||||||||||||||
import { getModelAndProvider, getAccessToken, getBucketAndFile } from './utils'; | ||||||||||||||
|
||||||||||||||
const getApiVersion = (provider: string, inputModel: string) => { | ||||||||||||||
const getApiVersion = (provider: string) => { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Unused parameter
Suggested change
|
||||||||||||||
if (provider === 'meta') return 'v1beta1'; | ||||||||||||||
return 'v1'; | ||||||||||||||
}; | ||||||||||||||
|
@@ -22,7 +23,7 @@ const getProjectRoute = ( | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
const { provider } = getModelAndProvider(inputModel as string); | ||||||||||||||
let routeVersion = getApiVersion(provider, inputModel as string); | ||||||||||||||
const routeVersion = getApiVersion(provider); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Unused parameter in function call
Suggested change
|
||||||||||||||
return `/${routeVersion}/projects/${projectId}/locations/${vertexRegion}`; | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
|
@@ -58,8 +59,9 @@ export const GoogleApiConfig: ProviderAPIConfig = { | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (vertexRegion === 'global') { | ||||||||||||||
return `https://aiplatform.googleapis.com`; | ||||||||||||||
return 'https://aiplatform.googleapis.com'; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return `https://${vertexRegion}-aiplatform.googleapis.com`; | ||||||||||||||
}, | ||||||||||||||
headers: async ({ c, providerOptions, gatewayRequestBody }) => { | ||||||||||||||
|
@@ -68,7 +70,6 @@ export const GoogleApiConfig: ProviderAPIConfig = { | |||||||||||||
if (vertexServiceAccountJson) { | ||||||||||||||
authToken = await getAccessToken(c, vertexServiceAccountJson); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
const anthropicBeta = | ||||||||||||||
providerOptions?.['anthropicBeta'] ?? | ||||||||||||||
gatewayRequestBody?.['anthropic_beta']; | ||||||||||||||
|
@@ -95,6 +96,9 @@ export const GoogleApiConfig: ProviderAPIConfig = { | |||||||||||||
mappedFn = `stream-${fn}` as endpointStrings; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
const url = new URL(gatewayRequestURL); | ||||||||||||||
const searchParams = url.searchParams; | ||||||||||||||
|
||||||||||||||
if (NON_INFERENCE_ENDPOINTS.includes(fn)) { | ||||||||||||||
const jobIdIndex = [ | ||||||||||||||
'cancelBatch', | ||||||||||||||
|
@@ -106,9 +110,9 @@ export const GoogleApiConfig: ProviderAPIConfig = { | |||||||||||||
const jobId = gatewayRequestURL.split('/').at(jobIdIndex); | ||||||||||||||
|
||||||||||||||
const url = new URL(gatewayRequestURL); | ||||||||||||||
const searchParams = url.searchParams; | ||||||||||||||
const pageSize = searchParams.get('limit') ?? 20; | ||||||||||||||
const after = searchParams.get('after') ?? ''; | ||||||||||||||
const params = new URLSearchParams(url.search); | ||||||||||||||
const pageSize = params.get('limit') ?? 20; | ||||||||||||||
const after = params.get('after') ?? ''; | ||||||||||||||
|
||||||||||||||
let projectId = vertexProjectId; | ||||||||||||||
if (!projectId || vertexServiceAccountJson) { | ||||||||||||||
|
@@ -147,9 +151,15 @@ export const GoogleApiConfig: ProviderAPIConfig = { | |||||||||||||
case 'cancelFinetune': { | ||||||||||||||
return `/v1/projects/${projectId}/locations/${vertexRegion}/tuningJobs/${jobId}:cancel`; | ||||||||||||||
} | ||||||||||||||
default: | ||||||||||||||
return ''; | ||||||||||||||
Comment on lines
+154
to
+155
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Missing default case in switch statement
Suggested change
|
||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (!inputModel) { | ||||||||||||||
throw new GatewayError('Model is required', 400); | ||||||||||||||
} | ||||||||||||||
Comment on lines
+159
to
+161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security Issue Fix Issue: Missing model validation could lead to unexpected behavior
Suggested change
|
||||||||||||||
|
||||||||||||||
const { provider, model } = getModelAndProvider(inputModel as string); | ||||||||||||||
const projectRoute = getProjectRoute(providerOptions, inputModel as string); | ||||||||||||||
const googleUrlMap = new Map<string, string>([ | ||||||||||||||
|
@@ -188,6 +198,7 @@ export const GoogleApiConfig: ProviderAPIConfig = { | |||||||||||||
} else if (mappedFn === 'messagesCountTokens') { | ||||||||||||||
return `${projectRoute}/publishers/${provider}/models/count-tokens:rawPredict`; | ||||||||||||||
} | ||||||||||||||
return `${projectRoute}/publishers/${provider}/models/${model}:rawPredict`; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Missing default return in switch statement
Suggested change
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
case 'meta': { | ||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: Redundant assignment of
status
property in constructorFix: Remove redundant assignment since it's already initialized in parameter
Impact: Cleaner code, no functional change