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

PyWeak<T>: a weak Py<T> reference which integrates with the weakref Python module #3134

Closed
SuperJappie08 opened this issue May 4, 2023 · 10 comments

Comments

@SuperJappie08
Copy link
Contributor

Hi,

I'm working on python and rust library (Rust library & Rust library wrapped to a Python module) which uses a bidirectional unbalanced tree structure, where the nodes NodeA and NodeB alternate.

The trees also have a HashMap index per node type for easy look up, which contains Weak<NodeX>
Since I have multiple tree types with the same base functionality, the tree is actually stored in a private encapsulated TreeData struct which contains the root node and the index maps. As a result, the parent of the root node is such a Weak<TreeData>.

NodeA is the only node type which can be the root, they can have 2 parent types NodeB or TreeData.

These nodes contain a method to get a reference to their parent in the rust library.
However, for the Python module I would like to return a reference to the encapsulated TreeData so TreeX which is wrapped to PyTreeX.

These returned objects should not be clones due to the referencing nature of the data structure, and introducing a rust Weak<TreeX> in the wrapping makes error handling more complex.

Therefore, I wanted to use the weakref python module to encapsulate a weakref.proxy of the external python tree into the retrieved nodes to return instead of the actual raw TreeData reference.

There is currently no 'nice' rust centric way to do this with PyO3.
In my current solution, I import the weakref module and store a PyObject which contains the weakref.proxy
Here is a code example:

#[derive(Debug, Clone)]
#[pyclass(name = "TreeX", weakref)]
struct PyTreeX {
   inner: TreeX,
   /// Python weakref to self
   me: PyObject,
}

impl PyTreeX {
   /// This function is used for creating a `PyTreeX`
    pub(crate) fn create(tree: TreeX) -> PyResult<Py<PyTreeX>> {
         Python::with_gil(|py| {
              let weakref = py.import(intern!(py, "weakref")).unwrap();
              let tree: Py<PyTreeX> = PyTreeX{
                        inner: tree,
                        me: py.None(),
                   }
                   .into_py(py)
                   .extract(py)?;
              
              weakref
                   .getattr("proxy")?
                   .call1((&tree,))?
                   .to_object(py)
                   .clone_into(&mut tree.borrow_mut(py).me);
                   
              Ok(tree)
              })
    }
}
/* Some other stuff */
#[derive(Debug)]
#[pyclass(name = "NodeA", weakref, frozen)]
pub struct PyNodeA {
   inner: Weak<RwLock<NodeA>>,
   /// Python weakref to the python parent tree
   tree: PyObject,
}

#[pymethods]
impl NodeA {
   #[getter]
   fn get_parent(slf: PyRef<'_, Self>) -> PyResult<Py<PyAny>> {
       match slf.try_internal()?.read().unwrap().parent() {
           NodeAParent::Tree(_) => Ok(slf.tree.clone()),
           NodeAParent::NodeB(node) => Ok(Into::<PyNodeB>::into((
               Weak::upgrade(node).unwrap(),
               slf.tree.clone(),
           ))
           .into_py(slf.py())),
       }
   }
}

impl From<(Weak<RwLock<NodA>>, PyObject)> for PyNodeA {
   fn from(value: (Weak<RwLock<NodeA>>, PyObject)) -> Self {
   	Self {
   		inner: value.0,
   		tree: value.1,
   	}
   }
}

So, I just import the weakref module and use it that way.
This works, but is not as clear in Rust as it can be, since (to my current knowledge) I have to use the PyObject type.

Proposal

Therefore, I propose a new python reference type related to Py<T> which would incorporate some of the weakref functionality.
This could look something like PyWeak<T> and corresponds with a python weakref.ref(T).
This reference type could introduce a method which could look more like rust's Weak::upgrade() or python's way of working with weakref.ref().

Such a upgrade/get version could be implemented in 2 ways.

  • PyWeak::ref(...) -> PyResult<T> where the same Python exception is raised as if you are using weakref.proxy (or ReferenceError for Python >=3.10 or AtributeError for earlier version)
  • PyWeak::get() -> Option<T> which behaves just like python's weakref.ref which needs to be called for it to return the object or None if it has been deallocated.
  • PyWeak::upgrade(...) -> Option<Py<T>>, this would be like rust's Rc::upgrade(...) and Arc::upgrade(...)

Something that would be really nice, which, I doubt, is possible, is something like rust's Arc::new_cyclic() to initialize cyclic references.

The biggest advantage over the way this currently can be achieved, is the improved type safety in Rust and possibly some speed up due to not having to import weakref in the Rust code.

Syntax example

The 'upgraded' version, of the previous example:

#[derive(Debug, Clone)]
#[pyclass(name = "TreeX", weakref)]
struct PyTreeX {
    inner: TreeX,
    /// Python weakref to self
    me: PyWeak<PyTreeX>,
}

impl PyTreeX {
     pub(crate) fn create(tree: TreeX) -> PyResult<Py<PyTreeX>> {
          Python::with_gil(|py| {
               let tree: Py<PyTreeX> = PyWeak::new_cyclic( |me| PyTreeX{
                         inner: tree,
                         me: me,
                    })?;
               
               Ok(tree)
               })
     }
}
/* Some other stuff */
#[derive(Debug)]
#[pyclass(name = "NodeA", weakref, frozen)]
pub struct PyNodeA {
    inner: Weak<RwLock<NodeA>>,
    /// Python weakref to the python parent tree
    tree: PyWeak<PyTreeX>,
}

