-
Notifications
You must be signed in to change notification settings - Fork 247
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
Implementation of __cause__ for ValidationError using ExceptionGroups #780
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #780 +/- ##
==========================================
- Coverage 93.80% 93.78% -0.03%
==========================================
Files 105 105
Lines 15468 15539 +71
Branches 25 25
==========================================
+ Hits 14510 14573 +63
- Misses 952 960 +8
Partials 6 6
Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #780 will degrade performances by 21.18%Comparing Summary
Benchmarks breakdown
|
I think this is an interesting idea! I haven't looked in detail at how it works, but I will say that what really should happen in the end is that I will look in more detail but just because of the calling into Python that's being done here I suspect there is going to be a huge performance hit to benchmarks (and it looks like CodSpeed is already flagging |
@adriangb if the performance of the backport is the problem, this could be supported 3.11+ only. I've updated the PR to only include the logic when python is 3.11+ (compile time excluded code on older versions), and uses the rust My focus is 3.11+, I personally wouldn't mind if this was a limitation. However the arguments for using the backport on older versions:
Out of interest, why would you eventually want the |
A You can also do weird things (which I think this PR should consider btw) like having a python function validator which does: try:
handler(x)
catch ValidationError as e:
raise ValueError(...) from e So now you have a ValidationError -> ValueError -> ValidationError relationship. |
Makes sense you're right. I guess it then comes down to the issues you'd mentioned with how infeasible it would be to convert I've just tested out your nested import typing as tp
from pydantic import BaseModel, field_validator, ValidationError, TypeAdapter, AfterValidator
def check(v: int):
if v < 0:
raise ValueError("INNER ERR")
return v
sub_validator = TypeAdapter(tp.Annotated[int, AfterValidator(check)])
class Baz(BaseModel):
bar: int
@field_validator("bar")
def check_bar(cls, v):
try:
return sub_validator.validate_python(v)
except ValidationError as e:
raise ValueError("SUB VALIDATION ERROR!") from e
class Foo(BaseModel):
foo: int
baz: Baz
@field_validator("foo")
def check_foo(cls, v):
if v < 0:
try:
check(v)
except ValueError as e:
new_err = ValueError("OUTER ERR")
new_err.add_note("SOME EXTRA INFORMATION")
raise new_err from e
return v
Foo(foo=-1, baz={"bar": -1}) Outputs:
Which works reasonably well out the box for the weird double |
8ed7226
to
d8642e6
Compare
I'm close to happy with this now. Fixed the random exception missing its traceback in the result and switched to a note rather than an extra warning exception. The changes made to @davidhewitt do you have any idea why this is and if there's a better way to solve it than I've done? |
Are you willing to reduce this to a minimal repro which can be reported as an issue to PyO3, and we can discuss there if it's a bug? Before 3.12 the Python exception state is stored as split type / value / traceback and so I'd be willing to believe the |
@davidhewitt done :), if there's a cleaner way of getting around it on the current pyo3 version for the use here, please let me know. |
Thanks - I think your implementation here is correct (and likely what we would land upstream in PyO3). |
9f0da27
to
9e9f879
Compare
@adriangb and I just had a catch up about this PR. Overall we like this a lot and we're happy to offer users a way to get at inner exceptions. Some thoughts which we had:
|
@davidhewitt great to hear!
No qualms against this for programmatic access, would definitely work.
TLDR it doesn't work. I'd actually already looked into this when trying to ease @adriangb's performance concerns with the backport. If you've got any ideas on this let me know, I gave up.
Valid concern, given how much looks like needs improving in An option for making the above enforceable/covered pd side, we could add another method to the So if lazy doesn't work, unless you guys think of something new it doesn't, options for backport are:
|
@davidhewitt I have actually found a way to get around the lazy problem but it's a bit weird: import sys
original_excepthook = sys.excepthook
# Todo: handle already seen / infinite loops
def traverse_causes_on_crash(start_exc: BaseException):
excs: list[BaseException] = [start_exc]
if isinstance(start_exc, BaseExceptionGroup):
excs.extend(start_exc.exceptions)
for exc in excs:
cause = getattr(exc, '__cause__', None)
if cause is not None:
traverse_causes_on_crash(cause)
def replacement_excepthook(exc_type, exc_value, exc_traceback):
# Traverse the causes:
traverse_causes_on_crash(exc_value)
# Call the original excepthook:
original_excepthook(exc_type, exc_value, exc_traceback)
if original_excepthook is not replacement_excepthook:
sys.excepthook = replacement_excepthook This allows the But this would make the whole thing 0 cost, snippet could either just be run on
|
6489d16
to
daf3eba
Compare
Other than PyPy, this is now working on all versions with no performance cost, the I'm more or less finished with it, so its ready for you to let me know what you want changing. If you see what might be causing the segfault in PyPy (in the exceptiongroup dep: Imo it should just be a dep as I've got it now, not just silently omit the group unless the backports installed. |
PyPy fixed, it uses Converting from draft! Please review |
@davidhewitt the note (3.11+) / If you check all the way to the top of this PR i give the examples for both versions. If we don't include the |
Thanks, yes that makes sense from a UX perspective and fits with our statement that this won't be stable. |
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.
otherwise LGTM.
Assuming @davidhewitt is happy with the rust code.
tests/test_errors.py
Outdated
|
||
enabled_config: CoreConfig = {'validation_error_cause': True} | ||
|
||
def check_grouped_exception(exc: BaseException) -> BaseException: |
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.
better to have two tests and add add a pytest.mark.skipif
to both, so only one runs.
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've separated out the contents tests as you suggested here :)
assert repr(cause) == repr(ValueError('Oh no!')) | ||
assert cause.__traceback__ is not None | ||
|
||
def multi_raise_py_error(v: Any) -> Any: |
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.
this seems like a separate test.
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.
Done, new traceback preservation check test.
tests/test_errors.py
Outdated
with pytest.raises(ValidationError) as exc_info: | ||
s2.validate_python('anything') | ||
|
||
def validate_s2_chain(val_err: ValidationError): |
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.
better not to use functions like this as it becomes hard to understand upon failure, especially as it only removes one duplication.
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.
No prob, duplicated the code!
tests/test_errors.py
Outdated
def singular_raise_py_error(v: Any) -> Any: | ||
raise ValueError('Oh no!') | ||
|
||
for _, config, expected in config: |
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.
please use parameterize rather than a loop - loops in tests are impossible to understand/debug.
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.
Done!
…ess in the traceback, added tests.
…rectly with the set and get ffi cause methods
…ess in the traceback, added tests.
…ongroups in test reqs, readded accidentally removed lint ignores
@samuelcolvin @davidhewitt sorry for the delay, refactored the tests as requested :) |
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 again, this LGTM and it's nice that it delivers some neat performance improvements on the error pathway too! Sorry that this has had such long cycle times on my end.
No problem and thanks! 🎉 |
Change Summary
Out of date, contents has changed significantly from initial draft. See the comments for the path to the current version.
Draft wanting feedback
ExceptionGroup
backport pre 3.11__cause__
of aValidationError
, including the respectiveloc
for each cause chainFollowing on from my issue in pydantic main, if
ValidationError
(s) utilised__cause__
they would become immensely more powerful in non-user facing, debugging scenarios.I've got a working but dirty implementation here, was trying for minimal lines.
Before I spend more time on it making it pretty, safe and optimised, I'm looking for thoughts on whether this is something Pydantic is willing to support.
This example implementation works with Python 3.7 upwards for compatibility, using the exceptiongroup backport to that effect. Works great in all python versions. (obviously
add_note()
is only used 3.11 upwards, but the chaining and groups are available on all supported versions).For 3.11 upwards this could be improved to:
add_note()
on the main exception instead of the extraUserWarningException
to specify theloc
If you're not liking the bloat for the average new user, maybe this could be enabled with a global flag.
This also seems to break 0 tests as nothing as of yet relies on
__cause__
.Edit: i've noticed something off with the second error group, aka the singular exception for
bar
, not sure why its not but should show the error location same as the source error offoo
chain, would fix.Implements an
ExceptionGroup
as the cause of aValidationError
, to display user-defined callback errors that caused theValidationError
aesthetically. Really useful for debugging as a user.Pre 3.11 it uses the exceptiongroup backport, 3.11+ uses the rust-native
BaseExceptionGroup
. In both cases, the cause is created lazily on read, so no performance cost. For everything other than PyPy, the lazy cause is only built once.The below example output is up to date.
Related issue number
pydantic/pydantic#6498 (comment)
Checklist
pydantic-core
(except for expected changes)3.11+
Pre 3.11 & PyPy:
Selected Reviewer: @dmontagu