Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.2k
Move core schema generation logic for path types inside theGenerateSchema
class#10846
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cloudflare-workers-and-pagesbot commentedNov 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Deploying pydantic-docs with |
Latest commit: | b2b8f7e |
Status: | ✅ Deploy successful! |
Preview URL: | https://b439c509.pydantic-docs.pages.dev |
Branch Preview URL: | https://perf-remove-prep-anns.pydantic-docs.pages.dev |
codspeed-hqbot commentedNov 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#10846 willnot alter performanceComparing Summary
|
github-actionsbot commentedNov 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Coverage reportClick to see where and how coverage changed
This report was generated bypython-coverage-comment-action |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
What does this tell us about our annotation application in general?
I think it reveals a limitation of our current annotation application. Because paths could be considered as a custom type (i.e. there's no core schema that represents this type, we currently build the validation logic with a customafter validator), we face the limitation of not having access to the annotated metadata as describedhere.
The currentmin/max_length_validator
function is not going to scale well in the future if we ever want to add support formin/max_length
for another type similar toPath
(and that type similarly doesn't have a core schema type for it).
I'll note that while this limitation shows up here with a type without a proper core schema for it, we also face weird behaviors with the following (and unlikePath
,int
has a core schema type):
defval(v):print('val called')returnvclassModel(BaseModel):a:Annotated[int,AfterValidator(val),Field(gt=0)]Model(a=-1)#> val called#> ValidationError raised
- the after validator is calledbefore the
gt
validation, which imo is not what should happen. - as a consequence, the
gt
constraint isn't represented in theint_schema()
, but instead thegreater_than_validator
Python fallback validator function is used.
On another note, maybe we should discuss how we validate "concrete" vs "abstract" types. For instance, for paths, we usepathlib.PurePath
foros.PathLike
annotations, but this is an opinionated choice and users could expectPath
to be used instead.
This relates to the behavior ofIterable
: we currently validate these by creating aValidationIterator
instance, and people complained about it. Maybe we should have a way of customizing which "concrete" implementation should be used for "abstract" types?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Path
typesUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Path
typesGenerateSchema
classaa85c3a
intomainUh oh!
There was an error while loading.Please reload this page.
…Schema` class (pydantic#10846)Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Some (verbose) context:#10036
I'm trying to remove the use of
prepare_pydantic_annotations_for_known_type
internally. We call this function in order to to pull metadata off of types and inject said metadata's constraints into core schema. This is bad practice, as we generally advertise being religious about respecting metadata order when building schema, but we violate that principle here.Though these checks don't drag much on performance, they do significantly clutter our internal schema generation logic. They represent the old pattern (we have since deprecated) of supporting a
__pydantic_prepare_annotations__
method.For example, we used to pull string constraint data like
min_length
off of aPath
like annotation + apply that to the innercore_schema.str_schema()
orcore_schema.bytes_schema()
.This removes support for constraints like
max_length
,min_length
, andstrip_whitespace
or other string constraints onPath
types. Users should implement these constraints with custom validators.This should be advertised in the v2.11 changelog.