-
Notifications
You must be signed in to change notification settings - Fork 154
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
Allow users to choose API endpoint version #56
Conversation
🦋 Changeset detectedLatest commit: e6af0be The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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.
Added two comments. LGTM thanks!
const chat = model.startChat({ | ||
generationConfig: { | ||
maxOutputTokens: 100, | ||
}, |
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.
curious, is there a reason to remove the max tokens limit here?
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.
Yeah, so this is a live backend test so it doesn't always come back with the same results so it's very loose in what it considers a success (any non-empty string). Lately, it will come back with nothing (empty string) if I set the token limit to 100, and I'm not sure any number guarantees something will come back, so I just removed it.
) {} | ||
toString(): string { | ||
let url = `${BASE_URL}/${API_VERSION}/${this.model}:${this.task}`; | ||
const apiVersion = this.requestOptions?.apiVersion || DEFAULT_API_VERSION; |
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.
Should we validate this.requestOptions?.apiVersion
to check if it is either v1
or v1beta
?
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.
No, I want to future-proof it so any version is allowed, so we don't have to update the SDK every time a new version becomes available on the backend.
@@ -28,7 +28,7 @@ export async function countTokens( | |||
params: CountTokensRequest, | |||
requestOptions?: RequestOptions, | |||
): Promise<CountTokensResponse> { | |||
const url = new RequestUrl(model, Task.COUNT_TOKENS, apiKey, false); | |||
const url = new RequestUrl(model, Task.COUNT_TOKENS, apiKey, false, {}); |
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.
Shouldn't this be the requestOptions? Otherwise the version won't get put in the url correctly.
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.
You're correct, opened an issue to fix this: #71
Add an option to
RequestOptions
so that users can choose API version. Right now the options are "v1" (default) and "v1beta".See design doc (Google internal link only) https://docs.google.com/document/d/17EccPH51QeayXPWdDx6Snz5NOLJXm4La41eEFyTXDz8/edit?tab=t.0#heading=h.ymlc40jj6ff0