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.

Sign up for GitHub

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

Jump to bottom

Remove format from double helper function #6432

Merged
merged 5 commits into from
Nov 6, 2024
Merged

Remove format from double helper function #6432

merged 5 commits into from
Nov 6, 2024

Conversation

rlazo
Copy link
Collaborator

@rlazo rlazo commented Nov 5, 2024 •
edited
Loading

We can omit the format for the double-precision floating point type

See firebase/firebase-ios-sdk#13990 for further context

@rlazo
Remove format for double helper functions
c88a4cc 
We can omit the format for the double-precision floating point type
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

Release note changes

The following release notes were modified. Please ensure they look correct.

Release Notes
firebase-vertexai 
### {{firebase_vertexai}} version 16.0.2 {: #vertex-ai_v16-0-2}

* {{fixed}} Improved error message when using an invalid location. GitHub [#6428](//github.com/firebase/firebase-android-sdk/issues/6428){: .external}

* {{fixed}} Fixed issue where Firebase App Check error tokens were unintentionally missing from the requests. GitHub [#6409](//github.com/firebase/firebase-android-sdk/issues/6409){: .external}

* {{fixed}} Clarified in the documentation that `Schema.integer` and `Schema.float` only provide hints to the model. GitHub [#6420](//github.com/firebase/firebase-android-sdk/issues/6420){: .external}

* {{fixed}} Fixed issue were `Schema.double` set the format parameter in `Schema`. GitHub [#6432](//github.com/firebase/firebase-android-sdk/issues/6432){: .external}

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

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.1

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

Test Results

 20 files   -  34   20 suites   - 34   14s ⏱️ - 1m 48s
113 tests  - 402  113 ✅  - 401  0 💤  - 1  0 ❌ ±0 
226 runs   - 804  226 ✅  - 802  0 💤  - 2  0 ❌ ±0 

Results for commit a63d93d. ± Comparison against base commit 80019ca.

This pull request removes 515 and adds 113 tests. Note that renamed tests count towards both. 
com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ descriptor should have expected values
com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ deserialize() should throw UnsupportedOperationException
com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ serialize() should throw UnsupportedOperationException
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Boolean) creates an object with the expected value
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Double) creates an object with the expected value (edge cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Double) creates an object with the expected value (normal cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(List) creates an object with the expected value (edge cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(List) creates an object with the expected value (normal cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Map) creates an object with the expected value (edge cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Map) creates an object with the expected value (normal cases)
…

com.google.firebase.vertexai.GenerativeModelTesting ‑ exception thrown when using invalid location
com.google.firebase.vertexai.GenerativeModelTesting ‑ system calling in request
com.google.firebase.vertexai.SchemaTests ‑ basic schema declaration
com.google.firebase.vertexai.SchemaTests ‑ full schema declaration
com.google.firebase.vertexai.StreamingSnapshotTests ‑ citation parsed correctly
com.google.firebase.vertexai.StreamingSnapshotTests ‑ empty content
com.google.firebase.vertexai.StreamingSnapshotTests ‑ http errors
com.google.firebase.vertexai.StreamingSnapshotTests ‑ image rejected
com.google.firebase.vertexai.StreamingSnapshotTests ‑ invalid api key
com.google.firebase.vertexai.StreamingSnapshotTests ‑ invalid json
…

♻️ This comment has been updated with latest results.

@rlazo rlazo changed the title Remove format for double helper functions Remove format from double helper function Nov 5, 2024
@rlazo rlazo requested a review from daymxn November 5, 2024 20:57
@rlazo
Copy link
Collaborator Author

rlazo commented Nov 5, 2024

cc @andrewheard

@andrewheard andrewheard mentioned this pull request Nov 5, 2024
Merged
daymxn
daymxn reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@daymxn daymxn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is double the default? Is this behavior we should rely on?

e: I just saw the ios issue which helped provide context.

Although, isn't this an issue with the backend? Shouldn't it enforce it?

@rlazo
Copy link
Collaborator Author

rlazo commented Nov 5, 2024

Is double the default? Is this behavior we should rely on?

e: I just saw the ios issue which helped provide context.

Although, isn't this an issue with the backend? Shouldn't it enforce it?

double is the largest floating-point number type in Kotlin (not counting Big* numbers). As in the case of long, we assume the type with the largest capacity is generic enough to represent the range of values the model can return if no is format specified.

As documented in the kdoc, even if everything is working as expected, integer and float are hints in the Schema.

daymxn
daymxn approved these changes Nov 5, 2024
View reviewed changes
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.

SGTM. May be worth directly linking to andrew's PR in the PR desc for context/auditing

@rlazo rlazo merged commit 9754546 into main Nov 6, 2024
26 of 27 checks passed
@rlazo rlazo deleted the rl.double.format branch November 6, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daymxn daymxn daymxn approved these changes

@andrewheard andrewheard andrewheard approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rlazo @andrewheard @daymxn @google-oss-bot