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

Migrate VertexAI serialization to be localized #6631

Merged
merged 11 commits into from
Jan 29, 2025

Conversation

emilypgoogle
Copy link
Collaborator

There are some considerations to how this should be finalized. Current implementation details that I've decided on which we can change:

  • Based on the example doc, all classes Foo have been renamed InternalFoo
  • All internal serialization classes, where relevant, are moved to inner classes of their API counterparts
  • All classes only used as child fields for serialization classes have been moved to inner classes of those classes
  • All toPublic and toInternal methods on API and serialization classes have been moved inside of those classes and conversions.kt has been mostly emptied.
  • A few serialization classes do not have API equivalents and are left in a Types.kt file

Possible changes:

  • Change all InternalFoo classes to have the same name, referenced as Foo.Internal rather than Foo.InternalFoo. This will probably make the codebase feel cleaner, but I'll wait for opinions on it
  • Move serialization only classes out of the internal serialization classes, either to inner classes of the API classes or top level classes themselves.
  • For classes that have serializers, rename serializers from InternalFooSerializer to Serializer for example Foo.InternalFoo.Serializer or Foo.Internal.Serializer instead of Foo.InternalFoo.InternalFooSerializer or Foo.Internal.InternalFooSerializer

@emilypgoogle emilypgoogle requested review from rlazo and daymxn January 17, 2025 22:29
Copy link
Contributor

github-actions bot commented Jan 17, 2025

Release note changes

No release note changes were detected. If you made changes that should be
present in the next release, ensure you've added an entry in the appropriate
CHANGELOG.md file(s).

Copy link
Contributor

Vertex AI Mock Responses Check ⚠️

A newer major version of the mock responses for Vertex AI unit tests is available. update_responses.sh should be updated to clone the latest version of the responses: v5.3

@google-oss-bot
Copy link
Contributor

1 Warning
⚠️ Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 17, 2025

Copy link
Contributor

github-actions bot commented Jan 17, 2025

Test Results

 20 files  ±0   20 suites  ±0   12s ⏱️ ±0s
113 tests ±0  113 ✅ ±0  0 💤 ±0  0 ❌ ±0 
226 runs  ±0  226 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f618e00. ± Comparison against base commit f2d05d6.

♻️ This comment has been updated with latest results.

Copy link
Member

@daymxn daymxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't currently compile, as there's some imports and usages that need to be updated. Like:

- private fun InternalGenerateContentResponse.validate()
+ private fun GenerateContentResponse.InternalGenerateContentResponse.validate()

Also, (and this probably a convo for @rlazo too), any reason we want the fully qualified names over something like Internal?

Current:

public class FunctionDeclaration(
  internal val name: String,
  internal val description: String,
  internal val parameters: Map<String, Schema>,
  internal val optionalParameters: List<String> = emptyList(),
) {
  internal val schema: Schema =
    Schema.obj(properties = parameters, optionalProperties = optionalParameters, nullable = false)

  internal fun toInternal() =
    InternalFunctionDeclaration(name, "", schema.toInternal())

  @Serializable
  internal data class InternalFunctionDeclaration(
    val name: String,
    val description: String,
    val parameters: Schema.InternalSchema
  )
}

With internal naming:

public class FunctionDeclaration(
  internal val name: String,
  internal val description: String,
  internal val parameters: Map<String, Schema>,
  internal val optionalParameters: List<String> = emptyList(),
) {
  internal val schema: Schema =
    Schema.obj(properties = parameters, optionalProperties = optionalParameters, nullable = false)

  internal fun toInternal() =
   Internal(name, "", schema.toInternal())

  @Serializable
  internal data class Internal(
    val name: String,
    val description: String,
    val parameters: Schema.InternalSchema
  )
}

So its usage would be like this:

- FunctionDeclaration.InternalFunctionDeclaration(...)
+ FunctionDeclaration.Internal(...)

@rlazo
Copy link
Collaborator

rlazo commented Jan 22, 2025

  • ther to inner classes of the API classes or top level classes themselves.

+1 to Call inner classes Internal rather than the full name, and the same applies to serializers.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 22, 2025

Size Report 1

Affected Products

  • firebase-vertexai

    TypeBase (f2d05d6)Merge (3f27cf8)Diff
    aar477 kB490 kB+13.2 kB (+2.8%)
    apk (aggressive)1.09 MB1.32 MB+223 kB (+20.4%)
    apk (release)9.15 MB9.16 MB+6.27 kB (+0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/n0LyJ4ToEP.html

@emilypgoogle emilypgoogle requested a review from daymxn January 23, 2025 20:30
@emilypgoogle emilypgoogle requested a review from rlazo January 24, 2025 18:06
Copy link
Member

@daymxn daymxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Besides the test failures, LGTM. I defer to @rlazo for the final approval.

Copy link
Member

@daymxn daymxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so long as the tests pass

@emilypgoogle emilypgoogle merged commit 31e0d06 into main Jan 29, 2025
30 checks passed
@emilypgoogle emilypgoogle deleted the ep/vertex-serializable branch January 29, 2025 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants