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

Model seems to ignore .optional() fields, and instead uses fasly values such as 0, empty strings etc' #1180

Open
1 task done
uriva opened this issue Nov 11, 2024 · 5 comments · May be fixed by #1214
Open
1 task done
Labels
bug Something isn't working

Comments

@uriva
Copy link

uriva commented Nov 11, 2024

Confirm this is a Node library issue and not an underlying OpenAI API issue

  • This is an issue with the Node library

Describe the bug

Model seems to ignore .optional() fields, and instead uses nullary values, empty strings etc'

I'm not sure if it's a library issue or chatgpt model issue.

To Reproduce

I suspect this is a KI or an upstream problem, but let me know if not I'll send a repro

Code snippets

No response

OS

ubuntu

Node version

22.7.0

Library version

4.57.0

@uriva uriva added the bug Something isn't working label Nov 11, 2024
@uriva
Copy link
Author

uriva commented Nov 12, 2024

According to the docs, the model treats all fields as required, but it is recommended to use a union with null to achieve this: https://platform.openai.com/docs/guides/structured-outputs#all-fields-must-be-required

This sounds like zod's .optional should be interperted like that.

@RobertCraigie RobertCraigie added openai api Related to underlying OpenAI API and removed bug Something isn't working labels Nov 18, 2024
@RobertCraigie
Copy link
Collaborator

Thanks for reporting!

This sounds like an issue with the underlying OpenAI API and not the SDK, so I'm going to go ahead and close this issue. When you use .optional() we mark the property as required and then update the schema to mark it as nullable.

Would you mind reposting at community.openai.com?

@RobertCraigie RobertCraigie closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2024
@uriva uriva changed the title Model seems to ignore .optional() fields, and instead uses nullary values, empty strings etc' Model seems to ignore .optional() fields, and instead uses fasly values such as 0, empty strings etc' Nov 18, 2024
@jovermier
Copy link

This may be the line of code that converts an optional property to required.
propDef.isOptional() && !refs.openaiStrictMode ? acc.required : [...acc.required, propName],

I don't see any code for making the property nullable. In my testing I don't see the output schema nullable either.

@RobertCraigie RobertCraigie reopened this Nov 18, 2024
@RobertCraigie RobertCraigie added bug Something isn't working and removed openai api Related to underlying OpenAI API labels Nov 18, 2024
@uriva
Copy link
Author

uriva commented Nov 18, 2024

also just to be compliant with the zod type, undefined is considerably better if possible.
maybe with something that strips the undefined values after, then the user will be oblivious to all this as they should be.

devin-ai-integration bot added a commit to devin-open-source/openai-node that referenced this issue Dec 9, 2024
This commit adds a warning when .optional() is used in schemas for OpenAI API
Structured Outputs, recommending the use of .nullable() instead.

- Added warning in optional.ts that triggers when openaiStrictMode is true
- Added test to verify warning behavior

Fixes openai#1180
@silasalberti
Copy link

As discussed above, the API doesn't allow optional fields and the docs recommend nullable fields as a workaround. The SDK already supports Zod's .nullable() which achieves this purpose. People might be unaware about that nuance, so I propose adding a warning on .optional() that recommends using .nullable() instead: #1214

(I briefly thought about instead making .optional() have the same behavior as .nullable(). However, as mentioned above that would not be compliant with the Zod type, i.e. zod validation fails if it gets null instead of undefined.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants