Avoid py token usage in the common path for circuit_to_dag#14589
Merged
mtreinish merged 3 commits intoQiskit:mainfrom Jul 7, 2025
Merged
Avoid py token usage in the common path for circuit_to_dag#14589mtreinish merged 3 commits intoQiskit:mainfrom
mtreinish merged 3 commits intoQiskit:mainfrom
Conversation
mtreinish
added
Changelog: None
Collaborator
|
One or more of the following people are relevant to this code:
|
mtreinish
force-pushed
the
hide-the-py-circ-to-dag
branch
3 times, most recently
from
dc8afe6 to
01b6788
Compare
This commit adjusts the Python usage in the DAGCircuit::from_circuit_data code paths. Most of the data model is in Rust at this point with the exception of things explicitly defined in Python (ie a custom gate in Python), ParameterExpression, and control flow. While the latter two are in progress (see Qiskit#13267 and Qiskit#13271) we don't need to block the development of C API features that depend on circuit->dag conversion. The Python usage in this code path is around parameterized global phase and deepcopying custom python gates. Neither of these are possible from C because they're in Python and not exposed to C. This commit removes the py token from the function signatures and actively acquires the gil inline if we're using Python parts of the data model when they're needed.
kevinhartman
previously approved these changes
Jun 12, 2025
Contributor
kevinhartman
left a comment
kevinhartman
left a comment
There was a problem hiding this comment.
This looks good to me (... and, it's a lot more minimal than I was expecting given your description heh 🙂).
jakelishman
reviewed
Jun 12, 2025
Member
|
I removed this from the queue on the basis of the previous comment. I don't mind being overruled and it re-added to the queue, but I think it should be considered first. I think we should be being a lot more careful about "hide the py". A method which is quite literally asking for Python-space deepcopies in its very name really feels like it should require the GIL to call. |
Member
|
Also, is the milestone set right? I'd have assumed this was for 2.2. |
Pull Request Test Coverage Report for Build 15744392705Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
In the previous commit the py token acquisition was moved down into the PackedInstruction::py_deepcopy() method instead of being the responsibility of the caller. This decision was made because we only need the py token in a context where objects defined in Python are encountered. The goal of hiding the py token is that in a context without python at all (i.e. the standalone C API) nothing callable from a pure rust context requires the py token. The token was moved down so that we only ever try to acquire the GIL in contexts where we have a python defined object. The method only ever uses it to deep copy in the case of PackedInstruction containing a Python defined object. In code review the concern was raised that deepcopy is a purely python construct and the `py_deepcopy` should very explicitly be the caller's responsibility to assert they have the GIL by passing the py token. To address this concern but also enable a Python free path through circuit to dag conversion this commit moves the `deepcopy()` (and `py_copy()`) methods up a layer so that they're defined on the Python owned operations. A new trait `PythonOperation` is added that contains these methods and the trait is implemented for the Python defined operation types: `PyGate`, `PyInstruction`, and `PyOperation`. This makes it more explicit where these methods are callable and makes the python usage explicit from the caller.
mtreinish
changed the title
raynelfss
approved these changes
Jul 3, 2025
Contributor
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
This looks good to me!
I thought about suggesting all of the matching to happen within a method or something similar, but with how it looks right now, I don't think there's a way around it considering that we will only call this for certain types of operations. We can always follow it up and think of a better-looking way of doing it.
That said, make sure you update the PRs description to include the PythonOperation trait's description and why it was added :)
Member
Author
It's in the commit message for the change: 2e496e6 |
12 tasks
aaryav-3
pushed a commit
to aaryav-3/qiskit
that referenced
this pull request
Oct 21, 2025
) * Avoid py token usage in the common path for circuit_to_data This commit adjusts the Python usage in the DAGCircuit::from_circuit_data code paths. Most of the data model is in Rust at this point with the exception of things explicitly defined in Python (ie a custom gate in Python), ParameterExpression, and control flow. While the latter two are in progress (see Qiskit#13267 and Qiskit#13271) we don't need to block the development of C API features that depend on circuit->dag conversion. The Python usage in this code path is around parameterized global phase and deepcopying custom python gates. Neither of these are possible from C because they're in Python and not exposed to C. This commit removes the py token from the function signatures and actively acquires the gil inline if we're using Python parts of the data model when they're needed. * Move deepcopy Python usage up to Python defined types In the previous commit the py token acquisition was moved down into the PackedInstruction::py_deepcopy() method instead of being the responsibility of the caller. This decision was made because we only need the py token in a context where objects defined in Python are encountered. The goal of hiding the py token is that in a context without python at all (i.e. the standalone C API) nothing callable from a pure rust context requires the py token. The token was moved down so that we only ever try to acquire the GIL in contexts where we have a python defined object. The method only ever uses it to deep copy in the case of PackedInstruction containing a Python defined object. In code review the concern was raised that deepcopy is a purely python construct and the `py_deepcopy` should very explicitly be the caller's responsibility to assert they have the GIL by passing the py token. To address this concern but also enable a Python free path through circuit to dag conversion this commit moves the `deepcopy()` (and `py_copy()`) methods up a layer so that they're defined on the Python owned operations. A new trait `PythonOperation` is added that contains these methods and the trait is implemented for the Python defined operation types: `PyGate`, `PyInstruction`, and `PyOperation`. This makes it more explicit where these methods are callable and makes the python usage explicit from the caller.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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
This commit adjusts the Python usage in the DAGCircuit::from_circuit_data code paths. Most of the data model is in Rust at this point with the exception of things explicitly defined in Python (ie a custom gate in Python), ParameterExpression, and control flow. While the latter two are in progress (see #13267 and #13271) we don't need to block the development of C API features that depend on circuit->dag conversion. The Python usage in this code path is around parameterized global phase and deepcopying custom python gates. Neither of these are possible from C because they're in Python and not exposed to C. This commit removes the py token from the function signatures and actively acquires the gil inline if we're using Python parts of the data model when they're needed.
Details and comments