-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Developer API encoding CountTokensRequest
#14512
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # FirebaseVertexAI/Sources/CountTokensRequest.swift # FirebaseVertexAI/Sources/GenerativeModel.swift
709598e
to
ff59d95
Compare
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.
Code Review
The pull request introduces support for encoding CountTokensRequest
for both Vertex AI API and Developer API. The changes involve modifying the CountTokensRequest
and GenerateContentRequest
structures and adding a new integration test. Overall, the code seems well-structured and addresses the objective. However, there are a few areas that could be improved for clarity and maintainability.
Summary of Findings
- Encoding Logic in
CountTokensRequest
: The encoding logic inCountTokensRequest
could be made more readable by extracting the encoding logic for each service type into separate functions. - Redundant Properties in
CountTokensRequest
: Theoptions
andapiConfig
properties inCountTokensRequest
are now redundant as they are accessed throughgenerateContentRequest
. Consider removing them to simplify the struct. - Missing Unit Tests: There are no unit tests for the encoding logic in
CountTokensRequest
. Adding unit tests would ensure that the encoding is correct for both Vertex AI and Developer APIs.
Merge Readiness
The pull request is almost ready for merging. However, I recommend addressing the feedback provided, particularly regarding the encoding logic in CountTokensRequest
and adding unit tests. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Addressing the high severity issues is crucial before merging.
Add support for encoding
CountTokensRequest
in the format required by both the Vertex AI API and the Developer API.CountTokensRequest
#no-changelog