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

PyAddToModule: Properly propagate initialization error #3919

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Mar 1, 2024

Better than panics...

@Tpt Tpt mentioned this pull request Mar 1, 2024
12 tasks
@Tpt Tpt force-pushed the type-object-bound branch from 6ebb2ea to 13775f9 Compare March 1, 2024 14:16
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

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.

newsfragments/3919.changed.md Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
@Tpt Tpt force-pushed the type-object-bound branch from 13775f9 to 59a6bd1 Compare March 5, 2024 10:02
@Tpt
Copy link
Contributor Author

Tpt commented Mar 5, 2024

@davidhewitt Thank you! Rebase done! Test added (I allowed myself to also add a test with raw identifiers) and usage of add_class too.

Copy link

codspeed-hq bot commented Mar 5, 2024

CodSpeed Performance Report

Merging #3919 will degrade performances by 10.46%

Comparing Tpt:type-object-bound (078ffb0) with main (57bbc32)

Summary

❌ 1 regressions
✅ 66 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main Tpt:type-object-bound Change
extract_str_extract_success 951.1 ns 1,062.2 ns -10.46%

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Mar 5, 2024
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks perfect, thanks!

I will aim to review #3902 again asap, probably this evening or tomorrow morning.

@davidhewitt davidhewitt added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@davidhewitt
Copy link
Member

While the CI failure is a bit messy, I think it's genuine. What's happening is that PyCode_Type isn't available on PyPy, and pyo3-ffi isn't removing this with #[cfg] so we're dying with a linker error.

@Tpt Tpt force-pushed the type-object-bound branch from 59a6bd1 to 078ffb0 Compare March 6, 2024 08:25
Comment on lines +253 to +263
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()),
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This must be where PyCode_Type is being needed. Is this just for custom exceptions? Maybe we can move it to create_exception! macro.

Copy link
Contributor Author

@Tpt Tpt Mar 6, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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].

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@davidhewitt davidhewitt added this pull request to the merge queue Mar 6, 2024
Merged via the queue into PyO3:main with commit fbd5311 Mar 6, 2024
41 of 42 checks passed
@Tpt Tpt deleted the type-object-bound branch March 6, 2024 20:05
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.

3 participants