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

Consider "impl pyo3::FromPyObject for Arc<str>" and other smart pointers #2090

Closed
obi1kenobi opened this issue Jan 6, 2022 · 6 comments
Closed

Comments

@obi1kenobi
Copy link
Contributor

First, huge thanks to everyone that made this awesome library possible! I've been having a blast with it lately.

Right now, it seems that Rust functions callable from Python may take arguments like i64 or String, but cannot take smart pointer versions of those same arguments: Rc<str>, Arc<str>, Box<i64> etc. Would it be reasonable to add that functionality, or is there some fundamental reason why it's a bad idea? At the moment, my functions have to accept String then internally convert to Rc/Arc to satisfy downstream APIs, and it seems like that conversion could just happen inside pyo3 itself.

If anyone familiar with pyo3 internals is available to mentor, I'd be open to trying to put together a PR for this. Thanks again!

@birkenfeld
Copy link
Member

One the one hand this would probably be easy to do, on the other hand, doing the Arc::new() (or just .into()!) in the function doesn't seem like a big burden, and in the case of Rc/Arc it avoids a mistaken impression that PyO3 is doing something smart behind the scenes that would avoid creating a new String for each all.

@davidhewitt
Copy link
Member

in the case of Rc/Arc it avoids a mistaken impression that PyO3 is doing something smart behind the scenes that would avoid creating a new String for each all.

This is exactly why I'm on the fence about this too. It would be desirable for users to not assume the wrong thing here.

Note that you can achieve most of this yourself using the #[pyo3(from_py_with)] annotation. E.g. I think something like the below (I've just written by hand so there may be typos):

fn make_rc<T>(src: &PyAny) -> PyResult<Rc<T>>
where
    T: for<'a> FromPyObject<'a>
{
    src.extract().map(Rc::new)
}

#[pyfunction]
fn foo(
    #[pyo3(from_py_with = "make_rc")]
    arg: Rc<i32>,
) {
    // ...
}

@obi1kenobi
Copy link
Contributor Author

#[pyo3(from_py_with)] is great, thanks for suggesting it! Unfortunately, this part is tripping me up: The function signature must be fn(&PyAny) -> PyResult<T> where T is the Rust type of the argument.

Entirely possible I might be misusing or misunderstanding something, but my code right now operates on Py<PyAny> types instead of &PyAny since my current approach requires the Python values to be GIL-independent. I have a Rust function that takes a HashMap<String, Py<PyAny>> argument, and need to turn it into HashMap<Arc<str>, Py<PyAny>> to satisfy a downstream API. Is that possible with #[pyo3(from_py_with)] or something similarly elegant?

Thanks again for the help and for maintaining this awesome project!

@davidhewitt
Copy link
Member

davidhewitt commented Jan 8, 2022

I think you can still use from_py_with, just the function you need to write is fn hashmap_of_arcs(src: &PyAny) -> PyResult<HashMap<Arc<str>, Py<PyAny>>>?

@obi1kenobi
Copy link
Contributor Author

I think you can still use from_py_with, just the function you need to write is fn hashmap_of_arcs(src: &PyAny) -> PyResult<HashMap<Arc<str>, Py<PyAny>>>?

Wow, I didn't realize I could do that. Thanks, that worked like a charm! I think that's the definitive answer to this issue, so I'm going to close it unless you feel strongly about keeping it open.

Thanks again for all the help!

@ChayimFriedman2
Copy link
Contributor

What with Box? It doesn't have Arc's problem, so why not impl PyFromObject for it?

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