-
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 APIConfig
to userInfo
dictionary in coders
#14592
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. |
This comment was marked as resolved.
This comment was marked as resolved.
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 introduces the APIConfig
to the JSONEncoder
and JSONDecoder
used in GenerativeAIService
. This allows the APIConfig
to be accessible during encoding and decoding of request/response types, which simplifies future decoding of CitationMetadata
. The changes look good overall, with a few minor suggestions for improvement.
Summary of Findings
- Force-unwrapping URLs: The code uses force-unwrapping when creating URLs, which can lead to runtime crashes if the URL string is invalid. Consider using optional binding or
URL(string:)
to handle potential nil values gracefully. - Fatal errors: The code uses
fatalError
in several places when theAPIConfig
is not found in the coder'suserInfo
. While this is acceptable for development, consider using a more graceful error handling mechanism in production code. - Redundant decoder configuration: The code initializes a
JSONDecoder
withkeyDecodingStrategy = .convertFromSnakeCase
which is no longer needed since theAPIConfig
is now being passed in.
Merge Readiness
The pull request is almost ready for merging. Before merging, I recommend addressing the force-unwrapping of URLs and considering a more graceful error handling mechanism than fatalError
. I am unable to directly approve this pull request, and users should have others review and approve this code before merging.
FirebaseVertexAI/Sources/Types/Internal/Imagen/ImagenGenerationRequest.swift
Show resolved
Hide resolved
FirebaseVertexAI/Sources/Types/Internal/Requests/CountTokensRequest.swift
Show resolved
Hide resolved
@paulb777 I thought about this a bit more and realized that it would be a breaking change for anyone that is using our public |
Added the
APIConfig
to theJSONEncoder
andJSONDecoder
used inGenerativeAIService
. TheAPIConfig
is accessible via the computed propertyapiConfig
during encoding and decoding of request / response types. This will simplify future decoding ofCitationMetadata
, which is deeply nested in theGenerateContentResponse
, based on the backend API in use.#no-changelog