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

#[pymodule] mod some_module { ... } v3 #3815

Merged
merged 7 commits into from
Feb 24, 2024
Merged

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Feb 9, 2024

Based on #2367 and #3294

Allows to export classes, native classes, functions and submodules and provide an init function

See test/test_module.rs for an example

Future work:

  • update examples, README and guide
  • investigate having #[pyclass] and #[pyfunction] directly in the #[pymodule]
  • properly register submodules in sys.modules

@Tpt Tpt force-pushed the mod-module branch 2 times, most recently from 846e128 to 6adfe20 Compare February 9, 2024 15:29
Copy link

codspeed-hq bot commented Feb 9, 2024

CodSpeed Performance Report

Merging #3815 will not alter performance

Comparing Tpt:mod-module (391b8f9) with main (0f92b67)

Summary

✅ 67 untouched benchmarks


impl<T: PyTypeInfo> PyAddToModule for T {
fn add_to_module(module: &PyModule) -> PyResult<()> {
module.add(Self::NAME, Self::type_object(module.py()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add_class uses T::lazy_type_object().get_or_try_init(py)? instead of T::type_object(py). Is there a reason to prefer one or the other?

Copy link
Member

Choose a reason for hiding this comment

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

T::lazy_type_object().get_or_try_init(py)? will fail more gracefully instead of panic if for some reason constructing the #[pyclass] fails, so it'd be nice to keep that property if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I have added PyTypeInfo::try_type_object_bound. Not sure if it's the best name.

@davidhewitt
Copy link
Member

👍 thank you; I will do my best to review fully tonight or tomorrow.

@Tpt
Copy link
Contributor Author

Tpt commented Feb 10, 2024

👍 thank you; I will do my best to review fully tonight or tomorrow.

Thank you! The day when I have a look a PyO3 things is Friday so if you ask me any significant revision it's going to wait for next Friday. So, please, don't do it this week-end if you have other things to do.

@davidhewitt
Copy link
Member

👍 based on that, and the volume of reviews I've been keeping up with, I'm now aiming to read this tomorrow! 😂

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.

Thank you for picking this up! Overall I think this looks good, and while there's bits we will definitely want like #[pyclass] and #[pyfunction] direct support, this is already a great first step.

I think the thing which I'd like discussed further is the new #[pyo3] use ... and #[pymodule_init] attributes?

For #[pyo3] use, I'm so used to seeing #[pyo3(...)] with options that #[pyo3] without them just looks a bit funny to me now, but maybe it is the right choice. #[py] use? #[py(use)]? #[pyo3(use)]? #[pyimport]?

For #[pymodule_init], similar bike shedding. #[pyo3(module_init)]? Could maybe even just be fn __module_init__ and we pick up the magic dunder name?

All ideas welcome!

pytests/src/lib.rs Outdated Show resolved Hide resolved

impl<T: PyTypeInfo> PyAddToModule for T {
fn add_to_module(module: &PyModule) -> PyResult<()> {
module.add(Self::NAME, Self::type_object(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.

T::lazy_type_object().get_or_try_init(py)? will fail more gracefully instead of panic if for some reason constructing the #[pyclass] fails, so it'd be nice to keep that property if we can.

pyo3-macros/src/lib.rs Outdated Show resolved Hide resolved
pytests/src/lib.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/module.rs Show resolved Hide resolved
pyo3-macros-backend/src/module.rs Show resolved Hide resolved
pyo3-macros-backend/src/module.rs Outdated Show resolved Hide resolved
pytests/src/lib.rs Outdated Show resolved Hide resolved
@Tpt
Copy link
Contributor Author

Tpt commented Feb 16, 2024

Thank you for the review!

For #[pyo3] use, I'm so used to seeing #[pyo3(...)] with options that #[pyo3] without them just looks a bit funny to me now, but maybe it is the right choice. #[py] use? #[py(use)]? #[pyo3(use)]? #[pyimport]?

I agre that #[pyo3] without options is strange. My impression is that it's only use to set modifiers inside other macros. What about #[pyexport]? "exporting" is more or less what the macro is doing.

For #[pymodule_init], similar bike shedding. #[pyo3(module_init)]? Could maybe even just be fn __module_init__ and we pick up the magic dunder name?

#[pyo3(module_init)] is weird imho, the other usages of #[pyo3] seems to me mostly for options, not completely changing behaviors. __module_init__ is a bit strange because this is not related to a Python magic function. What about keeping #[pymodule_init]?

Note: I still got two things to fix:

@Tpt Tpt force-pushed the mod-module branch 7 times, most recently from 72495f1 to f71feec Compare February 19, 2024 09:52
@davidhewitt
Copy link
Member

👍thanks! I will do my best to give this a second review tonight.

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, overall this looks great and I'm keen to use this myself!

I think the TypeInfo changes can / should probably be reverted, and let's make a final decision on a better name for the "use" attribute than #[pyo3].

Regarding releasing this, I'm trying to decide if it's better to release this in 0.21 or 0.22. There's still a couple of features missing from this like supporting the function / class inside the module and it'd be nice to have those as well as make this a headline feature in its own right rather than a secondary note behind the huge Bound API changes.

I don't want this to get into a state with horrible conflicts, nor block the rest of your progress. I wonder, can we do something a bit unorthodox like merge it anyway but hide it behind a secret environment variable?

Or we just include it in 0.21 but call it experimental and aim for complete and documented support in 0.22. Maybe that's fine.

Comment on lines 99 to 103
/// Returns the safe abstraction over the type object or an error if initialization fails
#[inline]
fn try_type_object_bound(py: Python<'_>) -> PyResult<Bound<'_, PyType>> {
Ok(Self::type_object_bound(py))
}
Copy link
Member

Choose a reason for hiding this comment

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

While I think there's potentially value in having a fallible way to get the type object on this trait, I think that's probably a design question worth having in its own PR.

Since the previous week I think LazyTypeObject now uses the Bound API, so I think you should be able to use that again without needing this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I think there's potentially value in having a fallible way to get the type object on this trait, I think that's probably a design question worth having in its own PR.

Agreed

Since the previous week I think LazyTypeObject now uses the Bound API, so I think you should be able to use that again without needing this here.

I have rebased my MR and I am using it in dc81d76

About why this new function, my reasoning is: In the add_to_module function implemented on types I need to find a way to call LazyTypeObject::get_or_try_init on pyclass derivation to properly propagate errors while calling the usual PyTypeInfo::type_object_bound on other types.
I see two ways to archive this:

  • make #[pyclass] generate a static add_to_module function like #[pymodule] and #[pyfunction] and implement the PyAddToModule trait on all types that are not created via #[pyclass] (custom exceptions...). To archive this easily I need a trait that is implemented by everyone but types created by #[pyclass]. I have not found such trait and was a bit reluctant to implement it.
  • The approach in dc81d76 you have reviewed and that I have reverted.

If the new MR state without dc81d76 is fine with you we can maybe merge it and figure out a way to call LazyTypeObject::get_or_try_init in a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

Yep understood, let's work out this in a follow-up. Even some builtin types like the datetime types can fail to import, e.g. #3818, so there is definitely room for PyO3's existing API to improve.

tests/ui/invalid_pymodule_in_root_module.rs Outdated Show resolved Hide resolved
pytests/src/lib.rs Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

Or we just include it in 0.21 but call it experimental and aim for complete and documented support in 0.22. Maybe that's fine.

I'd say experimental-inspect as a feature name provides some precedence here, e.g. experimental-declarative-modules?

@davidhewitt
Copy link
Member

davidhewitt commented Feb 21, 2024

I'd say experimental-inspect as a feature name provides some precedence here, e.g. experimental-declarative-modules?

Seems reasonable to me, I just added a commit for testing feature groups in #3834 which will extend to this trivially.

@Tpt
Copy link
Contributor Author

Tpt commented Feb 21, 2024

Thank you!

I think the TypeInfo changes can / should probably be reverted, and let's make a final decision on a better name for the "use" attribute than #[pyo3].

Done. Code in dc81d76

Or we just include it in 0.21 but call it experimental and aim for complete and documented support in 0.22. Maybe that's fine.
I'd say experimental-inspect as a feature name provides some precedence here, e.g. experimental-declarative-modules?
Seems reasonable to me, I just added a commit for testing feature groups in #3834 which will extend to this trivially.

Sounds great to me too! I have implemented it!

Tpt and others added 6 commits February 23, 2024 10:12
Based on PyO3#2367 and PyO3#3294

Allows to export classes, native classes, functions and submodules and provide an init function

See test/test_module.rs for an example

Future work:
- update examples, README and guide
- investigate having #[pyclass] and #[pyfunction] directly in the #[pymodule]

Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: Georg Brandl <[email protected]>
@davidhewitt
Copy link
Member

Thanks for rebasing! I'll catch up with all the discussion and give a final review here a bit later, hopefully with intent to merge!

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, I am happy with this implementation and very excited to see this feature making progress. I've wanted this literally for years, see #694 - nearly the first issue I opened on PyO3 back from when I first started contributing 😂

Are you happy to make an issue with all the next steps agreed from this PR? That way we can be sure to remember everything. If we describe the design relatively clearly we might find other contributors interested in taking on pieces of the puzzle too.

Here's a few thoughts I've had just now in addition to the other comments scattered across this PR:

  • documentation (e.g. in the guide) - even for 0.21 it would be nice to mention this experimental feature (but it wouldn't be a blocker)
  • let's consider deprecating #[pyfn] once this is stable
  • it would be nice to make #[pymodule_init] accept -> () or -> Result<()> as the return type, to remove the #[clippy::unnecessary_wraps] bit that sometimes comes up
  • it would be nice to explore the relationship between this and "two-phase initialization e.g. Implement PEP 489 - Multi-phase extension module initialization #2245. In particular I think we should look at #[pymodule_data] as a way to create a per-module data mechanism, but that can come much later.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 24, 2024
Merged via the queue into PyO3:main with commit e0e3981 Feb 24, 2024
40 checks passed
@Tpt Tpt deleted the mod-module branch February 24, 2024 17:17
@Tpt Tpt mentioned this pull request Feb 26, 2024
12 tasks
@Tpt
Copy link
Contributor Author

Tpt commented Feb 26, 2024

Thank you!

Are you happy to make an issue with all the next steps agreed from this PR? That way we can be sure to remember everything. If we describe the design relatively clearly we might find other contributors interested in taking on pieces of the puzzle too.

Done: #3900 Please add anything I missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants