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

get return values from run_until_complete #42

Open
davidhewitt opened this issue Aug 11, 2021 · 7 comments
Open

get return values from run_until_complete #42

davidhewitt opened this issue Aug 11, 2021 · 7 comments

Comments

@davidhewitt
Copy link

It might be nice if the signature of run_until_complete functions became:

pub fn run_until_complete<F, R>(event_loop: &PyAny, fut: F) -> PyResult<R> where
    F: Future<Output = PyResult<R>> + Send + 'static, 

... so that return values from anything running inside can propagate.

Just an idea, if it makes sense.

@awestlake87
Copy link
Owner

Yeah, I agree that the return types on several functions should be more generic.

One change that I was thinking about making recently was making future_into_py, and functions like it, return PyResult<T> where T: IntoPy<PyObject> instead of PyResult<PyObject>. This is a pretty straightforward change to make, and it didn't seem to have too much of an impact on my existing pyo3-asyncio code. In fact it made a lot of my tests cleaner since I could return Ok(()) instead of Python::with_gil(|py| Ok(py.None())). The main exception being when it can't infer the return type, like when someone uses .into() to turn a &PyAny into a PyObject. For this reason, I'm not sure how much of an impact this will have on people downstream.

I think it makes sense to change run_until_complete and run like you suggest as well.

@ShadowJonathan
Copy link

Personally I find being able to return Ok(()) very appealing.

Though according to docs, that gets converted to an empty python tuple?

@awestlake87
Copy link
Owner

Though according to docs, that gets converted to an empty python tuple?

That's unfortunate. Idk if it's worth changing future_into_py and local_future_into_py then.

Maybe we could have a struct None; where IntoPy<PyObject> returns py.None()? That way we could return Ok(None) and have it convert correctly. It might be nice to have something like that in the pyo3 core.

@ShadowJonathan
Copy link

That might conflict with None of Option<>, though maybe introducing PyNone to pyo3 could work.

@davidhewitt
Copy link
Author

Actually () does get converted to None - there's IntoPy<PyObject> and IntoPy<Py<PyTuple>>.

However is there a reason to constrain the type parameter? Might be a detail I'm missing, but it seems most flexible for the user if it returns any old PyResult<T>?

@awestlake87
Copy link
Owner

awestlake87 commented Aug 13, 2021

future_into_py makes a Python awaitable, so the value it returns must be converted into a Python value. It also needs to be Send + 'static since it's returned from a Rust runtime thread.

I didn't constrain the return values of run and run_until_complete past Send + Sync + 'static in that PR since they are intended to be used directly from Rust. So they should be able to return most Rust values you'd want.

@davidhewitt
Copy link
Author

Ah right, of course. I think IntoPy<PyObject> should be the right type constraint for future_into_py in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants