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
Replace TransformSchema with GetPydanticSchema #6484
Conversation
Here is a code snippet showing how the type introduced by this PR can be used to address #6477: from typing import Union, List, Optional, Any
import numpy as np
import numpy.typing as npt
from typing_extensions import TypeAlias, Annotated
from pydantic import BaseModel, GetPydanticSchema, InstanceOf
VALIDATE_NDARRAY_INSTANCES = False
if VALIDATE_NDARRAY_INSTANCES:
PydanticNDArray: TypeAlias = Annotated[npt.NDArray, GetPydanticSchema.for_type(InstanceOf[np.ndarray])]
else:
PydanticNDArray: TypeAlias = Annotated[npt.NDArray, GetPydanticSchema.for_type(Any)]
CustomNDArrayType: TypeAlias = Union[PydanticNDArray, List[PydanticNDArray]]
class SomeModel(BaseModel):
data: Optional[CustomNDArrayType]
print(SomeModel(data=None))
# data=None
print(SomeModel(data=np.array([1, 2, 3])))
# data=array([1, 2, 3])
print(SomeModel(data=[np.array([1, 2, 3])]))
# data=[array([1, 2, 3])]
print(SomeModel(data=[1]))
# If VALIDATE_NDARRAY_INSTANCES is False:
#> data=[1]
# If VALIDATE_NDARRAY_INSTANCES is True:
"""
pydantic_core._pydantic_core.ValidationError: 2 validation errors for SomeModel
data.is-instance[ndarray]
Input should be an instance of ndarray [type=is_instance_of, input_value=[1], input_type=list]
For further information visit https://errors.pydantic.dev/2.1.2/v/is_instance_of
data.list[is-instance[ndarray]].0
Input should be an instance of ndarray [type=is_instance_of, input_value=1, input_type=int]
For further information visit https://errors.pydantic.dev/2.1.2/v/is_instance_of
""" |
Deploying with Cloudflare Pages
|
please review (cc @adriangb regardless of who hooky picks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple suggestions but generally looks good!
pydantic/types.py
Outdated
return self.transform(handler(source_type)) | ||
get_pydantic_core_schema: Callable[[Any, _annotated_handlers.GetCoreSchemaHandler], CoreSchema] | None = None | ||
get_pydantic_json_schema: Callable[[Any, _annotated_handlers.GetJsonSchemaHandler], JsonSchemaValue] | None = None | ||
prepare_pydantic_annotations: Callable[[Any, tuple[Any, ...], ConfigDict], tuple[Any, Iterable[Any]]] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave this one off for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with that, but I would suggest we leave this line there commented, with a comment explaining that if we find that it would be useful, it could be uncommented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure fine by me
pydantic/types.py
Outdated
prepare_pydantic_annotations: Callable[[Any, tuple[Any, ...], ConfigDict], tuple[Any, Iterable[Any]]] | None = None | ||
|
||
@staticmethod | ||
def for_type(type_: Any) -> GetPydanticSchema: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave this off for now? I'd like it to be requested/ used in more places before we introduce an abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't include it, I would at least consider my proposed solution above to the numpy thing a request.
In particular, I think GetPydanticSchema.for_type(InstanceOf[np.ndarray])
is a lot easier to understand than
GetPydanticSchema(lambda _s, h: h(InstanceOf[np.ndarray]), lambda _s, h: h(InstanceOf[np.ndarray]))
.
I'm okay waiting until I have another example or two where this utility would be useful, but I think it's advanced enough that I'll be surprised if people request it even if they would find it useful; I sort of think we'd need documentation of it before people would realize it was an option. Basically, if we internally want it a few more times I think it would be worth including.
The best argument against including it now, in my opinion, is if we want/need to change the way it interacts with __prepare_pydantic_annotations__
; if we exclude it I guess for now we would just need to suggest users use the thing I said was "hard to understand" above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm actually I think I made a mistake in the for_type
method anyway, and you don't need to specify the get_pydantic_json_schema
if you specify the core one. So maybe that's another good reason not to include it 😅. I'll remove it for now.
please update (for me) |
please review |
GetPydanticSchema
is meant to be a convenience type that eliminates the need to make "marker classes" when doing simple things related to custom types.I specifically worked on this to provide a good way to handle #6477 with versions of python/numpy where
numpy.typing.NDArray
is not actually a type, as I couldn't do this easily with our existing annotations. (I'll include a comment below demonstrating that this works for this, so I can more easily link to it in that issue.)While I understand there may be some hesitancy to introduce another annotation type like this, I think this implementation will "stand the test of time" because it directly matches the custom hooks interface — this type will reduce boilerplate and not require breaking changes for as long as we don't introduce breaking changes into the custom hooks types.
Also, considering
TransformSchema
is not inpydantic.types.__all__
, is completely undocumented, and migrating usage of it toGetPydanticSchema
is essentially trivial, I think it's okay to remove it, which I am currently doing in this PR.That said, we could definitely keep it and just add
GetPydanticSchema
as a new type, I just think it has been mostly rendered redundant.skip change file check
Selected Reviewer: @adriangb