PyAddToModule: Properly propagate initialization error #3919
Conversation
There was a problem hiding this comment.
Thanks, couple of brief thoughts from me on this one. It might also be great to add a test, I think I had one somewhere anyway for functional modules.
I'm afraid there's been a large-ish churn in #3907 which you will need to fixup conflicts with.
|
@davidhewitt Thank you! Rebase done! Test added (I allowed myself to also add a test with raw identifiers) and usage of |
CodSpeed Performance ReportMerging #3919 will degrade performances by 10.46%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Looks perfect, thanks!
I will aim to review #3902 again asap, probably this evening or tomorrow morning.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
While the CI failure is a bit messy, I think it's genuine. What's happening is that |
Better than panics
| impl<$($generics,)*> $crate::impl_::pymodule::PyAddToModule for $name { | ||
| fn add_to_module( | ||
| module: &$crate::Bound<'_, $crate::types::PyModule>, | ||
| ) -> $crate::PyResult<()> { | ||
| use $crate::types::PyModuleMethods; | ||
| module.add( | ||
| <Self as $crate::PyTypeInfo>::NAME, | ||
| <Self as $crate::PyTypeInfo>::type_object_bound(module.py()), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This must be where PyCode_Type is being needed. Is this just for custom exceptions? Maybe we can move it to create_exception! macro.
There was a problem hiding this comment.
Yes but it means you can't reexpose a native Python type in your module. But it's probably not a big deal.
Instead, what about disabling PyCode on PyPy, this would remove a significant footgun? MR: #3934
There was a problem hiding this comment.
Yes disabling PyCode on PyPy would be correct I think.
Re-exposing a native Python type is an interesting idea. That could always be done manually in the #[pymodule_init]. Maybe that's sufficient?
There was a problem hiding this comment.
Re-exposing a native Python type is an interesting idea. That could always be done manually in the
#[pymodule_init]. Maybe that's sufficient?
Yes, it's definitely sufficient but maybe surprising for the user to have exceptions working and the other types not imho so if we can keep it, it's probably better. But I am happy with whatever you prefer.
Yes disabling PyCode on PyPy would be correct I think.
MR: #3934
There was a problem hiding this comment.
Thanks very much for fixing the PyPy (and chrono) issues!
I think my reluctance here is that I would quite like to remove create_exception! and just have #[pyclass] as the preferred way to create exception objects too. (#295 is the long history of that, I think it's nearly there.)
What do you think of the more extreme choice of removing this bit entirely? It might simplify things and then the rule is a bit simpler for the user, it'll only be #[pyclass] types which can be exported with #[pymodule_export].
There was a problem hiding this comment.
Anyway, I think to be honest this PR isn't the right discussion for my question, this functionality was already merged and is just being moved here in a way which happened to trigger the warning.
So let's proceed to merge once the PyPy issue is unblocked.
There was a problem hiding this comment.
Thanks very much for fixing the PyPy (and chrono) issues!
Thank you!
I think my reluctance here is that I would quite like to remove create_exception! and just have #[pyclass] as the preferred way to create exception objects too. (#295 is the long history of that, I think it's nearly there.)
Yes, it would be much nicer.
What do you think of the more extreme choice of removing this bit entirely? It might simplify things and then the rule is a bit simpler for the user, it'll only be #[pyclass] types which can be exported with #[pymodule_export].
It would indeed make the rule simpler. I am not sure the simplification implementation-wise is that big: the current implementation is just a single line relying only on public traits. If you think we will have #295 done before stabilizing declarative modules, indeed what you suggest seems like thebest way to go. In any case would love people to rely on #[pymodule_export] very rarely to make introspection/stub generation easier/better.
To summarize: I am fine with both options, pick the one that makes you the most comfortable.
Better than panics...