Use single-arg form of `#[pymodule]` function in docs and tests by maffoo · Pull Request #3899 · PyO3/pyo3

maffoo · 2024-02-26T00:58:11Z

As a follow-up to #3897, this deprecates the wrap_pyfunction macro and recommends replacing it with wrap_pyfunction_bound. As an experiment, I have also modified the pymodule macro to allow a single-argument form so that module functions can take just a single Bound<'_, PyModule> argument, since one can obtain a Python<'_> marker from the bound reference if needed. While this works, the macro implementation itself can probably be improved.

One question I have is around the macro hygiene and "no import" tests; when using &Bound<PyModule> instead of &PyModule, methods such as m.add_function are not available unless we also import the PyModuleMethods trait, so I've added an explicit use pyo3::types::PyModuleMethods in both those tests. For the "no import" test in particular this looks a little funny, so I'd be happy to know if there's a better approach.

LilyFirefly

Thanks for doing this!

I've added some thoughts below.

LilyFirefly · 2024-02-26T09:53:53Z

src/macros.rs

 /// This can be used with [`PyModule::add_function`](crate::types::PyModule::add_function) to add free
 /// functions to a [`PyModule`](crate::types::PyModule) - see its documentation for more information.
 #[macro_export]
+#[deprecated(since = "0.21.0", note = "Use `wrap_pyfunction_bound` instead.")]


Perhaps we should include ! to make it clear we mean a macro?

Suggested change

#[deprecated(since = "0.21.0", note = "Use `wrap_pyfunction_bound` instead.")]

#[deprecated(since = "0.21.0", note = "Use `wrap_pyfunction_bound!` instead.")]

LilyFirefly · 2024-02-26T10:00:51Z

