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

Trait Bound Documentation without GILPool #4474

Closed
kiranshila opened this issue Aug 23, 2024 · 6 comments
Closed

Trait Bound Documentation without GILPool #4474

kiranshila opened this issue Aug 23, 2024 · 6 comments

Comments

@kiranshila
Copy link

I'm attempting to implement a Python interface to some Rust hardware driver code that was written against Rust hardware traits. For the python interface, the implementation of this trait needs to be via calls to Python - but the logic of the driver is, of course, written in Rust. This results in a lot of ping-ponging between Rust and Python, something that seems really hard to get right.

In the docs, there is this page on trait bounds, but it uses the Python::with_gil/GILPool API. I'm still not quite sure how that interacts with the new Bound API - but my issue/question is on how these docs change if we weren't to use the GILPool. i.e, we need to get a Bound into the trait.

I have some code that looks similar to the docs example, but with the instance of the PyObject for which we want to implement a Rust trait for inside another Rust struct that is generic on T which requires the implementation of that trait.

// ------ Foreign Rust Library Code

trait ForeignTrait {
    fn trait_method(&mut self) -> f32;
}

struct ForeignType<T: ForeignTrait> {
    inner: T,
}

impl<T> ForeignType<T>
where
    T: ForeignTrait,
{
    fn do_something(&mut self) -> f32 {
        let x = self.inner.trait_method();
        x * x
    }
}

// ----- My Python Wrapper Code

/// A newtype around an instance of a python class that has a method I want to use from rust
struct PyWrapper(Py<PyAny>);

impl ForeignTrait for PyWrapper {
    fn trait_method(&mut self) -> f32 {
        Python::with_gil(|py| {
            let obj = self.0.bind(py);
            let x: f32 = obj
                .call_method0("fn_that_returns_a_float")
                .unwrap() // We'll do real error handling at some point
                .extract()
                .unwrap();
            x
        })
    }
}

/// A "top-level" struct that I'll expose to Python
/// Basically just a surrogate for `ForeignType` as an FFI-shim
#[pyclass]
struct TopLevel(ForeignType<PyWrapper>);

#[pymethods]
impl TopLevel {
    #[new]
    fn new(obj_from_python: Bound<'_, PyAny>) -> Self {
        Self(ForeignType { inner: PyWrapper(obj_from_python.unbind()) })
    }

    /// A shim for the inner function of ForeignType
    fn do_something(&mut self) -> f32 {
        self.0.do_something()
    }
}

In the real code I've written, I occasionally get RuntimeError: already borrowed - which means something about the setup here breaks some rules. I'm wondering what assumptions in this example are not correct such that I could get that runtime error, if this is related to some weird interaction between the Bound and Python::with_gil trait impl, what kind of solutions there are, and if we could work together on some docs to explain this.

Cheers!

@ChayimFriedman2
Copy link
Contributor

You will get a panic If fn_that_returns_a_float() will call TopLevel::do_something(). The way to fix that is by using Bound<Self> instead of &mut self:

/// A newtype around an instance of a python class that has a method I want to use from rust
struct PyWrapper(Py<PyAny>);

impl ForeignTrait for PyWrapper {
    fn trait_method(&mut self) -> f32 {
        Python::with_gil(|py| {
            let obj = self.0.bind(py);
            let x: f32 = obj
                .call_method0("fn_that_returns_a_float")
                .unwrap() // We'll do real error handling at some point
                .extract()
                .unwrap();
            x
        })
    }
}

/// A "top-level" struct that I'll expose to Python
/// Basically just a surrogate for `ForeignType` as an FFI-shim
#[pyclass]
struct TopLevel(Py<PyAny>);

#[pymethods]
impl TopLevel {
    #[new]
    fn new(obj_from_python: Bound<'_, PyAny>) -> Self {
        Self(obj_from_python.unbind())
    }

    /// A shim for the inner function of ForeignType
    fn do_something(this: Bound<'_, Self>) -> f32 {
        let wrapper = PyWrapper(this.borrow().0.0.clone_ref(this.py()));
        ForeignType { inner: wrapper }.do_something();
    }
}

@kiranshila
Copy link
Author

Thank you for your response! So, this implies that we have to construct a ForeignType on every call to an internal function? How would we manage a situation where that struct contains state?

@ChayimFriedman2
Copy link
Contributor

You can store a ForeignType and clone it instead. Or, if that's not possible, take it and return it back at the end, but that will mean users won't be able to call your method recursively (but they still won't get PanicException).

@kiranshila
Copy link
Author

I think I don't completely understand.

If I need to work through a PyRef how about

#[pyclass]
struct TopLevel(ForeignType<PyWrapper>);

#[pymethods]
impl TopLevel {
    #[new]
    fn new(obj_from_python: Bound<'_, PyAny>) -> Self {
        Self(obj_from_python.unbind())
    }

    /// A shim for the inner function of ForeignType
    fn do_something(this: Bound<'_, Self>) -> f32 {
        this.borrow_mut().0.do_something()
    }
}

@ChayimFriedman2
Copy link
Contributor

I think I don't completely understand.

If I need to work through a PyRef how about

#[pyclass]
struct TopLevel(ForeignType<PyWrapper>);

#[pymethods]
impl TopLevel {
    #[new]
    fn new(obj_from_python: Bound<'_, PyAny>) -> Self {
        Self(obj_from_python.unbind())
    }

    /// A shim for the inner function of ForeignType
    fn do_something(this: Bound<'_, Self>) -> f32 {
        this.borrow_mut().0.do_something()
    }
}

This won't work. PyRef alone isn't enough, you need to not hold the borrow when you call arbitrary Python code.

@davidhewitt
Copy link
Member

As discussed above in the thread, the docs are already updated for Bound and the issue was more to do with interior mutability.

Will close here as no further action.

@davidhewitt davidhewitt closed this as not planned Won't fix, can't repro, duplicate, stale Oct 22, 2024
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