Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
8e5e303 to
a1cdb25
Compare
| /** Whether [toFirebaseSchema] converts this schema with [freeFormObject]. */ | ||
| private fun Schema.convertsToFreeFormObject(): Boolean = | ||
| firebaseBranch() == FirebaseBranch.TYPED && | ||
| (type == Type.OBJECT || type == Type.TYPE_UNSPECIFIED || type == null) && | ||
| properties.isNullOrEmpty() |
There was a problem hiding this comment.
Consider reusing convertsToFreeFormObject() inside toFirebaseSchema's Type.OBJECT / Type.TYPE_UNSPECIFIED / null branch (lines 671–676) so the free-form object predicate is defined in one place and cannot drift between toFirebaseSchema and toFirebaseResponseSchema.
There was a problem hiding this comment.
Done in 5ddf267: the Type.OBJECT / Type.TYPE_UNSPECIFIED / null branch of toFirebaseSchema now calls convertsToFreeFormObject(), so toFirebaseSchema and toFirebaseResponseSchema share one definition of the free-form object case.
|
|
||
| @Test | ||
| fun requestConverter_generationConfigBuilder_freeFormResponseSchema_isLeftOut() { | ||
| // A tool argument with no properties becomes a string carrying JSON, but a response with that | ||
| // schema would be a quoted string. The JSON MIME type alone still asks for an object. | ||
| val request = | ||
| LlmRequest( | ||
| config = | ||
| GenerateContentConfig( | ||
| responseMimeType = "application/json", | ||
| responseSchema = Schema(type = Type.OBJECT), | ||
| ) | ||
| ) | ||
|
|
||
| val builder = Conversions().forRequest(request).generationConfigBuilder() | ||
|
|
||
| assertThat(builder.responseSchema).isNull() | ||
| assertThat(builder.responseMimeType).isEqualTo("application/json") |
There was a problem hiding this comment.
Please also add test assertions covering Schema(type = Type.OBJECT, properties = emptyMap()) / Schema(type = Type.TYPE_UNSPECIFIED) being omitted and an untyped anyOf union or enum response schema (type = null, properties = null) being preserved by toFirebaseResponseSchema.
There was a problem hiding this comment.
Done in 5ddf267: freeFormResponseSchema_isLeftOut now checks that Schema(type = Type.OBJECT), Schema(type = Type.OBJECT, properties = emptyMap()), Schema(type = Type.TYPE_UNSPECIFIED) and Schema() are all left out. The new untypedUnionAndEnumResponseSchemas_areKept checks that an untyped anyOf union and an untyped enum (type = null, properties = null) are kept as the response schema.
There was a problem hiding this comment.
Thanks for the clean fix and tests, @GCSimba! Left two minor comments on reusing convertsToFreeFormObject() in Conversions.kt and covering the remaining convertsToFreeFormObject branches in ConversionsTest.kt. please also rebase onto the latest main.
a1cdb25 to
5ddf267
Compare
|
Thanks for the review, @aravindmurali22! Rebased onto the latest |
|
@GCSimba could you please rebase your branch onto the latest main here as well? Thanks! |
5ddf267 to
fc22165
Compare
|
@GCSimba Thanks for the contribution! A couple of quick items before we proceed:
Appreciate your help with this! |
An agent's output schema reaches the model as responseSchema, but the Firebase backend never set it, so the request kept only the JSON MIME type and the model could answer with any JSON shape. presencePenalty and frequencyPenalty were dropped too, although Firebase supports all three and the Gemini backend sends them.
fc22165 to
3ee76e1
Compare
|
Rebased onto e665796 again; the diff is unchanged, and About the failing check: the last
Failed runs
If the new run hits one of these again, could you re-run the failed job? Thanks! |
An agent's
outputSchemareaches the model asGenerateContentConfig.responseSchema, but the Firebase backend never set it on Firebase'sGenerationConfig, so the request kept only the JSON MIME type and the model could answer with any JSON shape.presencePenaltyandfrequencyPenaltywere dropped the same way, although Firebase'sGenerationConfig.Builderhas all three fields and the Gemini backend sends them. With a Gemini 3 modelcanUseOutputSchemaWithToolsis true, so even an agent with tools relies on the response schema rather thanset_model_response.Set the two penalties directly and convert the response schema with the existing
toFirebaseSchema, which keepsrequiredthroughoptionalPropertiesand warns about constraints Firebase cannot carry. One exception: an object that declares no properties is left out.toFirebaseSchematurns it into a string carrying JSON, which suits a tool argument but would make the model answer with a quoted string; the JSON MIME type alone still asks for an object. Both functions use the sameconvertsToFreeFormObject()check to recognize such an object.Tests in
ConversionsTest:mapsResponseSchemamaps an object schema with a required field;freeFormResponseSchema_isLeftOutleaves outOBJECTwith no or emptyproperties,TYPE_UNSPECIFIED, and a schema without a type;untypedUnionAndEnumResponseSchemas_areKeptkeeps an untypedanyOfunion and an untypedenum.All but the free-form test fail without the fix.
Validation on Ubuntu 24.04 (WSL2), JDK 21, at 5ddf267 (rebased onto b961c9e):
ConversionsTest: 78 passed. WithConversions.ktfrommainit has 3 failures:mapsResponseSchema(expected: OBJECT),returnsBuilder(expected: 0.6) anduntypedUnionAndEnumResponseSchemas_areKept.:google-adk-kotlin-firebase:buildand:google-adk-kotlin-examples-android:build, the only module that depends on Firebase, passed. The Firebase unit tests ran 113 with no failures; the 8 skipped are the live integration tests, which need Firebase credentials. The build left the working tree clean.ktfmt --google-style(0.65) leaves the new source lines unchanged. In the test file it would only add trailing commas, as it would throughout the existing file.git diff --checkpassed.After rebasing onto e863fac (now fc22165, same diff),
ConversionsTestpassed again on the new base: 78/78.The Gradle runs used
-PjdkVersion=21,-Dorg.gradle.jvmargs="-Xmx3g -XX:MaxMetaspaceSize=1g",-Dorg.gradle.parallel=false,--max-workers=2,--no-daemon,--stacktrace, and--continue. No request reached a live Firebase backend, and the JDK 17/25 matrix was not run locally.This change was drafted with AI assistance (GitHub Copilot); I reviewed the diff and the validation results above.