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

fix: fixed gemini model handling of nullish optional fields in response schema #1411

Merged
merged 3 commits into from
Nov 26, 2024

Conversation

pavelgj
Copy link
Collaborator

@pavelgj pavelgj commented Nov 26, 2024

Zod schema like this:

schema: z.object({
  title: z.string(),
  subtitle: z.string().nulish(),
}),

and picoschema like this:

output:
  schema:
    title: string
    subtitle?: string

produce schema:

{
  "type": "object",
  "properties": {
    "title": {
      "type": "string"
    },
    "subtitle": {
      "type": [
        "string",
        "null"
      ]
    }
  },
  "required": [
    "title"
  ],
  "additionalProperties": false
}

which is not a valid schema in Gemini API request for constrained generation.

  status: 400,
  statusText: 'Bad Request',
  errorDetails: [
    {
      '@type': 'type.googleapis.com/google.rpc.BadRequest',
      fieldViolations: [
        {
          field: 'generation_config.response_schema.properties[1].value',
          description: `Invalid JSON payload received. Unknown name "type" at 'generation_config.response_schema.properties[1].value': Proto field is not repeating, cannot start list.`
        }
      ]
    }
  ]

convert

      "type": [
        "string",
        "null"
      ]

to

      "type": "string",

Checklist (if applicable):

  • Tested (manually, unit tested, etc.)

@pavelgj pavelgj requested a review from mbleigh November 26, 2024 18:34
@mbleigh
Copy link
Collaborator

mbleigh commented Nov 26, 2024

This was intentional and ["string", "null"] is a valid JSON schema type. I added it as the default behavior because Gemini would frequently add null values to optional keys instead of omitting them, causing schema validation to fail when it should have succeeded.

Why is this becoming a problem?

@pavelgj
Copy link
Collaborator Author

pavelgj commented Nov 26, 2024

This was intentional and ["string", "null"] is a valid JSON schema type. I added it as the default behavior because Gemini would frequently add null values to optional keys instead of omitting them, causing schema validation to fail when it should have succeeded.

Why is this becoming a problem?

Gemini response schema does not accept this as a valid schema.

  status: 400,
  statusText: 'Bad Request',
  errorDetails: [
    {
      '@type': 'type.googleapis.com/google.rpc.BadRequest',
      fieldViolations: [
        {
          field: 'generation_config.response_schema.properties[1].value',
          description: `Invalid JSON payload received. Unknown name "type" at 'generation_config.response_schema.properties[1].value': Proto field is not repeating, cannot start list.`
        }
      ]
    }
  ]

@mbleigh
Copy link
Collaborator

mbleigh commented Nov 26, 2024

This was intentional and ["string", "null"] is a valid JSON schema type. I added it as the default behavior because Gemini would frequently add null values to optional keys instead of omitting them, causing schema validation to fail when it should have succeeded.
Why is this becoming a problem?

Gemini response schema does not accept this as a valid schema.

  status: 400,
  statusText: 'Bad Request',
  errorDetails: [
    {
      '@type': 'type.googleapis.com/google.rpc.BadRequest',
      fieldViolations: [
        {
          field: 'generation_config.response_schema.properties[1].value',
          description: `Invalid JSON payload received. Unknown name "type" at 'generation_config.response_schema.properties[1].value': Proto field is not repeating, cannot start list.`
        }
      ]
    }
  ]

Then let's fix it at the Gemini plugin level, not the Picoschema level. In the Gemini plugins, can we recursively look for array type options and replace them with the first non-"null" value? There's already a cleanSchema function because I had to do other cleanup to make Gemini happy.

@pavelgj
Copy link
Collaborator Author

pavelgj commented Nov 26, 2024

This was intentional and ["string", "null"] is a valid JSON schema type. I added it as the default behavior because Gemini would frequently add null values to optional keys instead of omitting them, causing schema validation to fail when it should have succeeded.
Why is this becoming a problem?

Gemini response schema does not accept this as a valid schema.

  status: 400,
  statusText: 'Bad Request',
  errorDetails: [
    {
      '@type': 'type.googleapis.com/google.rpc.BadRequest',
      fieldViolations: [
        {
          field: 'generation_config.response_schema.properties[1].value',
          description: `Invalid JSON payload received. Unknown name "type" at 'generation_config.response_schema.properties[1].value': Proto field is not repeating, cannot start list.`
        }
      ]
    }
  ]

Then let's fix it at the Gemini plugin level, not the Picoschema level. In the Gemini plugins, can we recursively look for array type options and replace them with the first non-"null" value? There's already a cleanSchema function because I had to do other cleanup to make Gemini happy.

yeah... was just looking into cleanSchema...

interestingly, zod nullish() also produces:

     "type": [
        "string",
        "null"
      ]

we had someone on discord already run into this. I guess it makes sense to fix this at the plugin level to support both zod nullish and picoschema.

@pavelgj pavelgj force-pushed the pj/fixPicoschemaOptional branch from 903a72c to 41dbec0 Compare November 26, 2024 19:18
@pavelgj pavelgj changed the title fix: make picoschema generate valid property types for optional fields fix: fixed gemini model handling of nullish optional fields in response schema Nov 26, 2024
@pavelgj pavelgj merged commit 9a8a1d4 into main Nov 26, 2024
4 checks passed
@pavelgj pavelgj deleted the pj/fixPicoschemaOptional branch November 26, 2024 19:36
pavelgj added a commit that referenced this pull request Nov 30, 2024
pavelgj added a commit that referenced this pull request Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants