Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

andrewheard
Copy link
Contributor

@andrewheard andrewheard commented Feb 28, 2025

Add support for encoding CountTokensRequest in the format required by both the Vertex AI API and the Developer API.

  • Add unit tests for encoding CountTokensRequest

#no-changelog

# Conflicts:
#	FirebaseVertexAI/Sources/CountTokensRequest.swift
#	FirebaseVertexAI/Sources/GenerativeModel.swift
@andrewheard andrewheard force-pushed the ah/dev-api-counttokens branch from 709598e to ff59d95 Compare March 12, 2025 01:12
@andrewheard andrewheard marked this pull request as ready for review March 12, 2025 01:55
Copy link

@gemini-code-assist gemini-code-assist bot left a 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 in CountTokensRequest could be made more readable by extracting the encoding logic for each service type into separate functions.
  • Redundant Properties in CountTokensRequest: The options and apiConfig properties in CountTokensRequest are now redundant as they are accessed through generateContentRequest. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant