Skip to content

Conversation

@erbridge
Copy link
Contributor

@erbridge erbridge commented May 9, 2025

Prior to this change, we allowed the return of None values from models, serializing them as null, but the schema didn't specify them as nullable, which would cause model output to fail validation.

This only supports Pydantic V2, because, from what I can tell, Pydantic V1 doesn't give us any clues that a field allows None values.

Note: this is probably a breaking change.

@erbridge erbridge force-pushed the nullable-values-in-schema branch 16 times, most recently from 92c9b0a to 183c92e Compare May 12, 2025 12:10
Prior to this change, we allowed the return of `None` values from
models, serializing them as `null`, but the schema didn't specify them
as nullable, which would cause model output to fail validation.

This only supports Pydantic V2, because, from what I can tell, Pydantic
V1 doesn't give us any clues that a field allows `None` values.
@erbridge erbridge force-pushed the nullable-values-in-schema branch from 183c92e to 5637b18 Compare May 12, 2025 12:11
Copy link
Contributor

@aron aron left a comment

Choose a reason for hiding this comment

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

I think this is good to merge (bar the input_seed.py file).

Regarding whether this is a breaking change. From what I understand, optional types will now include the nullable field on the OpenAPI schema for both inputs and outputs. In practice this allows a client to pass field: null in addition to omitting it from the request body, it also correctly types the output to indicate that output can be null, this has always been the case when a prediction is in a non-terminal state.

The only two failure modes I can think of are:

  1. Where someone is using the generated OpenAPI schema for something else. The new presence of the nullable field could change the behavior of whatever system is using the schema.
  2. When the output schema is used to determine the type of the output. However this has always been a nullable field on the PredictionResponse so I would expect most consumers to handle this already despite the incorrect schema.

I think based on this we can class it as a bug fix without a major version bump.

@8W9aG
Copy link
Contributor

8W9aG commented May 21, 2025

I think this is good to merge (bar the input_seed.py file).

Regarding whether this is a breaking change. From what I understand, optional types will now include the nullable field on the OpenAPI schema for both inputs and outputs. In practice this allows a client to pass field: null in addition to omitting it from the request body, it also correctly types the output to indicate that output can be null, this has always been the case when a prediction is in a non-terminal state.

The only two failure modes I can think of are:

  1. Where someone is using the generated OpenAPI schema for something else. The new presence of the nullable field could change the behavior of whatever system is using the schema.
  2. When the output schema is used to determine the type of the output. However this has always been a nullable field on the PredictionResponse so I would expect most consumers to handle this already despite the incorrect schema.

I think based on this we can class it as a bug fix without a major version bump.

Luckily we have another breaking change going in right now, the next bump will be to 0.15.0 so we can make this part of that bump.

@8W9aG
Copy link
Contributor

8W9aG commented May 21, 2025

Going to merge this since we need to do a 0.15.0 release and I believe all the comments have been taken care of

@8W9aG 8W9aG merged commit 52550a7 into main May 21, 2025
25 checks passed
@8W9aG 8W9aG deleted the nullable-values-in-schema branch May 21, 2025 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants