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

Issue #560 #580

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mwigh
Copy link

@mwigh mwigh commented Oct 2, 2024

Needs to set required fields as required

Description of the change

When feeding a response_schema of type TypedDict to a GenerationConfig, then we need to set the field as required if it is required. This codes does that. It also avoids setting optional fields (Optional and NotRequired) as required.

Motivation

It solves the issue described in #560

Type of change

Bug fix

Checklist

  • I have performed a self-review of my code.
  • I have added detailed comments to my code where applicable.
  • I have verified that my change does not break existing code.
  • My PR is based on the latest changes of the main branch (if unsure, please run git pull --rebase upstream main).
  • I am familiar with the Google Style Guide for the language I have coded in.
  • I have read through the Contributing Guide and signed the Contributor License Agreement.

Needs to set required fields as required
@github-actions github-actions bot added status:awaiting review PR awaiting review from a maintainer component:python sdk Issue/PR related to Python SDK labels Oct 2, 2024
@MarkDaoust
Copy link
Collaborator

  • We should try and make sure that the behavior is consistent between TypedDict, Dataclass, and Pydantic.
  • We should be sure the behavior is consistent between function definitions and response_schema.

I don't think this fills both those requirements. I'll have to re-read this in more detail.

Needs to set required fields as required

Handle cases dataclasses, TypedDict and Pydantic models
@markmcd
Copy link
Member

markmcd commented Oct 8, 2024

Could you also add tests to cover the cases you're fixing? This way we insure against backsliding.

@mwigh
Copy link
Author

mwigh commented Oct 9, 2024

@markmcd
see comments in #560
I do not know what cases you want to cover

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:python sdk Issue/PR related to Python SDK status:awaiting review PR awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants