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

Normalize on py_foo() and foo() function name pattern in accelerate crate #12936

Open
mtreinish opened this issue Aug 9, 2024 · 3 comments · May be fixed by #13163
Open

Normalize on py_foo() and foo() function name pattern in accelerate crate #12936

mtreinish opened this issue Aug 9, 2024 · 3 comments · May be fixed by #13163
Labels
good first issue Good for newcomers Rust This PR or issue is related to Rust code in the repository type: feature request New feature or request

Comments

@mtreinish
Copy link
Member

What should we add?

In the accelerate crate when we have a split between a python facing function and a rust entrypoint for internal reuse we typically used the pattern where the rust function was named foo_inner() and the pyfunction was named foo(). For example:

https://github.com/Qiskit/qiskit/blob/main/crates/accelerate/src/dense_layout.rs#L101-L130

However in the circuit crate we started using a different pattern, where the pyfunction was named py_foo and the name attribute on the pyfunction macro was the name without the py prefix. As we're writing an increasing amount of rust code this seems like a better pattern. So we should migrate the accelerate crate to follow the same pattern internally.

@mtreinish mtreinish added good first issue Good for newcomers type: feature request New feature or request Rust This PR or issue is related to Rust code in the repository labels Aug 9, 2024
@jakelishman
Copy link
Member

Fwiw, I've been changing my naming conventions as I go, and at the moment I'm roughly around using a py_ prefix more to mean "this function deals with / returns objects for Python-space consumption" as opposed to "this function deals only with Rust objects", and avoiding using a prefix when it wasn't necessary to disambiguate (like the function is a term that only applies to Python space, or there's no chance of confusion with a Rust-specific method), rather than it being specifically pyfunctions/pymethods and not.

@raynelfss
Copy link
Contributor

Thank you for bringing this up, yes we should label the #[pymethods] that are python only and use the py_ prefix for each of those. I guess my only question would be what happens when the method has a hybrid use? For example, yes the method is exposed to python through #[pymethods] but it's not so different in its structure that it could be left as a method that's exposed to rust as well to avoid duplicating code.

  • Do we call it py_foo() and create a separate duplicate rust-native method foo() that's called by py_foo()?
  • Or would we be allowed to make an exception and just have one method foo() that's exposed through #[pymethods] but it is public within Rust?

I also think it is important to talk about methods that we'd like to keep in a private api of rust that use a py: Python argument and have no use in python. Do we create a third impl Foo to store these as well?

There's still a lot of figuring out to do when it comes to how to properly write Rust code that will be exposed to Python, but I think it is good we start thinking of these.

@mtreinish
Copy link
Member Author

I don't think we need to make a separate function for #[pymethods] if there is a way to write a function such that the returns and inputs are all python agnostic so that you can have a single implementation that works equally well from rust and python we can use the rust name without issue. PyO3 gives you a lot of tools to write a python function/method that is rust-native and performant that is also exposed to Python. In practice I've found for efficient python access we end up needing to handle the python<-> rust boundary manually but there is nothing stopping us from writing shared functions in theory. So if we had one we could just use foo(). I think we should reserve the py_ prefix for cases where it really is python specific.

@tglanz tglanz linked a pull request Sep 16, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Rust This PR or issue is related to Rust code in the repository type: feature request New feature or request
Projects
Status: Tagged but unassigned
Development

Successfully merging a pull request may close this issue.

3 participants