PyModule in #[pyfunction] #1143

sebpuetz · 2020-09-02T16:20:49Z

Closes #828

Error handling for module names
how to signal when PyModule should be passed
Documentation

davidhewitt

Thanks, I have a few quick thoughts on this:

👍 on adding add_function, and add_module, though I would like to also keep support for add_wrapped for now, because I don't see a strong reason that users would need to migrate existing code. I'm thinking in 0.12 we can add a doc to add_wrapped saying prefer the new methods, and in 0.13 we can mark as deprecated or consider a broader cleanup. (I still would love to have the new module syntax discussed in Unify #[pyfunction] and #[pyfn] #694)
Similarly, I think asking users to migrate to #[pyfunction(free)] and also changing the arguments to wrap_pyfunction! is quite a lot of churn to existing code. I agree you're probably right that most pyfunctions want to be part of modules, but I think we don't necessarily need to force users to change existing code immediately.

I've added an inline comment with a suggestion of how we might be able to achieve a similar result to what you've got, but without forcing ecosystem churn.

davidhewitt · 2020-09-02T19:17:14Z

pyo3-derive-backend/src/module.rs

    Ok(quote! {
-        fn #function_wrapper_ident(py: pyo3::Python) -> pyo3::PyObject {
+        fn #function_wrapper_ident(py: pyo3::Python #maybe_module_arg) -> pyo3::PyObject {


How about here, instead of requiring the module arg optionally depending on the free attribute, we add a little trait to derive_utils, e.g.

trait WrapPyFunctionArguments<'a> { fn arguments(self) -> (Python<'a>, Option<&'a PyModule>); } impl<'a> WrapPyFunctionArguments<'a> for Python<'a> { fn arguments(self) -> (Python<'a>, Option<&'a PyModule>>) { (self, None) } } impl<'a> WrapPyFunctionArguments<'a> for &'a PyModule { fn arguments(self) -> (Python<'a>, Option<&'a PyModule>>) { (self.py(), Some(self)) } }

Then it should be possible to call either wrap_pyfunction!(foo)(py) or wrap_pyfunction!(foo)(module) if we change this line to:

Suggested change

fn #function_wrapper_ident(py: pyo3::Python #maybe_module_arg) -> pyo3::PyObject {

fn #function_wrapper_ident(args: impl pyo3::derive_utils::WrapPyFunctionArguments) -> pyo3::PyObject {

And in this generated wrapper code here we could do different things depending on whether a PyModule is provided.

~~EDIT: This idea might not work because wrap_pyfunction! takes a reference to the wrapper, and by adding the impl Trait argument I don't think it can have a reference to it.~~

~~If that's the case, I'd still like us to look for a similar trick so that the new functionality comes with new syntax, without changing the existing downstream code.~~

EDIT 2: Looks like taking reference to functions with impl Trait arguments can work fine: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bb49984a3d7a9ec9277d7f1cd0b5ee8e

sebpuetz · 2020-09-02T19:32:12Z

Thanks for the comments!

Similarly, I think asking users to migrate to #[pyfunction(free)] and also changing the arguments to wrap_pyfunction! is quite a lot of churn to existing code. I agree you're probably right that most pyfunctions want to be part of modules, but I think we don't necessarily need to force users to change existing code immediately.

I don't think there are many uses of pyfunction(free) at all! It's fairly unnatural for a pyfunction to not be associated with a PyModule. Almost every case within the pyo3 codebase that needed to be fixed after this change was about tests where no module was set up and code was called from within Rust. So I don't think that many users would even notice this change!

I'll look into implementing your suggestions tomorrow!

davidhewitt · 2020-09-02T20:27:43Z

I don't think there are many uses of pyfunction(free) at all!

👍 I guess you're probably right about this! It's sometimes hard to judge how much certain patterns are used downstream.

My point still stands that if we can have our cake and eat it (i.e. add new feature without changing any existing code), that'd be the best thing =)

davidhewitt

🎉 looks like my suggestion worked!

sebpuetz · 2020-09-03T11:51:28Z

This works pretty well, although figuring out the lifetimes was a bit of work. Nice suggestions!

Now we have to figure out a way to signal which &PyModule argument in the signature is the one that we should inject.

davidhewitt · 2020-09-03T11:58:02Z

src/derive_utils.rs

+
+/// Trait to abstract over the arguments of Python function wrappers.
+#[doc(hidden)]
+pub trait WrapPyFunctionArguments<'a, 'b> {


Is there a reason to have two lifetime arguments in this trait? If we set them to the same, then Rust can just shorten the longer lifetime to match, which seems good enough to me.

Also, looking again at this trait, I noticed a different pattern to achieve the same thing:

pub enum WrapPyFunctionArguments<'a> { Python(Python<'a>), Module(&'a PyModule) }

And can then just implement Into<WrapPyFunctionArguments> for each of Python and &PyModule, and use impl Into<WrapPyFunctionArguments> in the generated wrapper.

I think that maybe the enum is less confusing to understand in the long run, I'm also happy to leave it as-is (as long as we take the second lifetime argument out). Just an idea in cause you find you prefer one design over the other.

Is there a reason to have two lifetime arguments in this trait? If we set them to the same, then Rust can just shorten the
longer lifetime to match, which seems good enough to me.

That's weird, I was trying that for almost an hour, now I went back to verify that it in fact doesn't work...and...it compiles!

I'll check how nice the enum variant turns out

sebpuetz · 2020-09-03T12:34:50Z

PyPy doesn't seem to have a PyModule_GetNameObject / PyPyModule_GetNameObject function, currently digging through PyPy source...

kngwyu · 2020-09-03T12:43:50Z

@sebpuetz
https://foss.heptapod.net/pypy/pypy/-/blob/branch/default/pypy/module/cpyext/modsupport.py#L136
Looks it has only PyModule_GetName

sebpuetz · 2020-09-03T12:44:49Z

Yeah, I'm now going through PyModule::name() instead, I'll also open a PR that feature-gates the GetNameObject

kngwyu

Thanks, nicely designed 👍

BTW, how about changing the argument of add_function to wrapper: &impl Fn(&'a Self) -> PyResult<PyObject>?

kngwyu · 2020-09-03T12:44:47Z

pyo3-derive-backend/src/module.rs

+            args: impl Into<pyo3::derive_utils::WrapPyFunctionArguments<'a>>
+        ) -> pyo3::PyObject {
+            let arg = args.into();
+            let (py, maybe_module) = match arg {


How about making this a method of WrapPyFunctionArguments?

PyFunction_New was previously implemented as a Rust function wrapper around a call to the extern C function PyFunction_NewExt with a hard-coded third argument. This commit removes the Rust wrapper and directly exposes the function from the CPython API.

Previously neither the module nor the name of the module of pyfunctions were registered. This commit passes the module and its name when creating a new pyfunction. PyModule::add_function and PyModule::add_module have been added and are set to replace `add_wrapped` in a future release. `add_wrapped` is kept for compatibility reasons during the transition. Depending on whether a `PyModule` or `Python` is the argument for the Python function-wrapper, the module will be registered with the function.

sebpuetz · 2020-09-03T12:52:43Z

BTW, how about changing the argument of add_function to wrapper: &impl Fn(&'a Self) -> PyResult<PyObject>?

Looking into this now, there are also a few expect() calls in these add functions that should be replaced with ?.

sebpuetz · 2020-09-03T13:11:35Z

I don't see how that's possible while not breaking existing code:

If add_function takes wrapper: &impl Fn(&'a Self) -> PyResult<PyObject>, we need to change the code-gen accordingly to return -> PyResult<PyObject> wrappers.

If the proc-macro generates -> PyResult<PyObject> wrappers, the old add_wrapped way breaks, because the argument in add_wrapped is wrapper: &impl Fn(&'a Self) -> PyObject.

Fwiw, I'm fine with making that breaking change and have all add_X fns on PyModule take wrappers that return results. With my proposed solution we'd see some weird behaviour, if PyModule::name() fails while creating the method, PyErr is converted to PyObject and returned. The error only gets recognized when function.getattr(self.py(), "__name__")?; fails - assuming that PyErr doesn't have a __name__ attribute.

So, should we change all add functions to take -> PyResult<PyObject> wrappers?

The other solution is complicating the codegen and create different wrappers...

kngwyu · 2020-09-03T13:18:51Z

I don't see how that's possible while not breaking existing code:

If add_wrapped(wrap_*!()) still works, I think it's not very breaking and OK.

@kngwyu

Suggestion by @kngwyu. Additionally replace some `expect` calls with error handling.

sebpuetz · 2020-09-03T13:23:03Z

Well, that doesn't work because the argument to add_wrapped() is required to be a function returning PyResult<PyObject>. I'll push how it'd look as an additional commit in a couple of minutes. We can then decide which version we want.

@kngwyu

Wrapping a function can fail if we can't get the module name. Based on suggestion by @kngwyu

sebpuetz · 2020-09-03T13:52:20Z

I think I found a solution by making the return value of the wrapper fn T: IntoPyCallback<PyObject> in add_wrapped. I'm not getting compilation errors when using add_wrapped(wrap_pyfunction!()) now, so I hope that this is a workable solution.

src/types/module.rs

This commit makes it possible to access the module of a function by passing the `need_module` argument to the pyfn and pyfunction macros.

davidhewitt

Thanks, this looks great to me🚀

Great docs and tests much appreciated 😊

Couple of final suggestions from me.

davidhewitt · 2020-09-05T07:30:00Z

guide/src/function.md

+use pyo3::wrap_pyfunction;
+use pyo3::prelude::*;
+
+#[pyfunction(need_module)]


Bikeshedding: I'd like to propose to call the attribute pass_module.

Motivation is that this was already the verb used in #828

Also I've seen this phrasing before e.g. in Python's click library: https://click.palletsprojects.com/en/7.x/api/#click.pass_context

davidhewitt · 2020-09-05T07:31:24Z

guide/src/function.md

+
+### Accessing the module of a function
+
+Functions are usually associated with modules, in the C-API, the self parameter in a function call corresponds


I think this first sentence is probably an implementation detail that the user doesn't need to know? Might want to take it out for simplicity.

davidhewitt · 2020-09-05T07:33:52Z

guide/src/function.md

+# fn main() {}
+```
+
+If `need_module` is set, the first argument **must** be the `&PyModule`. It is then possible to interact with


Maybe "it is then possible to use the module in the function body"?

davidhewitt · 2020-09-05T07:36:37Z

guide/src/function.md

+# fn main() {}
+```
+
+Within Python, the name of the module that a function belongs to can be accessed through the `__module__`


I think this is probably true of all Python functions (although you've just fixed this for pyO3) so not sure this sentence needs to be in this section?

davidhewitt · 2020-09-05T07:45:06Z

Oh also, CHANGELOG entry please! 😄

sebpuetz · 2020-09-05T08:33:24Z

Looks pretty green to me ;)

guide/src/function.md

birkenfeld · 2020-09-05T08:51:52Z

src/types/module.rs

+    /// You can also add a function with a custom name using [add](PyModule::add):
+    ///
+    /// ```rust,ignore
+    /// m.add("also_double", wrap_pyfunction!(double)(py, m));


Does this need to handle the Result from wrap_pyfunction?

Ah, good spot. This I think is an outdated doc and probably shows why using ignore in docstrings is usually a bad idea! Maybe this should be updated to not use ignore?

Yeah, nice catch. Looks we need to update this.

I made all the examples in the docs of fn add_X runnable.

src/types/module.rs

tests/test_module.rs

birkenfeld · 2020-09-05T08:55:53Z

src/types/module.rs

@@ -194,11 +195,50 @@ impl PyModule {
    /// ```rust,ignore
    /// m.add("also_double", wrap_pyfunction!(double)(py));


arguments are (py) here, but (py, m) below?

I missed these because the tests are ignored, I'll go through the guide again to make the add_X calls consistent. The correct way to add a function is to pass the PyModule to the wrapper. Python can be acquired from PyModule.

(py, m) was an outdated design we removed. ignore docstring example probably was a mistake.

wrap_pyfunction! output can take either py (for backwards compatibility, will probably deprecate eventually) or m (preferred, sets the __module__ correctly).

OK, thanks!

The C-exported wrapper generated through `#[pymodule]` is only required for the top-level module.

sebpuetz · 2020-09-05T13:55:47Z

@davidhewitt I pushed the changes discussed in #1149 as a new commit

davidhewitt · 2020-09-05T14:05:24Z

Thanks, I'll try to review this later today (I have some related experiments from importing submodules for #759 from a few weeks' ago).

kngwyu · 2020-09-06T04:51:48Z

I pushed the changes discussed in #1149 as a new commit

LGTM, let's merge this after David reviews.

davidhewitt · 2020-09-06T09:38:27Z

👍 I did some thinking this morning on this and I believe it will cause no issues with what I was trying out; I'll confirm for definite later.

davidhewitt · 2020-09-06T19:54:55Z

Yep looks great, let's merge this! 🎉

davidhewitt · 2020-09-06T19:55:24Z

Thank you for your hard work on this!

The change to wrap_pyfunction!() PyO3#1143 makes it impossible to implement `context.add_wrapped(wrap_pyfunction!(something))` in `inline-python`. `context` does not carry the GIL lifetime, which causes type deduction trouble now that `wrap_pyfunction` results in a generic function. ``` error[E0308]: mismatched types --> examples/rust-fn.rs:12:4 | 12 | c.add_wrapped(wrap_pyfunction!(rust_print)); | ^^^^^^^^^^^ one type is more general than the other | = note: expected enum `Result<&pyo3::types::PyCFunction, _>` found enum `Result<&pyo3::types::PyCFunction, _>` ``` By re-wrapping the function as a closure, we trigger 'closure signature hinting' when passing `wrap_pyfunction!()` as an argument to a function: Rustc will set the signature of the closure from the function that closure is passed to. This way, the generic arguments can be deduced in more contexts, fixing the problem.

The change to wrap_pyfunction!() PyO3#1143 makes it impossible to implement `context.add_wrapped(wrap_pyfunction!(something))` in `inline-python`. `context` does not carry the GIL lifetime, which causes type deduction trouble now that `wrap_pyfunction` results in a generic function. ``` error[E0308]: mismatched types --> examples/rust-fn.rs:12:4 | 12 | c.add_wrapped(wrap_pyfunction!(rust_print)); | ^^^^^^^^^^^ one type is more general than the other | = note: expected enum `Result<&pyo3::types::PyCFunction, _>` found enum `Result<&pyo3::types::PyCFunction, _>` ``` By re-wrapping the function as a closure, we trigger 'closure signature deduction' when passing `wrap_pyfunction!()` as an argument to a function: Rustc will deduce the signature of the closure from the function that closure is passed to. This way, the generic arguments can be deduced in more contexts, fixing the problem.

The change to wrap_pyfunction!() in PyO3#1143 makes it impossible to implement `context.add_wrapped(wrap_pyfunction!(something))` in `inline-python`. `context` does not carry the GIL lifetime, which causes type deduction trouble now that `wrap_pyfunction` results in a generic function. ``` error[E0308]: mismatched types --> examples/rust-fn.rs:12:4 | 12 | c.add_wrapped(wrap_pyfunction!(rust_print)); | ^^^^^^^^^^^ one type is more general than the other | = note: expected enum `Result<&pyo3::types::PyCFunction, _>` found enum `Result<&pyo3::types::PyCFunction, _>` ``` By re-wrapping the function as a closure, we trigger 'closure signature deduction' when passing `wrap_pyfunction!()` as an argument to a function: Rustc will deduce the signature of the closure from the function that closure is passed to. This way, the generic arguments can be deduced in more contexts, fixing the problem.

davidhewitt reviewed Sep 2, 2020

View reviewed changes

sebpuetz force-pushed the pyfunction-modules branch 2 times, most recently from e296c2b to e34294a Compare September 3, 2020 11:49

davidhewitt reviewed Sep 3, 2020

View reviewed changes

sebpuetz force-pushed the pyfunction-modules branch from e34294a to 4e3a35f Compare September 3, 2020 12:20

sebpuetz force-pushed the pyfunction-modules branch from 4e3a35f to ccfb358 Compare September 3, 2020 12:43

sebpuetz force-pushed the pyfunction-modules branch from ccfb358 to 6e17a05 Compare September 3, 2020 12:45

kngwyu approved these changes Sep 3, 2020

View reviewed changes

sebpuetz added 2 commits September 3, 2020 14:49

Move py fn wrapper argument expansion to associated function.

1f017b6

Suggestion by @kngwyu. Additionally replace some `expect` calls with error handling.

Make python function wrapper creation fallible.

3214249

Wrapping a function can fail if we can't get the module name. Based on suggestion by @kngwyu

sebpuetz force-pushed the pyfunction-modules branch from 6e17a05 to 3214249 Compare September 3, 2020 13:49

sebpuetz changed the title ~~WIP: PyModule in #[pyfunction]~~ PyModule in #[pyfunction] Sep 3, 2020

davidhewitt reviewed Sep 3, 2020

View reviewed changes

src/types/module.rs Outdated Show resolved Hide resolved

src/types/module.rs Outdated Show resolved Hide resolved

src/types/module.rs Outdated Show resolved Hide resolved

sebpuetz added 2 commits September 4, 2020 09:01

Possible to pass PyModule as first arg.

795c054

This commit makes it possible to access the module of a function by passing the `need_module` argument to the pyfn and pyfunction macros.

Rename add_module to add_submodule, documentation fixes.

4aae523

sebpuetz force-pushed the pyfunction-modules branch from 6f96ae8 to 4aae523 Compare September 4, 2020 07:03

Add documentation for accessing PyModule in #[pyfunction]s.

9137855

davidhewitt approved these changes Sep 5, 2020

View reviewed changes

sebpuetz force-pushed the pyfunction-modules branch from de0d708 to 410dfc0 Compare September 5, 2020 08:08

Doc fixes, changelog and rename.

e65b849

sebpuetz force-pushed the pyfunction-modules branch from 410dfc0 to e65b849 Compare September 5, 2020 08:21

sebpuetz mentioned this pull request Sep 5, 2020

Segfault when adding a module to itself #1149

Closed

birkenfeld reviewed Sep 5, 2020

View reviewed changes

Fix some more docs.

06cd7c7

sebpuetz force-pushed the pyfunction-modules branch from 25b105f to 06cd7c7 Compare September 5, 2020 09:24

Change add_submodule() to take &PyModule.

64b06ea

The C-exported wrapper generated through `#[pymodule]` is only required for the top-level module.

sebpuetz mentioned this pull request Sep 6, 2020

Introduce a PyFunction? #1156

Closed

davidhewitt merged commit 16fe583 into PyO3:master Sep 6, 2020

sebpuetz mentioned this pull request Sep 8, 2020

Add native Function types. #1163

Merged

m-ou-se mentioned this pull request May 6, 2021

Expand wrap_pyfunction!() to a closure to improve deduction. #1597

Merged

m-ou-se mentioned this pull request May 6, 2021

Upgrade to newer PyO3. fusion-engineering/inline-python#41

Closed

	fn #function_wrapper_ident(py: pyo3::Python #maybe_module_arg) -> pyo3::PyObject {
	fn #function_wrapper_ident(args: impl pyo3::derive_utils::WrapPyFunctionArguments) -> pyo3::PyObject {


		### Accessing the module of a function

		Functions are usually associated with modules, in the C-API, the self parameter in a function call corresponds

		@@ -194,11 +195,50 @@ impl PyModule {
		/// ```rust,ignore
		/// m.add("also_double", wrap_pyfunction!(double)(py));

PyModule in #[pyfunction] #1143

PyModule in #[pyfunction] #1143

Conversation

sebpuetz commented Sep 2, 2020 • edited by davidhewitt

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt Sep 2, 2020 • edited

Choose a reason for hiding this comment

sebpuetz commented Sep 2, 2020

davidhewitt commented Sep 2, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

sebpuetz commented Sep 3, 2020

davidhewitt Sep 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebpuetz commented Sep 3, 2020

kngwyu commented Sep 3, 2020

sebpuetz commented Sep 3, 2020

kngwyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebpuetz commented Sep 3, 2020

sebpuetz commented Sep 3, 2020 • edited

kngwyu commented Sep 3, 2020

sebpuetz commented Sep 3, 2020

sebpuetz commented Sep 3, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented Sep 5, 2020

sebpuetz commented Sep 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt Sep 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebpuetz commented Sep 5, 2020

davidhewitt commented Sep 5, 2020

kngwyu commented Sep 6, 2020

davidhewitt commented Sep 6, 2020

davidhewitt commented Sep 6, 2020

davidhewitt commented Sep 6, 2020

sebpuetz commented Sep 2, 2020 •

edited by davidhewitt

davidhewitt Sep 2, 2020 •

edited

davidhewitt Sep 3, 2020 •

edited

sebpuetz commented Sep 3, 2020 •

edited

davidhewitt Sep 5, 2020 •

edited