WIP [internal] Allow going from Rust-CPython pointers to PyO3 pyclass structs

[Eric-Arellano]

This will allow us to use the migration strategy suggested by @stuhood at https://github.com/pantsbuild/pants/pull/12437/files#r678512355: we can move the definition of types like AddPrefix from Python into the PyO3 native extension, then have the rust-cpython engine/ crate parse cpython::PyObjects into values like our custom PyAddPrefix and PyPathGlobs.

The benefits are that we can move our parsing code out of nodes.rs and intrinsics.rs into dedicated FFI files, and "would avoid copying from a Python object into a Rust struct by directly creating the relevant types in the constructor."

[Eric-Arellano]

This fails with a lifetime issue I'm struggling to grok. Anything obvious stick out to you @stuhood, @mejrs, @davidhewitt ?

❯ ./cargo check
    Checking engine v0.0.1 (/Users/ericarellano/code/pants/src/rust/engine)
error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'p` due to conflicting requirements
   --> src/externs/mod.rs:427:5
    |
427 |     pyo3::PyAny::from_owned_ptr(py, value.as_ptr() as *mut pyo3::ffi::PyObject)
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 426:41...
   --> src/externs/mod.rs:426:41
    |
426 |     let pyo3_val = pyo3::Python::with_gil(|py| unsafe {
    |  _________________________________________^
427 | |     pyo3::PyAny::from_owned_ptr(py, value.as_ptr() as *mut pyo3::ffi::PyObject)
428 | |   });
    | |___^
note: ...so that the expression is assignable
   --> src/externs/mod.rs:427:33
    |
427 |     pyo3::PyAny::from_owned_ptr(py, value.as_ptr() as *mut pyo3::ffi::PyObject)
    |                                 ^^
    = note: expected `pyo3::Python<'_>`
               found `pyo3::Python<'_>`
note: but, the lifetime must be valid for the anonymous lifetime defined on the function body at 425:48...
   --> src/externs/mod.rs:425:48
    |