tests/test_anyhow.rs


    Python::with_gil(|py| {
-        let func = wrap_pyfunction!(produce_ok_result)(py).unwrap();
+        let func = wrap_pyfunction_bound!(produce_ok_result, py).unwrap();


Is there a reason you changed the calling style here? For consistency across the codebase?

It might be valuable to test both styles somewhere. (Maybe we already do?)

Yeah, I'd be happy to change back if desired; I didn't notice whether there are explicit tests of both styles somewhere. TBH I didn't realize the single-arg call to make a closure was even a thing, but in all these cases where the closure is invoked immediately it seems like one call with two args is more clear.

LilyFirefly · 2024-02-26T10:05:17Z

pyo3-macros-backend/src/module.rs

-        #vis mod #ident {
-            #initialization
-        }
+    if function.sig.inputs.len() == 1 {


I think I like what this allows us to do, but I don't like the duplication here in the macro. I wonder if there's a way to get the best of both worlds?

In the future I think it would be helpful to add new functionality like this in a separate PR. That way it's easier to separate the changes we already know we want from those that we need to consider.

I agree this is a nice addition, but we should split this off as a separate PR. The changes to wrap_pyfunction(_bound) already have quite a big diff. (And since this is already in a separate commit, it should be easy to split off).

As for the implementation, maybe we can do something similar to #[pyfunction] which can already do this. I think this is the relevant code for that. This should avoid a lot of this duplication

Thanks for the link to the pyfunction code. I will take a look at that and split off the macro changes as a separate PR. I'd prefer to land that one first so that the wrap_pyfunction_bound changes can use the single-arg form for pymodule functions. WDYT?

I'd be fine with that. The changes are only tangentially related, so the order they land in does not matter to much I think.

LilyFirefly · 2024-02-26T10:11:14Z

newsfragments/3899.changed.md

@@ -0,0 +1,2 @@
+The `wrap_pyfunction!` macro is now deprecated. The `wrap_pyfunction_bound!` macro, which returns a `Bound<'_, PyCFunction>` should be used instead.


We might not actually need this bit, because it's part of the GIL Ref refactor.

Yeah, we skipped all the other gil-ref refactorings from the changelog, so I don't think we should start that here now.

The change to the #[pymodule] macro should be mentioned, but as in the other comment, I think we should land that separately.

I will split off the pymodule macro changes, but I'm not exactly sure what your suggestion is for this news fragment. Are you suggesting I keep the notice about deprecating wrap_pyfunction! but omit the reference to Bound, or should I not add a newsfragment at all?

I think you can remove the fragment here, and then add one with the notice about the #[pymodule] change in that new PR.

Icxolu

Thanks for taking this on! There are still some open questions in #3897, so it might take a few days until that lands. In the meanwhile I've added some comments/thoughts below.

Icxolu · 2024-02-26T17:49:46Z

src/tests/hygiene/pymodule.rs

+#[crate::pymodule]
+#[pyo3(crate = "crate")]
+fn my_module_bound(_py: crate::Python<'_>, m: &crate::Bound<'_, crate::types::PyModule>) -> crate::PyResult<()> {
+    use crate::types::PyModuleMethods;


I believe the point of this test is, to verify that this works without additional imports. So the macro code needs to be adapted that this is also generated.

Yeah, I feel like the hygiene and no-import tests are a bit at odds in this regard, because having things work without extra imports means that the macro would need to introduce a hidden import of the PyModuleMethods trait into the namespace, which feels unhygienic. IMO it makes sense that if the user wants to call m.add_function on a Bound<PyMethod> they should have to explicitly import PyModuleMethods, just like they would have to do if making this call without a macro. I noticed that in test_no_imports.rs we were already pulling in PyAnyMethods, presumably for the same reasons, so I've added PyModuleMethods there. For the hygiene tests, I've changed to using the syntax for qualified trait methods, instead of useing the trait.

Yeah, I can see your point here. These traits are all new, so I guess we need to figure out how we want to handle them.

I guess the main point to test here is that the macro expansion compiles without additional imports. So fully qualified syntax seems appropriate here.

Good catch with PyAnyMethods in test_no_imports.rs seems like I added that 😇, shame on me 😆. I think that's only needed for test_basic, so maybe we should move that local instead of moving PyModuleMethods global? That way we can maybe at least somewhat verify that macro expansion does not rely on those imports, while still keeping this close to what a user might write.

After writing this, I think I agree that we should not generate that import silently.

I think most code won't actually need to explicitly add PyAnyMethods because it's in the prelude.

Icxolu · 2024-02-26T17:50:31Z

tests/test_coroutine.rs

-                wrap_pyfunction!(my_fn, gil).unwrap().as_borrowed().as_any(),
+                wrap_pyfunction_bound!(my_fn, gil)
+                    .unwrap()
+                    .as_borrowed()


.as_borrowed should not be necessary anymore, because this is already a Bound now

Icxolu · 2024-02-26T17:53:15Z

tests/test_no_imports.rs

+
+#[pyo3::pymodule]
+fn basic_module_bound(_py: pyo3::Python<'_>, m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> {
+    use pyo3::types::PyModuleMethods;


Same here, this should be generated by the macro

See comment above about hygiene test.

Icxolu · 2024-02-26T18:09:48Z

pyo3-macros-backend/src/module.rs

-        #vis mod #ident {
-            #initialization
-        }
+    if function.sig.inputs.len() == 1 {


I agree this is a nice addition, but we should split this off as a separate PR. The changes to wrap_pyfunction(_bound) already have quite a big diff. (And since this is already in a separate commit, it should be easy to split off).

As for the implementation, maybe we can do something similar to #[pyfunction] which can already do this. I think this is the relevant code for that. This should avoid a lot of this duplication

Icxolu · 2024-02-26T18:15:21Z

newsfragments/3899.changed.md

@@ -0,0 +1,2 @@
+The `wrap_pyfunction!` macro is now deprecated. The `wrap_pyfunction_bound!` macro, which returns a `Bound<'_, PyCFunction>` should be used instead.


Yeah, we skipped all the other gil-ref refactorings from the changelog, so I don't think we should start that here now.

The change to the #[pymodule] macro should be mentioned, but as in the other comment, I think we should land that separately.

maffoo · 2024-02-28T00:49:34Z

Now that #3897 and #3905 have been merged, I've reworked this to not deprecated wrap_pyfunction! but instead just to use the single-arg form of pymodule functions in a number of places in docs and tests. @davidhewitt, please take a look.

davidhewitt · 2024-02-28T08:23:35Z

Thanks, I will aim to review this tonight or tomorrow 👍

LilyFirefly

That's much nicer!

Icxolu

Very nice! The modules feel very clean now 🎉

guide/src/function.md

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

davidhewitt

Agreed, looks brilliant, thank you!

I'd like to make sure we continue to test the old form so that we don't end up with all the existing code being broken accidentally, other thank that, looks great to merge!

davidhewitt · 2024-02-28T19:45:46Z

tests/test_module.rs

Let's perhaps have one test in this file with the two-argument form, just so that we can be sure that continues to work (even if we're not encouraging new usage of it).

Added a test with a two-argument module function.

codspeed-hq · 2024-02-28T21:08:48Z

CodSpeed Performance Report

Merging #3899 will improve performances by 12.48%

_{Comparing maffoo:wrap-pyfunction (106a471) with main (a15e4b1)}

Summary

⚡ 2 improvements
✅ 65 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`maffoo:wrap-pyfunction`	Change
⚡	`test_simple_args_kwargs_rs`	35.3 µs	31.7 µs	+11.42%
⚡	`test_simple_kwargs_rs`	35 µs	31.2 µs	+12.48%

davidhewitt

Looks great! Thanks again 👍

* Use single-arg form for `#[pymodule]` functions in docs and tests * Update guide/src/function.md Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> * Add test of two-argument module function * Fix new test --------- Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

maffoo force-pushed the wrap-pyfunction branch from aae06e9 to a0a71d8 Compare February 26, 2024 02:02

LilyFirefly reviewed Feb 26, 2024

View reviewed changes

Icxolu reviewed Feb 26, 2024

View reviewed changes

Icxolu added the CI-skip-changelog Skip checking changelog entry label Feb 26, 2024

This was referenced Feb 26, 2024

Pymodule bound #3897

Merged

Allow #[pymodule] functions to take a single module arg #3905

Merged

Use single-arg form for #[pymodule] functions in docs and tests

1486fe6

maffoo force-pushed the wrap-pyfunction branch from 9c62e38 to 1486fe6 Compare February 28, 2024 00:47

maffoo changed the title ~~Deprecate wrap_pyfunction macro in favor of wrap_pyfunction_bound~~ Use single-arg form of #[pymodule] function in docs and tests Feb 28, 2024

maffoo requested review from Icxolu and LilyFirefly February 28, 2024 00:49

LilyFirefly approved these changes Feb 28, 2024

View reviewed changes

Icxolu approved these changes Feb 28, 2024

View reviewed changes

guide/src/function.md Outdated Show resolved Hide resolved

Update guide/src/function.md

e4d7355

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

davidhewitt reviewed Feb 28, 2024

View reviewed changes

Add test of two-argument module function

be1b4a3

maffoo requested a review from davidhewitt February 28, 2024 20:38

Fix new test

106a471

davidhewitt approved these changes Feb 28, 2024

View reviewed changes

davidhewitt enabled auto-merge February 28, 2024 21:41

davidhewitt added this pull request to the merge queue Feb 28, 2024

Merged via the queue into PyO3:main with commit 68ec6de Feb 29, 2024

maffoo deleted the wrap-pyfunction branch May 28, 2024 04:02

	#[deprecated(since = "0.21.0", note = "Use `wrap_pyfunction_bound` instead.")]
	#[deprecated(since = "0.21.0", note = "Use `wrap_pyfunction_bound!` instead.")]

		@@ -0,0 +1,2 @@
		The `wrap_pyfunction!` macro is now deprecated. The `wrap_pyfunction_bound!` macro, which returns a `Bound<'_, PyCFunction>` should be used instead.

Conversation

maffoo commented Feb 26, 2024

Uh oh!

LilyFirefly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icxolu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maffoo commented Feb 28, 2024

Uh oh!

davidhewitt commented Feb 28, 2024

Uh oh!

LilyFirefly left a comment

Choose a reason for hiding this comment

Uh oh!

Icxolu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #3899 will improve performances by 12.48%

Summary

Benchmarks breakdown

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Feb 28, 2024 •

edited

Loading