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

[Question] Return different sub-classes from one function? #1637

Open
azerupi opened this issue May 26, 2021 · 12 comments
Open

[Question] Return different sub-classes from one function? #1637

azerupi opened this issue May 26, 2021 · 12 comments

Comments

@azerupi
Copy link

azerupi commented May 26, 2021

Is it possible to return different sub-classes from a function?

In Rust I have an enum containing data

pub enum Frame {
    Type,
    TypeResponse(u8, Version, u8),
    Ping(u8),
    Pong(u8),
}

In Python I though I would expose this by having a Frame base class and a sub-class for each type of frame:

#[pyclass(name="Frame", subclass)]
pub struct PyFrame { /* some fields */ }

#[pyclass(extends=PyFrame, subclass)]
pub struct PingFrame {
    #[pyo3(get)]
    sequence: u8,
}

#[pymethods]
impl PingFrame {
    #[new]
    pub fn new(sequence: u8) -> (Self, PyFrame) {
        let frame = Frame::new_ping(sequence);
        ((&frame).try_into().unwrap(), frame.into())
    }
}

impl TryFrom<&Frame<'_>> for PingFrame {
    type Error = ();
    
    fn try_from(frame: &Frame) -> Result<Self, Self::Error> {
        match frame.payload() {
            Frame::Ping(sequence) => Ok(PingFrame { sequence: *sequence }),
            _ => Err(())
        }
    }
}

Now I'm trying to have a function that can return any of the sub-classes depending on the frame enum variant. Is that possible?

fn parse(&mut self, py: Python, byte: u8) -> PyResult<Option<PyObject>> {
        match self.parser.parse(byte) {
            Ok(f) => Ok(Some(PingFrame::new(5).into_py(py))),
            Err(e) => return Err(ParseError { err: e }.into())
        }
    }

I tried to use PyFrame and PyObject in the return type, but that doesn't seem to work.

@azerupi azerupi changed the title [Question] Return different sub-classes in function of an enum? [Question] Return different sub-classes from one function? May 26, 2021
@birkenfeld
Copy link
Member