425 | pub(crate) fn from_rust_cpython_to_pyo3(value: &PyObject) -> &pyo3::PyAny {
    |                                                ^^^^^^^^^
note: ...so that reference does not outlive borrowed content
   --> src/externs/mod.rs:431:3
    |
431 |   pyo3_val
    |   ^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0495`.

[Eric-Arellano]

Oh hm..(bare with me, still internalizing Rust). I see now this implementation:

    unsafe fn from_owned_ptr(py: Python<'p>, ptr: *mut ffi::PyObject) -> &'p Self {
        Self::from_owned_ptr_or_panic(py, ptr)
    }

If I read correctly, that means the returned object shares the same lifetime as the Python marker type, meaning the GIL will be held the entire time?

Here, we'd need to take Python as an arg to from_rust_cpython_to_pyo3, so that its lifetime outlives this function?

[mejrs]

The lifetime of the &PyAny is bound to that of the py token which originates inside the function. So you can never return the &PyAny from the function. There are some ways around this though.

But you should probably rethink that function - for example mem::forgetting a reference does nothing, because references don't do anything when dropped. It won't leave the reference count incremented, if that is what you want.

[stuhood]

But you should probably rethink that function - for example mem::forgetting a reference does nothing, because references don't do anything when dropped. It won't leave the reference count incremented, if that is what you want.

Mmm. Can skip the mem::forget by using from_borrowed_ptr instead of from_owned_ptr here.

[stuhood]

As @mejrs said, the &pyo3::PyAny is only valid while the GIL is held: the simplest option would be to return an owned PyAny (or whatever the pyo3 equivalent is) by cloning it.

More complicated would be to continue to return a reference, but require that the GIL is already held before calling the function by accepting the py token as an argument: then the signature might look something like:

pub(crate) fn from_rust_cpython_to_pyo3<'p, 'o>(py: Python<'p>, value: &'o PyObject) -> &'p pyo3::PyAny {
  ..
}

Which of those is preferable depends on whether the GIL is already likely to be held at callsites? But I'm also not familiar enough with pyo3's API to know.

[davidhewitt]

The owned PyAny is currently bundled up in a smart pointer Py<PyAny>, which also doesn't have a lifetime tied to the GIL.

[davidhewitt]

That said, I would probably go with &PyAny and require the GIL to be held at callsites. Python::with_gil had some performance overhead so it seems undesirable to do it unconditionally inside a low level conversion function like this. If you took the GIL token as an argument, you could do a bulk conversion of many objects without having to acquire and release the GIL for each one.

[Eric-Arellano]

Will reopen when I have time to pick this up.

[Eric-Arellano]

I set up a simplified repository at https://github.com/Eric-Arellano/rust-cpython-pyo3 and got the code to compile, but PyO3 gives a TypeError when trying to do this:

    pyo3_py_person = pyo3_extension.PyPerson("some user", 23)
    cpython_greeting = cpython_extension.greet_py_person(pyo3_py_person)
    print(cpython_greeting)

TypeError("'PyPerson' object cannot be converted to 'PyPerson'"

--

I also am seeing weirdness from doing the conversion entirely within PyO3:

fn from_pyo3_pointer_to_pyo3<'py>(value: &PyAny, py: Python<'py>) -> &'py PyAny {
    // NB: Gives TypeError when using `from_borrowed_ptr` instead of `from_owned_ptr`.
    unsafe { pyo3::PyAny::from_owned_ptr(py, value.as_ptr()) }
}

    pyo3_py_person = pyo3_extension.PyPerson("some user", 23)
    pyo3_greeting = pyo3_extension.greet_py_person(pyo3_py_person)
    print(pyo3_greeting)
    print(pyo3_py_person)

Rather than rendering the PyPerson repr, it renders 'write'. This isn't very surprising. I'm using from_owned_ptr, so it makes sense the object becomes invalid after calling the function.

--

This all leads me to wonder if this approach is viable.

[davidhewitt]

From a quick look I see two things wrong with the rust-cpython-pyo3 example repository you link:

• unsafe { pyo3::PyAny::from_owned_ptr(py, value.as_ptr()) } will create a double-free; you should use value.into_ptr() instead or go back to using from_borrowed_ptr as you were before.
• You're defining a #[pyclass] in a lib crate and then using that in two other final extension-module crates. Unfortunately this will create two separate instantiations of that #[pyclass], because of Rust's static linking model. This is why you see the confusing TypeError - there really are two different PyPerson classes created. (This is basically Sharing pyclasses between multiple Rust packages PyO3/pyo3#1444).

[mejrs]

I'm not sure this approach is worthwhile. A gradual transition where you keep everything working at all times sounds like a lot of effort. I would sooner do a big refactor where you change over all at once, fix the errors and sort out any bugs you find. If you're worried, write more tests before you start.

[Eric-Arellano]

Thank you both @mejrs and @davidhewitt! I agree. I changed my focus this weekend to instead refactor our rust-cpython code so a) it's easier to port, and b) I understand it 😄

[stuhood]

@davidhewitt: If PyO3/pyo3#1444 is unavoidable at the moment, is there any way to expose both Py03 types/methods and rust-cpython out of the same module, such that we could build a single .so file containing both libraries? Maybe by getting a pointer to the module from either Py03 or rust-cpython before it is finalized, and then passing it to the other to define more types in?

I'm not sure this approach is worthwhile. A gradual transition where you keep everything working at all times sounds like a lot of effort. I would sooner do a big refactor where you change over all at once, fix the errors and sort out any bugs you find. If you're worried, write more tests before you start.

Both approaches are tons of effort, but if the incremental cost can be lowered sufficiently, it might be worth it at our size. It allows for casual pushing of the problem here and there, and multiple contributors... rather than huge challenging-to-maintain branches.

[Eric-Arellano]

rather than huge challenging-to-maintain branches.

So far, #13526 is going a lot better than expected! It helps that PyO3 and Rust-Cpython share so many APIs. I suspect this port will be much simpler than when you went from CFFI to Rust-Cpython.

Where the port has been causing issues, I'm trying to split out prework refactor PRs to simplify Rust-CPython.

(TBD how painful it will be to port the pyclasses.)

[davidhewitt]

is there any way to expose both Py03 types/methods and rust-cpython out of the same module, such that we could build a single .so file containing both libraries? Maybe by getting a pointer to the module from either Py03 or rust-cpython before it is finalized, and then passing it to the other to define more types in?

This would probably work. Ultimately a PyModule is just a Python module represented by a pointer, even if the Rust types are different.

Note that the traits being incompatible between the two libraries would mean that the two sets of types probably would not be usable in each other's macros.