Unify Python custom instruction kinds by jakelishman · Pull Request #15652 · Qiskit/qiskit

jakelishman · 2026-02-05T15:18:43Z

Summary

In gh-15277¹ we unified the internal storage of the three variants of Python operation in a single PyOperationTypes enum, where each variant wraps the same PyInstruction type. We didn't change the handling within the OperationRef view object, to avoid modifying how all the downstream matching would happen.

As we move towards removing the Python structures from qiskit-circuit entirely, we will replace the field with a custom operation. We will have to decide how much of a type-level split between different objects we want to maintain, and the "custom instruction" code will all have down some form of dynamic-typing path.

As a stepping stone towards that, this commit moves the PyOperationTypes enum discriminant into PyInstruction itself, which has the knock-on effect of requiring OperationRef to reflect the same backing structure. This was mentioned as a possibility in the original PR, but not attempted at the time due to worries of less ergonomic matching.

In practice, the matching is actually easier overall in this form because of the significant de-deduplication in most cases; in many cases, the handling of Python-space objects is independent of their types. With the kind field now inside PyInstruction, all the methods of PyInstruction like matrix, which were already fallible can now simply handle the Gate/Instruction/Operation split themselves, so there's no risk of a forgotten match causing trouble.

The legacy three-fold Gate/Instruction/Operation split is already a relic of Python space that Rust mostly shouldn't need to care about, particularly the Operation element of it. It may well make sense in future custom gates to have a way of checking for "unitary" vs "non-unitary" without requesting the matrix, but this is compatible with PyInstruction being the single implementation of a "custom" gate from Rust's perspective to wrap all Python objects.

Details and comments

This includes a roll-up of #15649 right now, because I found that failure while making this PR.

This PR can also be thought of as preliminary work to Ray's work on making it possible to define custom instructions from Rust. We don't have to merge this, but it's worth considering from an API perspective about how we'll want to present a "unitary"/"non-unitary" split in Rust space for matching.

The diff is a bit inflated because of substantial changes to assign_parameters to make the match ergonomics more reliable, and to correctly handle StandardInstruction::Delay without using the general-purpose Python-dependent path. I could separate that change out into a preceding PR, if preferred.

7deafbf: Combine Python types in PackedInstructions ↩

In Qiskitgh-15277[^1] we unified the internal storage of the three variants of Python operation in a single `PyOperationTypes` enum, where each variant wraps the same `PyInstruction` type. We didn't change the handling within the `OperationRef` view object, to avoid modifying how all the downstream matching would happen. As we move towards removing the Python structures from `qiskit-circuit` entirely, we will replace the field with a custom operation. We will have to decide how much of a type-level split between different objects we want to maintain, and the "custom instruction" code will all have down some form of dynamic-typing path. As a stepping stone towards that, this commit moves the `PyOperationTypes` enum discriminant into `PyInstruction` itself, which has the knock-on effect of requiring `OperationRef` to reflect the same backing structure. This was mentioned as a possibility in the original PR, but not attempted at the time due to worries of less ergonomic matching. In practice, the matching is actually easier overall in this form because of the significant de-deduplication in most cases; in many cases, the handling of Python-space objects is independent of their types. With the `kind` field now inside `PyInstruction`, all the methods of `PyInstruction` like `matrix`, which were _already_ fallible can now simply handle the `Gate`/`Instruction`/`Operation` split themselves, so there's no risk of a forgotten match causing trouble. The legacy three-fold `Gate`/`Instruction`/`Operation` split is already a relic of Python space that Rust _mostly_ shouldn't need to care about, particularly the `Operation` element of it. It may well make sense in future custom gates to have a way of checking for "unitary" vs "non-unitary" without requesting the matrix, but this is compatible with `PyInstruction` being the single implementation of a "custom" gate from Rust's perspective to wrap all Python objects. [^1]: 7deafbf: Combine Python types in PackedInstructions

qiskit-bot · 2026-02-05T15:18:48Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@levbishop
@mtreinish

coveralls · 2026-02-05T15:58:22Z

Pull Request Test Coverage Report for Build 21717172255

Details