I feel like I've figured out the solution to this once already, but I don't remember where :(

The tuple (Subclass, Baseclass) should be possible to convert to a PyObject but it clearly doesn't right now.

Maybe we should rethink the subclassing strategy and require a field of the subclass to be of type Base?

@mejrs
Copy link
Member

mejrs commented May 26, 2021

The guide has a paragraph on inheritance with an example that looks like what you are trying to do.

But this seems like a weird thing to use inheritance for (do you happen to use inheritance for other reasons?). If you just want to return the enum to Python code, you should probably implement IntoPy for the enum.

Something like this:

use pyo3::prelude::*;

#[pyclass]
struct A{}

#[pyclass]
struct B{}

enum Thing {
    A,
    B,
    None
}

impl IntoPy<PyObject> for Thing {
    fn into_py(self, py: Python) -> PyObject {
        match self {
            Self::A => A{}.into_py(py),
            Self::B => B{}.into_py(py),
            Self::None => py.None()
        }
   }
}

@azerupi
Copy link
Author

azerupi commented May 26, 2021

The guide has a paragraph on inheritance with an example that looks like what you are trying to do.

I'm probably looking right past it, but I can't see where in the examples there is a function that returns different sub-classes. 😕

But this seems like a weird thing to use inheritance for (do you happen to use inheritance for other reasons?). If you just want to return the enum to Python code, you should probably implement IntoPy for the enum.

At first I wanted to return a struct that contained that enum. However that enum has a lifetime and that is not allowed in pyo3.
Your example looks interesting, I'm going to see if that can lead me to something that works. Thanks!

@birkenfeld
Copy link
Member

But this seems like a weird thing to use inheritance for

I disagree. It's very common in Python to use subclasses for what we use an enum in Rust.

@mejrs
Copy link
Member

mejrs commented May 26, 2021

I disagree. It's very common in Python to use subclasses for what we use an enum in Rust.

Oh, like the factory pattern in Java? That does make sense - I've just never seen (never noticed, probably) any Python code doing it.

At first I wanted to return a struct that contained that enum. However that enum has a lifetime and that is not allowed in pyo3. Your example looks interesting, I'm going to see if that can lead me to something that works. Thanks!

☺️

The lifetime issue is not something you can paper over easily because Rust and Python's ownership mechanics are fundamentally different, but there are solutions for it:

  • Just Clone It
  • Hold and return references to Python objects
  • Turn the reference into something with a static lifetime, for example by wrapping the referenced object in a shared ownership construct like Rc/Arc.

What is right for you depends on how and why you're using references.

I'm probably looking right past it, but I can't see where in the examples there is a function that returns different sub-classes.

I might have answered a bit too quickly here 😳

I think the basic issue is that for a function to dynamically return one of potentially different Python objects, that object should be a Py<PyAny>. Therefore you must coerce the class into that, which you cannot do because pyclasses that extend do not implement IntoPy 🤔

@azerupi
Copy link
Author

azerupi commented May 27, 2021

The lifetime issue is not something you can paper over easily because Rust and Python's ownership mechanics are fundamentally different

Of course, I completely understand that. That's why I was trying to convert the enum into python classes with a parent class for common behavior.

I have some (soft) constraints on what I can do to the code with the lifetime. Basically we have made a Rust library to parse some protocol and would like to provide a Python API to use it in scripts. In this library a Parser struct has an internal buffer and the Frame enum variants can reference that buffer for their payload.

In an ideal situation I would like the parsing library to not have to clone/copy the parser's buffer to the frame and only do that in the Python API. That brought me to essentially the design in your example, where I convert each variant into a specific class, with the addition of a base class for common methods and data.

I think the basic issue is that for a function to dynamically return one of potentially different Python objects, that object should be a Py. Therefore you must coerce the class into that, which you cannot do because pyclasses that extend do not implement IntoPy

Yes, as @birkenfeld noted, you can turn SubClass into a PyObject, you can turn ParentClass into a PyObject but at the moment there doesn't seem to be a way to convert (SubClass, ParentClass) into a PyObject (or should it be (PyObject, ParentClass)?).

@althonos
Copy link
Member

I did do that (with a custom attribute macro) to turn an enum variant into the appropriate subclass, and again.

The IntoPyObject @mejrs suggested will probably work, while for the FromPy I ended up hacking something matching the qualified type name and casting (safe as long as nobody inherits from your base class on Python's side)

@davidhewitt
Copy link
Member

davidhewitt commented May 27, 2021

I disagree. It's very common in Python to use subclasses for what we use an enum in Rust.

Agreed - I've been wondering about how we might implement #[pyenum] (#417) in the future, and keep coming back to the conclusion that some kind of subclass structure would be most logical.

Maybe we should rethink the subclassing strategy and require a field of the subclass to be of type Base?

I've thought about exactly this in the past. To allow safely converting Py<Subclass> to Py<Base> we would need to guarantee that:

  • Base is the first field of Subclass
  • Subclass is #[repr(C)]

I don't think that this is possible to do in Rust's type system, so we would need to rely on unsafe and for users to keep this promise when subclassing. That seems undesirable to me, so the status quo (where we lay out the inheritance internally) seems great in comparison.


To add my own piece to this discussion, I think the (SubClass, ParentClass) return value of #[new] is a mistake for a number of reasons. (I get confused which way round should be all the time, and it looks like @birkenfeld does too.) I've actually been experimenting with a few alternatives in my own time, though haven't written a PR yet.

If I put my experiments together into a couple of PRs, in PyO3 0.14 we could write the original example something like this:

// PingFrame::new returns Py<PingFrame>

#[pymethods]
impl PingFrame {
    #[new]
    pub fn new(py: Python, sequence: u8) -> Py<PingFrame> {
        let frame = Frame::new_ping(sequence);
        let ping_frame: Self = frame.try_into().unwrap();
        pyo3::new_base(py, frame)
            .add_subclass(ping_frame)
            .finish()
    }
}

// And the parse function returns Py<Frame>

   fn parse(&mut self, py: Python, byte: u8) -> PyResult<Option<Py<Frame>>> {
        match self.parser.parse(byte) {
            Ok(f) => Ok(Some(PingFrame::new(py, 5).into_super())),
            Err(e) => return Err(ParseError { err: e }.into())
        }
    }

Perhaps my ideas are best explained in a series of PRs and issues (I can put more stuff together at the weekend). The relevant pieces of my ideas for this issue are the following:

  • make inherited structures more convenient by adding pyo3::new_base() to create a PyClassInitializer<Base>
  • add .finish() to convert PyClassInitializer<T> into Py<T>.
  • add .into_super() to convert Py<SubClass> to Py<Base>

@birkenfeld
Copy link
Member

I've thought about exactly this in the past. To allow safely converting Py<Subclass> to Py<Base> we would need to guarantee that:

* `Base` is the first field of `Subclass`
* `Subclass` is `#[repr(C)]`

I don't think that this is possible to do in Rust's type system,

It might not be possible in the type system, but in the proc-macro? At least adding #[repr(C)] is easy, checking for the base field might not be very pretty though :)

But your proposal is also nice, although it won't allow access to the base class data if you take a &Subclass argument.

@davidhewitt
Copy link
Member

davidhewitt commented May 27, 2021

It might not be possible in the type system, but in the proc-macro? At least adding #[repr(C)] is easy, checking for the base field might not be very pretty though :)

That's very true, I think both would be doable in the proc macro. Though the other downside (that I forgot to mention above) is that sometimes we might not know the memory layout of Base until runtime - in #991 and #1344. I've been reading around and think I can just about see a possible route to implement these if we're allowed to control the layout of the PyCell internally (we'd have to do sizing at runtime).

I guess we could always have different behaviour for Python base types vs Rust base types.

But your proposal is also nice, although it won't allow access to the base class data if you take a &Subclass argument.

I completely agree with this downside. That's one of the main reasons I've been thinking so hard on this topic.

I think it should be possible to make PyCell<Subclass> deref to PyCell<Base>, which would at least partially improve the situation...

@daemontus
Copy link
Contributor

daemontus commented Feb 23, 2023

Hi! I ran into this question while looking into how instances of a PyO3 subclasses should be handled when they appear as return types. I believe the discussion above is likely quite outdated, so I figured I might as well share my solution (which works with current PyO3), in case anyone else rediscovers this question.

fn function_that_returns_a_subtype(py: Python) -> PyResult<PyObject> {
    let initializer: PyClassInitializer<SuperType> = PyClassInitializer::from(
          todo!("Initialize an instance of SuperType.")
    );
    let object = if branch() {
        let initializer: PyClassInitializer<SubTypeA> = initializer.add_subclass(
              todo!("Initialize an instance of SubTypeA.")
        );
        PyCell::new(py, initializer)?.to_object(py)
    } else {
        let initializer: PyClassInitializer<SubTypeB> = initializer.add_subclass(
              todo!("Initialize an instance of SubTypeB.")
        );
        PyCell::new(py, initializer)?.to_object(py)
    };
    Ok(object)
}

Do you think it would be useful to add something along these lines to the PyO3 guide? Because if so, I'd be happy to turn it into a PR.

@oscarbenjamin
Copy link

Do you think it would be useful to add something along these lines to the PyO3 guide?

My 2 pence is yes it would be good to have something in the guide. I just spent a while looking before ending up here and your solution seems to do exactly what I wanted (thanks for that!).

Actually I wasn't even trying to return different subclasses from the same function but just to construct an instance of a sub-pyclass from rust code in general. If there is a better way to do that then I think it would be good to add that to the guide somewhere as well.

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

No branches or pull requests

8 participants