-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Implement fail-fast feature #9708
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
Conversation
|
please review |
CodSpeed Performance ReportMerging #9708 will not alter performanceComparing Summary
|
sydney-runkle
left a comment
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.
Looking great overall - I've added a few comments / change requests. Let me know when you'd like another review 🚀 !
|
Super excited to include this in v2.8 - it'll be a great addition to our performance tips docs as well :). |
Co-authored-by: Sydney Runkle <[email protected]>
|
@sydney-runkle Could you please review this PR again? Especially |
sydney-runkle
left a comment
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.
Looks great - just a few more changes!
I'll also pull this down locally to do some testing after this next round to make sure things are ready to move ahead!
We can do a pydantic-core minor release next week to prep for v2.8, then get this across the line!
| min_length: int | None = None | ||
| max_length: int | None = None | ||
| strict: bool | None = None | ||
| fail_fast: bool | 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.
Same question - is fail_fast applicable to all sequence types? It probably should be for this case, but the case about where we reuse SEQUENCE_CONSTRAINTS is where more of my concern lies.
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.
Also, let's add some tests for my_field: Sequence = Field(..., fail_fast=True), etc.
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.
Then see what happens with a deque or something like that
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.
Same question - is fail_fast applicable to all sequence types?
You are right, it's not applicable to all sequence types. But I don't know how to correctly pass fail_fast to schema. Before calling apply_known_metadata it' always calls sequence_like_prepare_pydantic_annotations that fails because fail_fast is not in a list of SEQUENCE_CONSTRAINTS.
Maybe you have PR example how to correctly implement staff like this?
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.
Also, let's add some tests for my_field: Sequence = Field(..., fail_fast=True), etc.
It won't work for now. I guess first we need to implement a way how from python say rust to stop validation.
More context here - pydantic/pydantic-core#1322 (comment)
|
please review |
sydney-runkle
left a comment
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.
Awesome work here!
Approved. We'll merge this in once we get the next pydantic-core version released. Should be later this week 🚀
|
I'm really liking this feature. I'm guessing we could have something similar at the model level: from pydantic import BaseModel, ConfigDict, ValidationError
class Model(BaseModel):
a: int
b: int
model_config = ConfigDict(fail_fast=True)
try:
Model(a="some", b="string")
except ValidationError as e:
print(e)
# Only one error, for a
# or:
try:
Model.model_validate({"a": "some", "b": "string"}, fail_fast=True)
except ValidationError as e:
...Other than that, I'm hoping this is forward compatible with what was suggested here (making it an exception), then it could also be extended to any type: from typing import Annotated
from pydantic import BaseModel, BeforeValidator
def my_validator(value: Any):
raise FailFast # Or `FailNow`?
class Model(BaseModel):
a: Annotated[int, BeforeValidator(my_validator)]
b: int
# Validating `Model` would not do the validation for `b`. |
|
Agreed, this is super promising as a new pattern. Issues / PRs welcome re the ideas suggested above by @Viicos 🚀 |
Change Summary
Implement fail-fast feature
Related issue number
pydantic/pydantic-core#1322
Checklist
Selected Reviewer: @tiangolo