Add methods to more easily add operations to a DAGCircuit from Rust

[raynelfss]

What should we add?

The problem:

We crurrently have to methods that allow us to add an operation to the DAGCircuit from rust.

• DAGCircuit::push_back and DAGCircuit::push_front which accepts a PackedInstruction instance as an argument and must have valid interned qargs and cargs.
• DAGCircuit::py_apply_operation_back and DAGCircuit::py_apply_operation_front which performs the addition of instructions in a similar way to what was done before, however this is only with python types and is not exposed to the rest of the rust api.

In each case we're either required to re-intern the qargs/cargs, or have an already valid instance of PackedInstruction that could work in this circuit, which is what I ultimately did for #13032. But now that we are building transpiler passes in Rust alone, having to own mutable access to the DAGCircuit and inserting qargs directly seems very unsafe.

The solution

Adding rust-native apply_operation_back and apply_operation_front to DAGCircuit that is exposed to the rest of the crates.

Having said methods would allow us to:

• Insert a PackedOperation of any type and insert qargs and cargs by index (Qubit or Clbit as defined by the circuit crate), since that's how it is registered in the DAGCircuit.
• Avoid having mutable access to private methods in the DAGCircuit, such as the interners or BitData.
• Overall make it easier to modify and create DAGCircuit instances from transpiler passes that need it, such as the BasisTranslator.

This shouldn't be too hard to implement and would be of great help to all of the developers currently working on transpiler passes.

Discussion

Feel free to start an educated discussion in the comments, I'd love to hear what the rest of the team thinks about this.

Tasks

Beta Give feedback

1. Move Option<Box<_>> into ExtraInstructionAttributes. #13127
   Changelog: None +1
2. Add rust-native apply_operation methods #13143
   Changelog: None Rust +2

[kevinhartman]

On the ElidePermutations PR linked above, we started dicussing the need and possible signature for a Rust-native apply_operation_back.

From #13094 (comment):

I think the signature for this should be something like:

fn apply_operation_back(
    op: PackedOperation, 
    qargs: &[Qubit], 
    cargs: &[Clbit], 
    params: Option<&[Param]>,
    extra_attrs: Option<&ExtraInstructionAttributes>,
) -> NodeIndex

Note that I used the name apply_operation_back (I called the Python version py_apply_operation_back with anticipation of this Rust-oriented version).

I think the py_op cache will get updated on demand within the instruction, so perhaps we can get away with keeping it out of the interface here. Maybe @jakelishman has thoughts?

I think we're ready to implement something like this. It didn't exist originally only because we didn't need it when porting DAGCircuit (since it only deals with its own PackedInstructions, and thus could use the lower-level DAGCircuit::push_back that came along with that PR).

[jakelishman]

I'm fine with the method as described by Kevin, but I also think there ought to be ways to push operations back while passing an already-known Interned key. As in Sasha's case, there's plenty of cases where we need to pass stuff in where we can know that the interned key is safe, and looking up and then re-hashing the &[Clbit] or &[Qubit] would be a waste of cycles.

Personally I have no problem with exposing a method to allow manual insertions into the interners as well - I don't think it should be the standard way to do that, but you can't break data coherence by doing it, since interners are append-only, and adding the key doesn't invalidate existing keys. That said, I don't immediately have uses for that in mind - I have a rough imagined optimisation to the Sabre post-routing rebuilder that pre-allocates the interner, but no proof it'd be a good idea - so I'm not pushing for adding them.

[jakelishman]

Oh, fwiw, we probably want to think about the transfer of ownership of various objects as well - I don't know what patterns are going to be most useful, but we possibly want to make it so we don't need unnecessary heap allocations when the caller already has owned variants, etc.

[kevinhartman]

we probably want to think about the transfer of ownership of various objects as well - I don't know what patterns are going to be most useful, but we possibly want to make it so we don't need unnecessary heap allocations when the caller already has owned variants, etc.

I'd think it probably makes sense to accept the bits as slices (e.g. &[Qubit]) since the sequence may already exist within the interner. The ExtraInstructionAttributes should go by value because the PackedInstruction we build inside DAGCircuit::apply_operation_back will always need ownership. That is true for the SmallVec<[Param; 3]> as well.

So perhaps:

fn apply_operation_back(
    op: PackedOperation, 
    qargs: &[Qubit], 
    cargs: &[Clbit], 
    params: Option<SmallVec<[Param; 3]>>,
    extra_attrs: Option<ExtraInstructionAttributes>,
) -> NodeIndex { ... }

This way the user can move the by-value stuff in if they don't need it, or clone it explicitly and pass the clones, which apply_operation_back would otherwise need to do anyhow.

[jakelishman]

Right, most of the time the keys will already exist for sure. If the caller is typically dealing with varying-sized gates, though, they might be collecting them into a Vec on their end, in which case we might want something that's effectively an exposure of Interner::insert_owned in addition to Interner::insert. Fwiw, most current DAGCircuit functions use insert_owned, though hopefully that'll change as we reduce the Python dependence.

[raynelfss]

About ExtraInstructionAttributes would we want to receive an already Boxed instance or let the function itself perform the boxing? We will mostly receive these from a Python extraction (Be it from CircuitInstruction, PackedInstruction, or OperationfromPython all of which will already have performed the Boxing of said attributes.

Were we to accept Boxed instances, it would give these methods a little bit of flexibility allowing them to be re-used in the Python-exposed variants of themselves. What do you think? @jakelishman @kevinhartman

[kevinhartman]

@raynelfss

IMO that is another hint that merging #13127 would be helpful. There aren't really any cases where we want the inner value type of this struct to exist outside of a Box, and in that PR, the nuance of that is encapsulated away from the user.

[raynelfss]

Sounds good, #13127 will be considered a blocker for this. I will also review said PR.