Skip to content
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

add exception hierarchy pytests #3475

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidhewitt
Copy link
Member

WIP solution for #3452

The goal here is to get examples and documentation working for how to create a Python exception hierarchy and interact with it from Rust. This will probably go through a few rounds of rebases as features / bugfixes inspired by it will be opened as separate PRs.


#[pymethods]
impl CustomException {
#[new(signature = (*_args, **_kwargs))]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a bug here which confused me for a bit; #[new] ignores signature.

Either this should be a hard error or it should be equivalent to #[new] #[pyo3(signature = (*_args, **_kwargs))].

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Oct 11, 2023
@davidhewitt
Copy link
Member Author

@stinodego did you have any luck with an exception hierarchy on the polars side? I think this is a baseline which you might be able to start from, I think the most practical way forward is that if you try implementing what you want starting from this, please report back on your experience and we can fix any issues or papercuts.

@stinodego
Copy link

@stinodego did you have any luck with an exception hierarchy on the polars side? I think this is a baseline which you might be able to start from, I think the most practical way forward is that if you try implementing what you want starting from this, please report back on your experience and we can fix any issues or papercuts.

I haven't worked on this since I opened the issue - I'll take this as a template and try to set it up for Polars! Sometime next week probably. Will let you know my findings.

@stinodego
Copy link

stinodego commented Jan 10, 2024

Apologies for getting back to this so late. When I try to adapt this for Polars, I get an error that PyClass isn't implemented for PyException. Any idea how to solve this? I must be doing something wrong.

@davidhewitt
Copy link
Member Author

Is polars using the abi3 feature by any chance? That currently doesn't support subclassing native types (although I intend to fix that in the mid term).

@stinodego
Copy link

stinodego commented Jan 11, 2024

Ah that's right - we are using the abi3-py38 feature.

I have been able to get a basic version of a hierarchy working through the create_exception macros - I didn't realize it would allow me to do basically what I had intended:

create_exception!(polars.exceptions, PolarsBaseError, PyException);
create_exception!(polars.exceptions, ComputeError, PolarsBaseError);
create_exception!(polars.exceptions, ColumnNotFoundError, PolarsBaseError);

Though the setup in this PR definitely looks nicer and more powerful.

@davidhewitt
Copy link
Member Author

In the long run I would like to deprecate the create_exception macro and only support the #[pyclass] route as proposed here, but you highlight a good point here that I probably need to solve #1344 first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants