Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.06k stars 2.33k forks source link

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

Open mtreinish opened 1 month ago

mtreinish commented 1 month ago

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.

jakelishman commented 1 month ago

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 commented 1 month ago

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.

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 commented 1 month ago

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.