Skip to content

Conversation

@lmmx
Copy link

@lmmx lmmx commented Dec 11, 2025

Implements PyCapsule::new_pointer as described in the issue above

Purpose

Allows users to write much simpler code, without getting under the hood of pyo3-ffi to make a pyo3::PyCapsule

#[pyfunction(name = "rms_norm")] fn rms_norm_jax(py: Python<'_>) -> PyResult<Bound<'_, PyCapsule>> { unsafe { PyCapsule::new_pointer(py, ffi::RmsNorm as *mut c_void, None) } }

instead of

#[pyfunction(name = "rms_norm")] fn rms_norm_jax(py: Python<'_>) -> PyResult<Bound<'_, PyCapsule>> { let fn_ptr: *mut c_void = ffi::RmsNorm as *mut c_void; let name = std::ptr::null(); unsafe { let capsule = pyo3::ffi::PyCapsule_New(fn_ptr, name, None); if capsule.is_null() { return Err(pyo3::exceptions::PyRuntimeError::new_err( "Failed to create PyCapsule", )); } let any: Bound<'_, PyAny> = Bound::from_owned_ptr(py, capsule); Ok(any.cast_into_unchecked::<PyCapsule>()) } }
@lmmx lmmx force-pushed the raw-pointer-capsule branch from be6bd40 to 26e4220 Compare December 11, 2025 23:54
///
/// # Safety
///
/// - `pointer` must be non-null and valid for the intended use case.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to say that the pointer must be non-null, as this is checked internally by the function and will raise an error if the pointer NULL.

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 for working on this, I think the proposed functionality makes sense. Some initial comments on the design below.

/// ```
pub unsafe fn new_pointer(
py: Python<'_>,
pointer: *mut c_void,
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 using NonNull<c_void> here to make it clear in the API contract that the pointer must be non-null? Can then avoid checking for this in the code below, and defer to the caller to decide how they construct the NonNull.

Comment on lines +241 to +247
// We need to keep the name alive if it was provided, but since we're not
// using PyO3's destructor mechanism, we need to leak it or store it.
// For simplicity, we leak the name - this matches Python's expectation
// that capsule names are static or long-lived.
if let Some(name) = name {
std::mem::forget(name);
}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest rather than leaking we make the name argument Option<&'static CStr>, given this is now trivial to construct.

I might even go further and recommend we always name capsules (this seems to be good practice), i.e. we could perhaps just have name: &'static CStr?

py: Python<'_>,
pointer: *mut c_void,
name: Option<CString>,
destructor: Option<ffi::PyCapsule_Destructor>,
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to users to document that the destructor must be a raw function unlike the new_with_destructor case because there is nowhere to store a destructor closure in this form.

/// assert_eq!(ptr.as_ptr(), my_handler as *mut c_void);
/// });
/// ```
pub unsafe fn new_pointer(
Copy link
Member

Choose a reason for hiding this comment

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

Name bikeshedding, what do you think of new_with_pointer and new_with_pointer_and_destructor?

I somehow feel like new_pointer is describing that a "capsule pointer" gets returned rather than the capsule is constructed with a raw pointer.

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

Labels

None yet

3 participants