372 of 457 (81.4%) changed or added relevant lines in 26 files are covered.
23 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.09%) to 88.053%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/cext/src/dag.rs	0	1	0.0%
crates/synthesis/src/two_qubit_decompose.rs	7	8	87.5%
crates/transpiler/src/passes/high_level_synthesis.rs	2	3	66.67%
crates/transpiler/src/passes/unitary_synthesis/decomposers.rs	3	4	75.0%
crates/circuit/src/operations.rs	55	57	96.49%
crates/transpiler/src/passes/basis_translator/mod.rs	12	14	85.71%
crates/circuit/src/dag_node.rs	0	3	0.0%
crates/qpy/src/value.rs	11	14	78.57%
crates/quantum_info/src/convert_2q_block_matrix.rs	4	7	57.14%
crates/qpy/src/circuit_writer.rs	45	49	91.84%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/parameter/parameter_expression.rs	1	87.17%
crates/qpy/src/value.rs	1	81.74%
crates/synthesis/src/discrete_basis/solovay_kitaev.rs	1	81.52%
crates/transpiler/src/passes/basis_translator/mod.rs	1	87.21%
crates/transpiler/src/passes/high_level_synthesis.rs	1	87.09%
crates/circuit/src/circuit_instruction.rs	2	81.28%
qiskit/circuit/delay.py	2	73.26%
crates/circuit/src/circuit_data.rs	3	89.82%
crates/qasm2/src/lex.rs	5	92.03%
crates/circuit/src/packed_instruction.rs	6	92.61%

Totals
Change from base Build 21716415231:	0.09%
Covered Lines:	99789
Relevant Lines:	113328

💛 - Coveralls

gadial

Overall looks good to me. Left some comments - there might be some small fixes required.

gadial · 2026-02-17T08:54:19Z

-                    OperationRef::Unitary(unitary) => unitary.clone().into(),
-                    OperationRef::PauliProductMeasurement(ppm) => ppm.clone().into(),
-                }
+                self.instruction.operation.py_deepcopy(py, None)?


Not sure I understand why we can now ignore OperationRef::ControlFlow etc.

We aren't ignoring them, we're just delegating the match to the PackedOperation method that does the same thing - it's just a de-duplication thing.