#[pymethods]
impl NodeA {
    #[getter]                                             // The return type stil would have to be `Py<PyAny>`
    fn get_parent(slf: PyRef<'_, Self>) -> PyResult<Py<PyAny>> {
        match slf.try_internal()?.read().unwrap().parent() {
            NodeAParent::Tree(_) => Ok(slf.tree.clone()),
            NodeAParent::NodeB(node) => Ok(Into::<PyNodeB>::into((
                Weak::upgrade(node).unwrap(),
                slf.tree.clone(),
            ))
            .into_py(slf.py())),
        }
    }
}

impl From<(Weak<RwLock<NodA>>, PyWeak<PyTreeX>)> for PyNodeA {
	fn from(value: (Weak<RwLock<NodeA>>, PyWeak<PyTreeX>)) -> Self {
		Self {
			inner: value.0,
			tree: value.1,
		}
	}
}

Feasibility

While looking more into the weakref modules, I noticed that there are C bindings available (in both Python's C-API and PyO3-ffi).

I'm not 100% sure if something like this is even possible.

I would love to help implement. However, my rust experience is very limited in this sector.
And I also need to finish the project which raised the question in the next month(s), so I am a bit preoccupied.

I wonder if something like this has been considered already, or if there already exists a 'better' way to do such things.

@SuperJappie08
Copy link
Contributor Author

I started working on this. It currently only does weakref.ref.
And would it be better to be a separate module/file, since I currently put in src/instance.rs (Same place as Py<T>)?

@adamreichold
Copy link
Member

And would it be better to be a separate module/file, since I currently put in src/instance.rs (Same place as Py)?

I'd say that depends on whether it is tied to the internals of Py. If not, a separate module would probably be better as it is easier to read and understood a smaller isolated piece of code.

@davidhewitt
Copy link
Member

Please note that #3205 may have impacts on the design of Py, which may affect what we want to do here too, but it's too early to know if we want to commit to that direction.

@SuperJappie08
Copy link
Contributor Author

I've made a basic working version.
Currently, PyWeak is a modified copy of Py and it only supports the basic Python Ref type.
Some parts of the documentation still need to be improved.

However, I still have some design questions/problems:

  • a PyWeak::new_cyclic is difficulty to do 'nicely' (especially for abi3), since a non-initialized weakref object can't be created. There might still be 2 ways to achieve this:
    • The Hacky Way: A python Weakref is created on a different object, that normally will not be referenced, and the internal pointers are changed. This would only work for Non ABI targets and is difficult to do correctly in edge cases.
    • Alternative Way: An new reftype inherited from Ref is created and used instead,
  • Currently only the basic reftype can be used, Not Proxy and there is no way to distinguish between then currently (Phantom Data?)
  • Theoretically a weakref is just a Python object, so it could also be implemented more like PyList, which maybe has its benefits. (These implementations could coexist)

What do you think?

@davidhewitt
Copy link
Member

I agree with your last suggestion of having coexisting implementations. A python weakref.ref is its own object which contains a pointer to the inner target, so my gut tells me your suggestion of implementing in the same way as PyList (and PyAny) makes sense.

Then we could have (with the current API) &PyWeakRef, &PyWeakProxy, Py<PyWeakRef> and Py<PyWeakProxy>, perhaps?

These untyped versions are not as elegant for you as PyWeak<T>, but it's pretty clear how they would function. I think also there would be no way to statically guarantee that a weakref.ref contained an element of type T, so it would not be safe to implement FromPyObject for PyWeak<T>.

Building an implementation of PyWeak<T> on top of the dynamic abstractions would presumably be straightforward.

For new_cyclic ... weakref.proxy is not subclassable so I think that option is off the table for a hypothetical PyWeakProxy<T>. I haven't managed to think of a way to do this which is elegant (or even safe).

@SuperJappie08
Copy link
Contributor Author

SuperJappie08 commented Aug 18, 2023

A way to allow for PyWeak<T> to be typed (3rd section), might be able to be achieved by creating are own PyO3Weak which needs to be statically typed, this can be done since it is possible to inherit from Ref.

This might also allow for cyclic references.

I will start to add the PyAny like types to my fork/implementation soonish at some point.

@SuperJappie08
Copy link
Contributor Author

I (finally) had some spare time to add some of the features discussed.

Currently, PyWeakRef (weakref.ReferenceType), PyWeakProxy (weakref.ProxyType) and PyWeakCallableProxy (weakref.CallableProxyType) are implemented similar to PyList in the new Bound API.

I might look into creating a PyWeakProxyGeneric for combining PyWeakProxy and PyWeakCallableProxy, since they get created by the same function (C PyWeakref_NewProxy can return callable and non-callable proxy variant based on the supplied input).
Currently, the code panics, when a non-callable is past to the callable proxy and vice versa.

Shall I already make a pull-request for this?

@davidhewitt
Copy link
Member

@SuperJappie08 I think we can definitely review a PR for these types. Those new types being added to the Bound API seems pretty clear-cut OK to include 👍

The PyWeakProxyGeneric - I'd need to see the code to follow the design a bit clearer & understand the use case!

@ngoldbaum
Copy link
Contributor

I think this can be closed because #3835 added the wrappers.

That said, there's an issue with the API that was added that I think we should probably fix in the long term.

Python 3.13 adds PyWeakref_GetRef and deprecates PyWeakref_GetObject, to be removed in Python 3.15. This function fixes a number of issues with PyWeakref_GetObject - in particular it adds NULL checking and explicitly checks for weakref objects, so there's no need to ever panic.

I think we can probably add a new API to replace get_object and get_object_borrowed named something like get_ref that returns a PyResult<Option<Bound<'py, PyAny>>>. I think that would also avoid the possibility of panics.

@davidhewitt
Copy link
Member

Let's close this for now, can always revisit further later.

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

4 participants