Add Custom Gates/Instructions in Rust#15684
Conversation
- Add `CustomOperation` trait that allows users to implement methods available in both `Instruction`s and `Gate`s. - Add the packed representation of `CustomOperation` within a `PackedOperation` called `NativeOperation`. - Use example within `BasisTranslator`'s `compose transforms function where a dummy gate called `CustomDummy` is added to each `DAGCircuit` instance natively. - Adapt codebase to new additions.
Pull Request Test Coverage Report for Build 22157447980Warning: 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 |
- Add missing case for `CustomInstruction` views in QPY. - Add docstring to `CustomOperation` trait. - Add miri flag to `try_add_to_circuit`.
- Fix formatting and clean up tests.
- Add missing docstrings.
|
One or more of the following people are relevant to this code:
|
- The following commit removes different views for `Instruction` vs `Gate` in favor of a consolidated view. Users should be able to use `CustomOperation::kind()` to see the type of operation enclosed.
- Add `CircuitData::push_custom_operation` method to directly push `CustomOperation` instances into a `CircuitData` instance. - Add `PackedInstruction::from_custom_operation` method to create a full packed instruction directly from a `CustomOperation` implementor.
mrossinek
left a comment
There was a problem hiding this comment.
Okay this looks great from what I am able to tell!
I might be missing some context on the internal workings and relationship of Operation, Instruction and Gate, so some of these questions may be trivial/irrelevant, but I figured I'd comment on what stood out to me while going through the changes anyways 👍
| /// If the instance is a gate, returns the unitary matrix that represents it, | ||
| /// if the parameters are correct. Otherwise, it returns None. | ||
| fn matrix(&self, _params: &[Param]) -> Option<Array2<Complex64>> { | ||
| // TODO: Make fallible. |
There was a problem hiding this comment.
Is this to be done still in this PR or left as a follow-up?
There was a problem hiding this comment.
Probably for a follow up. The reason here being mainly that the return type would become more complex. We can use tools like anyhow or thiserror to implement Result return types. But to keep the PR small and have it be the initial implementation, we'd leave those for later.
| /// If the instance is a gate, returns the number of control qubits. | ||
| fn num_ctrl_qubits(&self) -> Option<NonZero<u32>> { | ||
| None | ||
| } |
There was a problem hiding this comment.
As per the above, should this also be fallible? I assume that might be the case when the custom kind is not a Gate?
There was a problem hiding this comment.
Yes, which is why instead of returning zero on a non-controlled gate, we would return None instead. This might change whenever we implement result handling for these methods.
| extras_key: 0, | ||
| num_ctrl_qubits, | ||
| ctrl_state: (1 << num_ctrl_qubits) - 1, // default control state: all 1s | ||
| gate_class_name: Default::default(), // TODO: Use a class name. |
There was a problem hiding this comment.
Should this TODO be addressed?
There was a problem hiding this comment.
It should be addressed by #15723 since there are no Python class names existing here. Although, I could still call py_type instead since the method does exist even if it defaults to an Err as #15723 fixes it by default.
Edit: I've decided to leave it up to the subsequent PR to remove the TODO so we don't add an unnecessary py token.
| // TODO: Separate the handling of these two as gate also contains matrix definiton | ||
| // And we don't currently serialize gate matrices. |
There was a problem hiding this comment.
Another TODO which might need to be addressed?
There was a problem hiding this comment.
That actually might be something for QPY to fix? It seems QPY prioritizes serializing definition over matrix for any operation. Do you have any opinion on this matter? @gadial
The `CustomOp` struct acts as a container to the `Box<dyn CustomOperation>` and its presence outisde of this crate seems to be a little redundant. The following commit reduces the visibility of this struct to only the `circuit` crate, which makes it easier to process a custom operation without the added layer of wrapping it within this struct.
mrossinek
left a comment
There was a problem hiding this comment.
Thanks for working on all my comments above! 👍
Approving this pro-forma although you will still need a review from a core team member.
Cryoris
left a comment
There was a problem hiding this comment.
I finished a first pass and the approach here LGTM, I left some comments on the implementation below, e.g. I think that the QPY changes could be reduced a bit and generally we could be a bit more consistent with existing flows.
Since this new type is modelled closely following the interfaces of StandardInstruction and StandardGate, and less the one of Operation, it seems that CustomInstruction would be a more descriptive name. What do you think?
Also I was wondering if we actually wanted the full StandardInstruction interface. E.g. we might take the chance to properly handle controlled operations via annotations instead of proliferating the current funky mix 🙂
| /// Describes the kind of operation associated with this type. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| #[repr(u8)] | ||
| pub enum CustomOperationKind { |
There was a problem hiding this comment.
Wouldn't it make more sense to just have the CustomOperation implement an is_gate() -> bool function instead of implementing this struct? It seems a bit overkill given that we don't have any other kinds planned here 🤔
There was a problem hiding this comment.
I can always revert this to use is_gate() but in my mind having a more descriptive variant-based system made sense here. We can always add an is_gate() method that does the matching for you but I would agree on it being a little redundant.
There was a problem hiding this comment.
If there are other variants we want to use, I agree -- but are there any? 🙂
| /// Casts a reference to a CustomOperation to its original type if the correct | ||
| /// type is specified. | ||
| pub fn downcast_ref<T: CustomOperation + 'static>(&self) -> Option<&T> { | ||
| (self.type_id() == TypeId::of::<T>()).then(|| unsafe { &*(self as *const _ as *const T) }) |
There was a problem hiding this comment.
Why can't we just do a single as *const T here, but require casting twice?
There was a problem hiding this comment.
The first cast causes it to become a *const dyn CustomOperation which then gets cast to a pointer of the suggested type T, then we turn it into a reference by referencing the dereferenced pointer (which is like an unsafe version of ::as_ref but since we checked that the types match before it shouldn't fail.
An easier way of performing this cast would be to use the Any trait which these objects implement based on the SuperTrait, however, we can't upcast the dyn as it wasn't something we were able to do safely until Rust 1.86 (which is not our MSRV). So until we update to 1.86+ this is the only way of doing it.
There was a problem hiding this comment.
You may add a SAFETY comment
| #[repr(align(8))] | ||
| pub(crate) struct CustomOp(Box<dyn CustomOperation>); | ||
|
|
||
| impl Deref for CustomOp { |
There was a problem hiding this comment.
Reading the code it was a little unexpected that dereferencing the wrapper CustomOp gave a CustomOperation -- to me this blurs the line between the two objects and makes it a bit harder to understand the distinction. An alternative would be to explicitly add a method that returns a reference to the inner CustomOperation, like
impl CustomOp {
fn inner_as_ref(&self) { self.0.as_ref() };
}which would require being more explicit in 1 or 2 places where Rust automatically derefences.
Both are valid approaches, though personally I'd be a bit in favor of being explicit and more readable. But of course I'm happy to be convinced otherwise 🙂
There was a problem hiding this comment.
CustomOp in this case is never really meant to be accessed outside of Circuit since it is just a wrapper. So using Deref or DerefMut shouldn't be a negative here, there's a reason why these traits exist.
If there was any additional data or functionality being lost here from exposing the inner reference using inner_as_ref I would've exposed a view. However, it just looks like it adds an extra step that doesn't change much.
| } | ||
| OperationRef::Gate(gate) => gate.matrix_as_static_1q(), | ||
| OperationRef::Unitary(unitary) => unitary.matrix_as_static_1q(), | ||
| OperationRef::CustomOperation(g) => { |
There was a problem hiding this comment.
Would it make more sense to just implement matrix_as_static_1q on CustomOperation?
There was a problem hiding this comment.
I'm a little torn on whether to include this. On one hand it would be good to obtain an owned version of the inner matrix of the gate in question. However, not all operations have static matrices. It is, though, a method that is available for all StandardGate instances.
If you feel strongly about it and have a stronger case than mine for including it other than because it is required here, then I wouldn't oppose it.
There was a problem hiding this comment.
No strong opinion here, as long as this is the only place where it's necessary I'm also fine hardcoding it. Thought if we're already spelling it out we might as well add it as method 😄
| fn pack_instruction( | ||
| instruction: &PackedInstruction, | ||
| custom_operations: &mut HashMap<String, PackedOperation>, | ||
| custom_operations: &mut HashMap<String, PackedInstruction>, |
There was a problem hiding this comment.
I believe I changed it because of the modifications for pack_custom_instruction as it needs to keep track of the parameters so they can be packed later on. You could still obtain them for custom operations in Python because they'd keep a pointer to their Python instance, which keeps ownership of its parameter. However, for a CustomGate in Rust, which doesn't own its parameters, it needs the specific instances as they'd affect the way they are defined in the end.
| params: &[Param], | ||
| qpy_data: &mut QPYWriteData, | ||
| ) -> PyResult<formats::CircuitInstructionV2Pack> { | ||
| let params = pack_instruction_params(params, qpy_data)?; |
There was a problem hiding this comment.
A more conservative change here would be to leave pack_instruction_params as is and just pass the PackedInstruction by reference into this function here. That's also how all other cases seem to be handled.
There was a problem hiding this comment.
Yeah, I can't remember why I changed this. It is a much better use for it as you only send in what you need. But it does become a bit more invasive. I'll try changing the function back to its original state but I also think it's a sensible change.
There was a problem hiding this comment.
Update: I left it this way to avoid having to match the CustomOperation once more.
| } | ||
| // TODO: Separate the handling of these two as gate also contains matrix definiton | ||
| // And we don't currently serialize gate matrices. | ||
| OperationRef::CustomOperation(gate) => { |
There was a problem hiding this comment.
Don't we only need this once we have CustomOperations in Python? If yes, this should go into the other PR 🙂
There was a problem hiding this comment.
AFAIK, when a Custom Gate is found it creates a Gate or Instruction in Python and it assigns the definition retrieved. So I think this should still be valid, but feel free to correct me if I'm wrong.
| placeholder_params = extract_py.params_view().iter().cloned().collect(); | ||
| extract_py.operation | ||
| }; | ||
| let gate = PackedOperation::from_custom_operation(CustomDummy { |
There was a problem hiding this comment.
How does this change impact the BT result? Previously we were getting the exact type from the name (e.g. "x" would actually have been a StandardGate::X packed op, right?) whereas now everything is a custom operation.
There was a problem hiding this comment.
This is the way the original BT pass did it in Python, where we would insert a placeholder operation with the same name and attributes (mainly num_qubits and parameters). But whatever the gate held inside does not matter as it gets filtered out further along in the process. Mapping to its original type here, although a cool way of getting things done, might end up taking more space in the case that we find an operation that is bigger than a StandardGate or any of the special ops. In the case we get a unitary gate, which in most cases we shouldn't, if the unitary is big enough, it ends up being heavier than just using a placeholder gate like we do here.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
it's great that tests were added here. perhaps it's worth to add more tests for gates defined using a matrix? for controlled gates? define some non-unitary custom operation?
| Ok(formats::CircuitInstructionV2Pack { | ||
| num_qargs: op.num_qubits(), | ||
| num_cargs: op.num_clbits(), | ||
| extras_key: 0, |
There was a problem hiding this comment.
what is extras_key ? perhaps it's worth to add a comment
There was a problem hiding this comment.
I think that slot is reserved to denote that annotations/conditions are present in an instruction. Which I don't think we use here yet
There was a problem hiding this comment.
so why is it needed in this PR? maybe it should be added in a follow-up?
There was a problem hiding this comment.
QPY serialization? Well, it is mainly because it matches on each PackedOperation instance view.
At the end of the day whenever we serialize a custom operation we can QPY just calls on Gate or Instruction and assigns whatever definition it finds. So I think this is a valid addition here.
There was a problem hiding this comment.
I meant the extras_key parameter? why do you need it here? at least add some comment explaining that it will be used later.
There was a problem hiding this comment.
Oh, I see the confusion. When I mentioned that we don't use it, I failed to mention that what I put there is the "default" value noting that we don't really have annotations/conditions present here. I can add the comment for clarity.
- Rename `CustomOp` to `BoxedCustomOperation`. - Remove `from_custom_operation` function. - Rename `from_boxed_custom_operation` to `from_custom_operation`. - Add comment to `extras_keys` to explain use of default value.
| ) -> PyResult<Bound<'py, PyAny>> { | ||
| Err(PyNotImplementedError::new_err( | ||
| "Rust native operations cannot be exposed to Python yet.", | ||
| )) |
There was a problem hiding this comment.
The defaults on definition, matrix, label etc. are fine — None is a legitimate "not supported" value. But create_py_op and py_type default to Err(NotImplementedError), which doesn't look like a sensible fallback — it can lead to silent deferred crash. These two can be made required methods so the compiler catches missing implementations instead of Python raising a NotImplementedError at runtime.
There was a problem hiding this comment.
The only context we would ever need to call on these methods (being py_type or create_py_op) is if Python were to ever receive a Rust native operation or Rust had a python interpreter attached via PyO3.
Since this is a feature that has genuinely not been implemented (until #15723) we have to give some reasonable fallback. In this case panicking would be a bit too restrictive since it is an unrecoverable error and returning an Option object does not make much sense for the future plans (as we plan on returning a view to Python just like we do with Python operations retrieved from Rust). However, returning a Result with a Python "Not implemented" exception makes perfect sense since:
- If the user calls on the method, while attached to Python, it falls onto their grasp to process the result accordingly, hence if the
Errvariant is returned, which is currently is always the case until this gets finalized, the user is responsible for handling that. - If the user calls on this method without being attached to Python (which should be impossible given that you do require a
pytoken), then the method would panic. But so would anything else that uses PyO3 in this context.
There was a problem hiding this comment.
The concern in my mind wasn't about the return type — it's whether these methods should have a default implementation at all.
- Expand docstring for `BoxedCustomOperation`. - Rename unwrapping of `CustomOperation` as `operation` not `gate`.
Summary
CustomOperationtrait that allows users to implement methods available in bothInstructions andGates.CustomOperationwithin aPackedOperationcalledCustomOp.BasisTranslator'scompose_transformsfunction where a dummy gate calledCustomDummyis added to eachDAGCircuitinstance natively.Breakdown
This PR adds a pathway to implement custom operations in Rust. Here's a breakdown as to how it works.
Implementing a gate with its own information fields
Let's say we want to introduce a Gate called FXGate, the FXGate has some specific data that it needs to store as well, so we create a struct in Rust of this manner.
Implementing
OperationandCustomOperationFor
FXGateto work as an actual operation within the circuit, it needs to implementCustomOperation, which itself is a subtrait of the existentOperationtrait.CustomOperationcontains the inherent methods that are available inStandardGateandStandardInstruction. All implementors must correctly define the following methods:CustomOperation::kind: defines whether the operation itself is aGateor anInstruction.CustomOperation::clone_dyn: Allows a user to clone the operation when it gets dynamically dispatched.An example implementation of this would be the following.
Adding our gate to the circuit as a
CustomOpAs it stands,
FXGatecan now be added directly to a circuit, but it first needs to be dynamically dispatched. A way of doing this is to useCustomOpwhich will consume the instance and enclose it within aBoxpointer as adyn CustomOperation.In this way you have correctly inserted your gate into the circuit and can be identified by name alone. However, how can a user retrieve the original instance?
Downcasting from
Box<dyn CustomOperation>or&dyn CustomOperation>Any implementor of
CustomOperationinherits the methoddowncast_refwhich allows the user to retrieve aCustomOpor a reference to adyn CustomOperationand downcast it to a reference of its original instance if it is correct. For example:This operation is fallible so it will not panic if the incorrect type is passed.
Notes:
Instructionwhich is incorrect and is being explored.Details and comments