gadial · 2026-02-17T08:55:20Z

        let definition = match self.instruction.operation.view() {
            OperationRef::StandardGate(g) => g.definition(self.instruction.params_view()),
-            OperationRef::Gate(g) => g.definition(),
-            OperationRef::Instruction(i) => i.definition(),


Previously Operation was caught by the catch-all? So now the behavior was changed?

This is one of the core changes and arguments for/against unifying into PyCustom rather than three separate kinds that everywhere has to know what the semantics are. Now, because all these methods (definition in this case) were fallible already, even for Gate and Instruction which "supported" them, there's just a catch within PyCustom::definition that knows to return None if the kind is an operation without performing an invalid Python call, so call sites don't have to have that baked-in knowledge of the Python semantics.

gadial · 2026-02-17T08:55:53Z

    fn try_matrix(&self) -> Option<Array2<Complex64>> {
        match self.op() {
            OperationRef::StandardGate(g) => g.matrix(self.params_view()),
-            OperationRef::Gate(g) => g.matrix(),


Also here, Operation and Instruction are treated separately?

Same as before, but matrix is perhaps more contentious than definition. There's an argument that Gate and Instruction could/should be separate types in the custom-gates system that Ray's working on (but Operation almost certainly shouldn't), and this PR was part of an exploration of that.

Personally, I think I'm currently in the camp that the "unitary" marker needn't be a type-level marker (i.e. no need to separate Gate and Instruction at the type level for custom gates) - I think there's value in it being dynamically decided by a class, but I could be convinced.

gadial · 2026-02-17T08:58:20Z

+            Ok(Some(Self::Instruction))
+        } else {
+            ob.is_subclass(imports::OPERATION.get_bound(py))
+                .map(|ok| ok.then_some(Self::Operation))


Seems we don't return informative error if we fail?

Returning Ok(None) is (imo) completely informative from a programmatic perspective - it's not an error to call this function with a Python type object that isn't in the Operation hierarchy, it just doesn't correspond to a PyOpKind.

gadial · 2026-02-17T09:10:13Z

    /// returns 2^num_ctrl_bits-1 (the '11...1' state) if the gate has not control state data
    pub fn ctrl_state(&self) -> u32 {
+        if self.kind != PyOpKind::Gate {
+            return 0;


I don't think we should return 0 here. Note the comment: returns 2^num_ctrl_bits-1 (the '11...1' state) if the gate has not control state data

If num_ctrl_bits is 0 (which it would be if catch triggers), then $2^0 - 1 = 0$ and it's consistent, no?

Arguably this method (and num_ctrl_bits) are completely ill defined for self.kind != PyOpKind::Gate, but that was pre-existing.

Right... But I'd still feel more comfortable is the returned result relied on num_ctrl_qubits explicitly, due to my fear of the unknown tomorrow where we decide to change one and forget the other.

I changed it to be explicitly Option<u32> in 7b51661, which is the most complete way of doing this.

(But just a reminder - #15649 needs review/merge before this PR can merge.)

gadial · 2026-02-17T09:15:57Z

        PyValueError::new_err(format!("Could not find operation data for {}", name))
    })?;
+    let OperationRef::PyCustom(inst) = operation.view() else {
+        panic!(


I think our goal is to avoid panics inside QPY code? Or is it relevant only for reader, not writer?

It's 100% true for the reader (and actually I need to talk to you about that in general - there are loads of panics currently possible in the Rust QPY, mostly via expect and unwrap calls), but this case is a panic because it corresponds purely to an internal logic error earlier in the code that can't be triggered just because of untrusted input. This function must only be called (by ourselves) with keys that correspond to PyCustom instructions - if we do something other than that, there's an error in our own QPY code that's completely impossible to recover from.

The previous version of this function didn't panic, but it silently let the invalid call pass, which is going to at best produce an invalid QPY payload that fails to deserialise (so we can spot the problem) but at worst produce a valid QPY payload that fails to roundtrip the circuit correctly.

I was trying to avoid making larger-scale changes here, but really this should be handled at the type-system level in the arguments passed to this function.

jakelishman

Thanks for the review Gadi. This PR is an "on-hold" PR though, because it's based on #15649, which should be reviewed and merged separately and before this one.

jakelishman · 2026-02-17T12:15:49Z

-                    OperationRef::Unitary(unitary) => unitary.clone().into(),
-                    OperationRef::PauliProductMeasurement(ppm) => ppm.clone().into(),
-                }
+                self.instruction.operation.py_deepcopy(py, None)?


We aren't ignoring them, we're just delegating the match to the PackedOperation method that does the same thing - it's just a de-duplication thing.

jakelishman · 2026-02-17T12:18:45Z

        let definition = match self.instruction.operation.view() {
            OperationRef::StandardGate(g) => g.definition(self.instruction.params_view()),
-            OperationRef::Gate(g) => g.definition(),
-            OperationRef::Instruction(i) => i.definition(),


This is one of the core changes and arguments for/against unifying into PyCustom rather than three separate kinds that everywhere has to know what the semantics are. Now, because all these methods (definition in this case) were fallible already, even for Gate and Instruction which "supported" them, there's just a catch within PyCustom::definition that knows to return None if the kind is an operation without performing an invalid Python call, so call sites don't have to have that baked-in knowledge of the Python semantics.

jakelishman · 2026-02-17T12:22:47Z

    fn try_matrix(&self) -> Option<Array2<Complex64>> {
        match self.op() {
            OperationRef::StandardGate(g) => g.matrix(self.params_view()),
-            OperationRef::Gate(g) => g.matrix(),


Same as before, but matrix is perhaps more contentious than definition. There's an argument that Gate and Instruction could/should be separate types in the custom-gates system that Ray's working on (but Operation almost certainly shouldn't), and this PR was part of an exploration of that.

Personally, I think I'm currently in the camp that the "unitary" marker needn't be a type-level marker (i.e. no need to separate Gate and Instruction at the type level for custom gates) - I think there's value in it being dynamically decided by a class, but I could be convinced.

jakelishman · 2026-02-17T12:24:28Z

+            Ok(Some(Self::Instruction))
+        } else {
+            ob.is_subclass(imports::OPERATION.get_bound(py))
+                .map(|ok| ok.then_some(Self::Operation))


Returning Ok(None) is (imo) completely informative from a programmatic perspective - it's not an error to call this function with a Python type object that isn't in the Operation hierarchy, it just doesn't correspond to a PyOpKind.

jakelishman · 2026-02-17T12:25:57Z

    /// returns 2^num_ctrl_bits-1 (the '11...1' state) if the gate has not control state data
    pub fn ctrl_state(&self) -> u32 {
+        if self.kind != PyOpKind::Gate {
+            return 0;


If num_ctrl_bits is 0 (which it would be if catch triggers), then $2^0 - 1 = 0$ and it's consistent, no?

Arguably this method (and num_ctrl_bits) are completely ill defined for self.kind != PyOpKind::Gate, but that was pre-existing.

jakelishman · 2026-02-17T12:31:21Z

        PyValueError::new_err(format!("Could not find operation data for {}", name))
    })?;
+    let OperationRef::PyCustom(inst) = operation.view() else {
+        panic!(


It's 100% true for the reader (and actually I need to talk to you about that in general - there are loads of panics currently possible in the Rust QPY, mostly via expect and unwrap calls), but this case is a panic because it corresponds purely to an internal logic error earlier in the code that can't be triggered just because of untrusted input. This function must only be called (by ourselves) with keys that correspond to PyCustom instructions - if we do something other than that, there's an error in our own QPY code that's completely impossible to recover from.

The previous version of this function didn't panic, but it silently let the invalid call pass, which is going to at best produce an invalid QPY payload that fails to deserialise (so we can spot the problem) but at worst produce a valid QPY payload that fails to roundtrip the circuit correctly.

I was trying to avoid making larger-scale changes here, but really this should be handled at the type-system level in the arguments passed to this function.

alexanderivrii

I took an (honestly rather quick) look at the PR. The idea to combine Py {Gate, Instruction, Operation} into a single PyCustom makes sense to me. Ergonomically-wise, whenever we need to do the same thing in each of the 3 cases, the PR makes the code shorter by removing deduplication. On the other hand, whenever we need a specific variant (usually PyGate), the match statements become a bit longer and harder to read. Creating a Python object from within Rust space did not change much (except for specifying the op kind of the operation). Overall, I am in favor of this, provided that it helps Ray's thread of work.

One general question: how do you see ControlledGate/AnnotatedOperation in Rust in the future?

alexanderivrii · 2026-02-22T12:53:58Z

+    pub fn num_ctrl_qubits(&self) -> Option<u32> {
+        if self.kind != PyOpKind::Gate {
+            return None;
+        }


Completely unrelated to this PR: why do we need the methods num_ctrl_qubits and ctrl_state? This seems to bring up closer to ControlledGate, which I don't think we want in the Rust space.

alexanderivrii · 2026-02-22T13:00:08Z

+        // The `definition` attribute isn't part of the `Operation` interface, so it's invalid for
+        // us to access it.
+        if self.kind == PyOpKind::Operation {
+            return None;
+        }


Hmm, while I mostly agree with the comment above, we have not really prevented from including custom definitions for Operations, so I am wondering if this might break expectations.

alexanderivrii · 2026-02-22T13:05:27Z

+    let gate_ob = match inst.op.view() {
+        OperationRef::PyCustom(PyInstruction {
+            kind: PyOpKind::Gate,
+            ob,
+            ..
+        }) => ob,


This is somewhat less ergonomic than it was before.

jakelishman added 2 commits February 5, 2026 14:11

DNM: rollup of qpy/annotations

c5a9dd1

jakelishman added this to the 2.4.0 milestone Feb 5, 2026

jakelishman requested a review from a team as a code owner February 5, 2026 15:18

jakelishman added Changelog: None Do not include in the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Feb 5, 2026

github-project-automation Bot added this to Qiskit 2.4 Feb 5, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.4 Feb 5, 2026

jakelishman added the on hold Can not fix yet label Feb 5, 2026

gadial reviewed Feb 17, 2026

View reviewed changes

jakelishman commented Feb 17, 2026

View reviewed changes

Use explicit None for maybe-absent data

7b51661

alexanderivrii reviewed Feb 22, 2026

View reviewed changes

alexanderivrii modified the milestones: 2.4.0, 2.5.0 Mar 18, 2026

alexanderivrii removed this from Qiskit 2.4 Mar 18, 2026

alexanderivrii added this to Qiskit 2.5 Mar 18, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 Mar 18, 2026

Conversation

jakelishman commented Feb 5, 2026

Summary

Details and comments

Footnotes

Uh oh!

qiskit-bot commented Feb 5, 2026

Uh oh!

coveralls commented Feb 5, 2026

Pull Request Test Coverage Report for Build 21717172255

Details

💛 - Coveralls

Uh oh!

gadial left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants