Remove Python dependency from circuit construction #13986
Conversation
In preparation for providing a C API for building and interacting with circuits this commit removes the required Python component from the circuit construction APIs on CircuitData. After Qiskit#13860 the only use case of Python in the circuit constructor is to manipulate a ParameterExpression global phase as ParameterExpression is still a Python only construct. This moves to using Python::with_gil() in those places only when it's needed. This gives us a path for building a circuit without the Python interpreter for the C API.
mtreinish
added
Changelog: None
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13820220898Details
💛 - Coveralls |
There was a problem hiding this comment.
This looks like a nice start, it'll allow us to add C API functions to generate a circuit and add standard gates. I left one comment about copying below and another question is how we can allow circuit inspection; e.g. it would be nice if we can give index access to CircuitInstructions -- but these are very py-heavy right now 😅
crates/circuit/src/circuit_data.rs
Outdated
| self_.reserve(py, reserve); | ||
| self_.extend(py, data)?; | ||
| self_.reserve(reserve); | ||
| Python::with_gil(|py| -> PyResult<()> { self_.extend(py, data) })?; |
There was a problem hiding this comment.
It would be nice if we could also do this without the py token to enable copying circuits. What do you think about moving the with_gil inside extend in a way that we check if an instruction has parameter expressions and only then acquire the GIL? Maybe this has too much overhead and we can just wait for the Rust parameter expressions, too 😄
There was a problem hiding this comment.
I left it here because extend() was being passed a Bound<PyAny> I can drop the py here and just use it from the bound.
There was a problem hiding this comment.
Fixed in 004b7b8 If we need to call extend() in a no python context we'll probably want to split the method into two variants, one for rust data types the other for python. We can save that for a follow up PR though.
| @@ -159,7 +158,7 @@ impl CircuitData { | |||
| qubit_indices: BitLocator::new(), | |||
There was a problem hiding this comment.
The BitLocator has a cached: OnceLock<Py<PyDict>>, can this become an issue at runtime or is this fine since there's no way we can populate the cache with Python objects?
There was a problem hiding this comment.
It shouldn't be a problem we should only be initializing or working with the cached attribute when returning to python. There shouldn't be any code paths we access from cext where we could use this (or BitLocator more broadly tbh). We can refactor more if we do encounter issues though.
Summary
In preparation for providing a C API for building and interacting with circuits this commit removes the required Python component from the circuit construction APIs on CircuitData. After #13860 the only use case of Python in the circuit constructor is to manipulate a ParameterExpression global phase as ParameterExpression is still a Python only construct. This moves to using Python::with_gil() in those places only when it's needed. This gives us a path for building a circuit without the Python interpreter for the C API.
Details and comments