Add GenerateContentRequest as an optional param to CountTokensRequest #148
Conversation
| @@ -157,7 +158,8 @@ export class GenerativeModel { | |||
| async countTokens( | |||
| request: CountTokensRequest | string | Array<string | Part>, | |||
| ): Promise<CountTokensResponse> { | |||
| const formattedParams = formatGenerateContentInput(request); | |||
| const formattedParams = formatCountTokensInput(request, this.model); | |||
| // countTokens() appends the model to the CountTokensRequest proto. | |||
There was a problem hiding this comment.
I think this comment should be for the previous line? countTokens doesn't append anything. It uses the model param to build the request url.
There was a problem hiding this comment.
It adds it to the request body, but I'll remove the comment.
There was a problem hiding this comment.
countTokens() doesn't, but formatCountTokensInput() does, right? That's why I think it would be best to attach it to the previous line? countTokens() just calls makeRequest() and returns response.json().
There was a problem hiding this comment.
countTokens() also adds it to the proto request object here.
There was a problem hiding this comment.
Hm, that seems to have been there since initial commit but I can't remember why it was needed. I don't know if countTokens is special or if I forgot to remove the model there when I removed it from generateContent and the other methods, let me double check. If it's just left over from the beginning we should probably remove it to avoid unexpected behavior.
There was a problem hiding this comment.
I don't think this blocks this PR though.
Expand the model's countTokens method to alternatively accept a GenerateContentRequest.
Added integration and unit tests.