-
Notifications
You must be signed in to change notification settings - Fork 617
fix(ai): fix serialization for Content
with no parts
field
#6964
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
base: main
Are you sure you want to change the base?
Conversation
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
Test Results 12 files - 12 12 suites - 12 19s ⏱️ -15s For more details on these failures, see this check. Results for commit 75e5e14. ± Comparison against base commit af24598. ♻️ This comment has been updated with latest results. |
Coverage Report 1Affected ProductsNo changes between base commit (af24598) and merge commit (8c73fb4).Test Logs |
Size Report 1Affected Products
Test Logs |
@@ -90,7 +90,7 @@ constructor(public val role: String? = "user", public val parts: List<Part>) { | |||
@Serializable | |||
internal data class Internal( | |||
@EncodeDefault val role: String? = "user", | |||
val parts: List<InternalPart> | |||
@EncodeDefault val parts: List<InternalPart> = emptyList() |
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.
can you add a comment here noting why we need the encodeDefault in this case, as it's not immediately apparent.
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.
I actually can't remember why I added this here 😳 the tests still pass even if we don't use encodeDefault.
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.
Devs can send a request with an empty list, so makes sense that @EncodeDefault
that is used for encoding, not decoding, isn't 100% necessary. Should be safe to remove
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.
Removed
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.
SGTM. Don't forget to add an entry to the changelog
… into rpf/fix-content-part-serialization
In the Gemini Dev API,
Content#parts
is optional, while in the Vertex AI API it is marked as required.This PR should settle the difference by exposing a list of parts with at least 1 element whenever
Content#parts
is not present.Golden file PR: FirebaseExtended/vertexai-sdk-test-data#40