- Notifications
You must be signed in to change notification settings - Fork293
Add fail-fast for dicts#1543
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
please review |
codecovbot commentedNov 17, 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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #1543 +/- ##==========================================- Coverage 90.21% 89.60% -0.61%========================================== Files 106 112 +6 Lines 16339 17867 +1528 Branches 36 40 +4 ==========================================+ Hits 14740 16010 +1270- Misses 1592 1837 +245- Partials 7 20 +13
... and60 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
codspeed-hqbot commentedNov 17, 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#1543 willnot alter performanceComparing Summary
|
After implementing this feature I thought maybe it's make sense to add kinda validation context structure to not duplicate fail-fast check logic, it can look like this: structValidationContext{fail_fast:bool,errors:Vec<ValLineError>,}implValidationContext{fnnew(fail_fast:bool) ->Self{Self{ fail_fast,errors:Vec::new(),}}fndefault() ->Self{returnSelf::new(false);}fnshould_stop(self) ->bool{self.fail_fast &&self.has_errors()}fnhas_errors(self) ->bool{ !self.errors.is_empty()}fnadd_error(mutself,error:ValLineError){self.errors.push(error);}} |
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.
Thanks, overall the implementation seems fine.
RE your refactoring idea, I was beginning to think of something similar recently, see#1517, probably this solves related problems.
I realise that looking at this, I'm a bit unsure what the user-facing result is. What order do they expect validation of fields to be in? Does it matter if we validate the first field but not the last N, because the second was fail fast? What if the next time we randomly did these in a different order, and the performance was different? Maybe there were even side effects 👀
Under what cases is fail-fast useful for a model?
I would love to understand some more about this. I think if it's about making errors shorter, we could make it easier to see just the first error. If it's about performance, well, maybe we just make validation faster so that fail-fast doesn't matter.
Unlike lists and sequences, with potentially large inputs, a model should have a relatively small finite number of fields, so I don't know how much performance is gained by failing early...
@davidhewitt Your points totally makes sense to me. I removed fail-fast for model and dataclass and left it for dict and typed-dict. |
Uh oh!
There was an error while loading.Please reload this page.
Change Summary
Add fail-fast feature to:
dataclass-argumentsmodel-fieldsRelated issue number
#1345
Checklist
pydantic-core(except for expected changes)