Remove Python dependency from circuit construction #13986
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13820220898Details
💛 - Coveralls |
Cryoris
reviewed
Mar 11, 2025
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.
There was a problem hiding this comment.
The other, longer-term solution would be to wait until ParameterExpression is in Rust and have a clean Circuit - PyCircuit split, then extend would naturally be py-free. But I'm good as is for now 👍🏻
| @@ -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.
1 task
Cryoris
approved these changes
Mar 13, 2025
There was a problem hiding this comment.
LGTM, I just have one question that can be answered in parallel: how expensive is a repetitive call to Py::with_gil compared to passing the py token? I'm thinking e.g. of a circuit construction where we have a parameterized global phase and repeatedly add to this phase.
There is definitely an overhead to this, |
|
I ran asv with this PR (rebased on current main) against main and this was the result: I'm not sure how reliable some of the numbers are TBH, for some of the benchmarks we're in a time scale where 5-6% is ~1 μs. I didn't run these on an idle system so other system noise can cause discrepencies like that. It's not showing any major regression though. |
|
Thanks for running the benchmarks! Seems like it doesn't change much and it'll enable the C circuits 👍🏻 We'll get a clean py-token-flow again once we correctly split the Python interface off I guess, so any slow down would only be an intermediate state 🙂 |
raynelfss
pushed a commit
to raynelfss/qiskit
that referenced
this pull request
Mar 20, 2025
* Remove Python dependency from circuit construction 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. * Remove with_gil for calling extend() in new()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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