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 in #[pyfunction] #1143

Merged
merged 10 commits into from
Sep 6, 2020
Merged

Conversation

sebpuetz
Copy link
Contributor

@sebpuetz sebpuetz commented Sep 2, 2020

Closes #828

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

@davidhewitt davidhewitt Sep 2, 2020

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

sebpuetz commented Sep 2, 2020

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
Copy link
Member

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 =)

@sebpuetz sebpuetz force-pushed the pyfunction-modules branch 2 times, most recently from e296c2b to e34294a Compare September 3, 2020 11:49
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 like my suggestion worked!

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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.


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

@davidhewitt davidhewitt Sep 3, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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

@kngwyu
Copy link
Member

kngwyu commented Sep 3, 2020

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks, nicely designed 👍

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

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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
Copy link
Member

kngwyu commented Sep 3, 2020

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.

Suggestion by @kngwyu.

Additionally replace some `expect` calls with error handling.
@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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.

Wrapping a function can fail if we can't get the module name.

Based on suggestion by @kngwyu
@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 3, 2020

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.

@sebpuetz sebpuetz changed the title WIP: PyModule in #[pyfunction] PyModule in #[pyfunction] Sep 3, 2020
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
This commit makes it possible to access the module of a function
by passing the `need_module` argument to the pyfn and pyfunction
macros.
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, this looks great to me🚀

Great docs and tests much appreciated 😊

Couple of final suggestions from me.

use pyo3::wrap_pyfunction;
use pyo3::prelude::*;

#[pyfunction(need_module)]
Copy link
Member

Choose a reason for hiding this comment

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

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


### Accessing the module of a function

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

Choose a reason for hiding this comment

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

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.

# fn main() {}
```

If `need_module` is set, the first argument **must** be the `&PyModule`. It is then possible to interact with
Copy link
Member

Choose a reason for hiding this comment

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

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

# fn main() {}
```

Within Python, the name of the module that a function belongs to can be accessed through the `__module__`
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Oh also, CHANGELOG entry please! 😄

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 5, 2020

Looks pretty green to me ;)

guide/src/function.md Outdated Show resolved Hide resolved
/// 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));
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to handle the Result from wrap_pyfunction?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

src/types/module.rs Outdated Show resolved Hide resolved
tests/test_module.rs Show resolved Hide resolved
@@ -194,11 +195,50 @@ impl PyModule {
/// ```rust,ignore
/// m.add("also_double", wrap_pyfunction!(double)(py));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@davidhewitt davidhewitt Sep 5, 2020

Choose a reason for hiding this comment

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

(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).

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks!

The C-exported wrapper generated through `#[pymodule]` is only
required for the top-level module.
@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 5, 2020

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

@davidhewitt
Copy link
Member

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
Copy link
Member

kngwyu commented Sep 6, 2020

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

LGTM, let's merge this after David reviews.

@davidhewitt
Copy link
Member

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

@sebpuetz sebpuetz mentioned this pull request Sep 6, 2020
@davidhewitt
Copy link
Member

Yep looks great, let's merge this! 🎉

@davidhewitt
Copy link
Member

Thank you for your hard work on this!

@davidhewitt davidhewitt merged commit 16fe583 into PyO3:master Sep 6, 2020
m-ou-se added a commit to m-ou-se/pyo3 that referenced this pull request May 6, 2021
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.
m-ou-se added a commit to m-ou-se/pyo3 that referenced this pull request May 6, 2021
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.
m-ou-se added a commit to m-ou-se/pyo3 that referenced this pull request May 6, 2021
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.
m-ou-se added a commit to m-ou-se/pyo3 that referenced this pull request May 6, 2021
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.
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.

Accessing the module object in pyfunctions
4 participants