-
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 countTokens
support for Developer API via VinF
#14644
base: main
Are you sure you want to change the base?
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
/gemini review |
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
This pull request adds support for countTokens
using the Developer API through the Vertex AI in Firebase backend. The changes include modifications to the GenerativeModel
, VertexAI
, and CountTokensRequest
classes to handle the different model resource name formats required by the various backends. Integration tests have also been updated to ensure no regressions. Overall, the changes seem well-structured and address the specific edge case described in the pull request description.
Summary of Findings
- Model Name Handling: The introduction of the
modelName
property inGenerativeModel
and its usage in constructing thegenerateContentRequestModelResourceName
seems like a good approach to handle the different model name formats. However, it's crucial to ensure that this new property is always correctly initialized and used consistently throughout the codebase. - Code Duplication: The
developerModelResourceName
function inVertexAI.swift
has similar logic for.firebaseVertexAIStaging
and.firebaseVertexAIProd
. Consider refactoring to reduce code duplication. - Test Coverage: The integration tests have been updated, but it's important to ensure that all edge cases and different configurations are thoroughly tested to prevent regressions.
- Documentation: The documentation for the
modelResourceName
parameter in theGenerativeModel
initializer is comprehensive and helpful. Ensure that all public APIs are well-documented.
Merge Readiness
The pull request introduces important functionality for handling a specific edge case with the countTokens
API. The code appears to be well-structured and addresses the issue effectively. However, before merging, it's recommended to address the potential code duplication in developerModelResourceName
and ensure comprehensive test coverage for all configurations. I am unable to directly approve this pull request, and other reviewers should review and approve this code before merging.
let countTokensRequest = CountTokensRequest( | ||
modelResourceName: modelResourceName, generateContentRequest: generateContentRequest | ||
) |
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.
@@ -102,7 +102,7 @@ struct CountTokensIntegrationTests { | |||
|
|||
@Test(arguments: [ | |||
/* System instructions are not supported on the v1 Developer API. */ | |||
InstanceConfig.developerV1, | |||
InstanceConfig.developerV1Spark, |
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.
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.
For reference in this PR review, developerV1
and developerV1Beta
now use the Vertex AI in Firebase backend, which doesn't support the Developer API on v1
. developerV1Spark
talks directly to the Generative Language backend.
/// The form depends on the backend and will be one of: | ||
/// - Vertex AI via Vertex AI in Firebase: | ||
/// `"projects/{projectID}/locations/{locationID}/publishers/google/models/{modelName}"` | ||
/// - Developer API via Vertex AI in Firebase: `"projects/{projectID}/models/{modelName}"` |
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.
Lines 64 and 65 are confusing to me. Let's discuss.
Added support for
countTokens
using the Developer API through the Vertex AI in Firebase (firebasevertexai.googleapis.com
) backend. This combination is an edge case where the model resource name (e.g.,projects/my-project/models/gemini-2.0-flash
) in thecountTokens
request URL differs from the resource name in the request fieldCountTokensRequest.generateContentRequest.name
. The integration tests have been updated to perform acountTokens
request using bothfirebasevertexai.googleapis.com
(Vertex AI and Developer API) andgenerativelanguage.googleapis.com
(Developer API only) to ensure no regressions.#no-changelog