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

Introduce a PyFunction? #1156

Closed
sebpuetz opened this issue Sep 6, 2020 · 12 comments
Closed

Introduce a PyFunction? #1156

sebpuetz opened this issue Sep 6, 2020 · 12 comments

Comments

@sebpuetz
Copy link
Contributor

sebpuetz commented Sep 6, 2020

Inspired by the discussion in #1149 I thought it might be nice to add a native PyFunction in Rust. It seems like functions come in two flavours in CPython:

PyCFunction (defined in https://github.com/python/cpython/blob/master/Objects/methodobject.c already bound in ffi::methodobject.rs, type object PyCFunction_Type) and PyFunction (defined in https://github.com/python/cpython/blob/3.7/Objects/funcobject.c afaic unbound in pyo3) which are distinct types. PyCFunction seems to relate to built-ins and methods defined in extension modules while PyFunction is a regularly def'd Python function.

I'm not sure how we could bind PyFunction in Rust, but e.g. this would allow extracting built-in / extension functions in Rust:

#[repr(transparent)]
pub struct PyFunction(PyAny);

pyobject_native_var_type!(PyFunction, ffi::PyCFunction_Type, ffi::PyCFunction_Check);
[...]

#[pyfunction]
fn some_fun(built_in: &PyFunction) {
    // call the function, add function to module, whatever you want to do with it
}

and we would be able to change PyModule::add_function to take Fn(python: &PyModule) -> PyResult<&PyFunction> instead of the generic -> PyResult<PyObject> since we'd have a proper Rust type for functions.


I have also been looking for ways to construct such PyFunctions in Rust at runtime but didn't get too far:

pub fn new<'p, F>(module: &'p PyModule, name: &'static str, fun: F) -> PyResult<&'p PyFunction>
    where F: Fn(&PyModule, &PyTuple, &PyDict) -> PyResult<PyObject>

The issue is that we (as far as I can tell) need an extern "C" function that we put in the MethodDef, if we define this wrapper as a local method inside the fn new, we can't access it since local functions don't capture their context. To call the proper implementation, we'd need to get the correct function identifier into the method body at compile time, which is how we currently do it through proc-macros.

A (probably) possible way is to pass the function wrapper instead of building it locally as ffi::PyCFunctionWithKeywords which would mean we'd have to offer an API / macro to produce such functions. As of now we're producing these wrappers inside of __pyo3_get_function_X, so they're hidden from the user.

Then there's also the construction that we already offer: wrap_pyfunction!(function_name)(module) which returns a PyObject thats actually a &PyFunction.

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 6, 2020

I pushed a branch on my fork that implements the first half of what I discsussed above: sebpuetz@fd37b21

#[pyfunction] generated getters return &PyFunctions and add_function expects this as the return type.

@davidhewitt
Copy link
Member

Interesting idea! I haven't thought too hard this morning, so apologies if any of the below is stupid / obvious...

PyCFunction seems to relate to built-ins and methods defined in extension modules while PyFunction is a regularly def'd Python function.

Would we actually need want two types? PyCFunction and PyFunction? If I understand correctly, wrap_pyfunction!(function_name)(module) creates a PyCFunction object?

Also worth considering is #684 - last time I looked at that I think I concluded we might want to consider making our own function type. (But I can't remember clearly why.)

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 6, 2020

I think we'd need two function types if we'd want to be able to take (and construct) regular Python functions. From my understanding they're just (rightfully so) different types, a PyCFunction doesn't need its code, just a function pointer, while a PyFunction very much needs its code.

Would we actually need want two types? PyCFunction and PyFunction? If I understand correctly, wrap_pyfunction!(function_name)(module) creates a PyCFunction object?

wrap_pyfunction! just changes the Rust function identifier to the proc-macro-generated function's identifier which returns a PyCFunction object.

Although we could provide preliminary PyFunction and PyCFunction where PyFunction can only be passed from Python to Rust but not defined within Rust. If we figure out a nice way to construct PyCFunction in Rust then we can expose that

@kngwyu
Copy link
Member

kngwyu commented Sep 6, 2020

I have also been looking for ways to construct such PyFunctions in Rust at runtime but didn't get too far:

Via PyCFunction_New?
Is it possible?
And what is it for?

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 6, 2020

I have also been looking for ways to construct such PyFunctions in Rust at runtime but didn't get too far:

Via PyCFunction_New?

Is it possible?

And what is it for?

The idea is to construct a PyFunction the same way we currently offer a way to construct PyModules.

This is currently achieved with the wrap_pyfunction!(ident)(module), but the return type is underspecified as PyObject. The linked commit above shows how this could look like if we add a PyFunction as a native type.

@kngwyu
Copy link
Member

kngwyu commented Sep 6, 2020

Yeah, but I think we can do that by casting PyObject to PyCFunction.

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 6, 2020

I think we need to define some words to figure this out: there's the PyCFunction alias in pyo3 that describes the function type that's being called by Python.

Then there's PyCFunction which describes the Python object that's used to call the actual C function. I'm interested in exposing the latter as a native type which is currently not part of the API. With the current implementations there is no (Rust) type that we can cast the PyObject to. That's why I linked the commit on my fork which adds such a type and also makes the change to the proc macro and the add_function wrapper argument.

@kngwyu
Copy link
Member

kngwyu commented Sep 6, 2020

I'm sorry for the confusion and misunderstanding: I agree with adding PyCFunction wrapper.
I was just doubtful about the definition of the constructor you wrote. It looks like trying to wrap some closures, which I think is impossible.
Looks you already pointed out it has to be a more limited one (e.g., PyCFunction::new(name: &str, meth: fn() -> ...) -> &Self) and it would work fine.

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 6, 2020

Once #1143 goes in, I'll also open a PR to get a basic PyCFunction wrapper in. I think it'd be something that would complement the other changes nicely since we'd get proper typing on both the add_X functions for PyModule. Fleshing out the Rust API for the type could happen later on.

@davidhewitt
Copy link
Member

I think the Rust API for PyCFunction::new might be a bit fiddly to set up, so I agree we can land maybe just the wrapper in 0.12 for add_submodule, and then can figure out the rest of Rust API for it as we work towards 0.13.

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Sep 7, 2020

Yep, that was also my plan. I'll probably get around to open a PR later today. I'll also look into the non-extension PyFunctions, I think we'd want PyModule::add_function to abstract over PyFunction and PyCFunction. So native PyFunction and PyCFunction types with an additionally some wrapper enum which implements From for both could come in nicely.

If that's not straight forward to implement, we might consider restricting to PyCFunction for now or even leave it unchanged. Although, we'd end up with a somewhat inconsistent API.

@sebpuetz
Copy link
Contributor Author

#1163, I'll open a new issue to track building an API for the function types...

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

No branches or pull requests

3 participants