Enable rust native Target creation.
#14228
Conversation
Pull Request Test Coverage Report for Build 14835963529Details
💛 - Coveralls |
a60684e to
ee11c02
Compare
- Add rust native way of adding an instruction to the `Target` without any need for Python. - Additions are guaranteed to be reflected in Rust and Python.
ee11c02 to
9fadda4
Compare
- Remove `PyResult` from the output of the rust native functions. - Add quotation marks to the output of error messages for gate names. - Add rust native unit testing for the new methods to make sure they're working properly.
Target creation.Target creation.
|
One or more of the following people are relevant to this code:
|
- Use rustdoc format for docstrings - Removce generic type for property map in `add_instruction`. - Add `QargsRef::as_owned_qargs` to return an owned version of Qargs.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
- Remove `as_owned_qargs`. - Use `String` instead of owned `Qargs` for `TargetError`.
| &mut self, | ||
| instruction: &'a str, | ||
| qargs: T, | ||
| properties: Option<InstructionProperties>, |
There was a problem hiding this comment.
Do we need the Option here? Like I realize the data model supports None to indicate an ideal instruction with no properties, but I wonder if in practice this is something we should be supporting on an update method. The python space typing indicated None wasn't an allowed type only InstructionProperties.
There was a problem hiding this comment.
That's a good question. Getting rid of the option seems logical. However, what if the user were to update the properties of a global/variadic instruction temporarily, and then back to its original value None? What could the user do in this case and is this supported behavior?
There was a problem hiding this comment.
I'm fine leaving it. I don't actually think that's a thing people will do in practice. The handling of globals is ill defined especially with properties set and it's not commonly used at all. That was not part of the original design intent of the target, and the fact it was possible was an implementation mistake that leaves us with undefined behavior. This is why I asked you to open #14268 so we can fix this hole in the API for 3.0 (regardless of what we do it will probably be backwards incompatible).
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
The following commits expose rust native methods
add_instructionandupdate_instruction_properties, opposite to their Python counterparts.Details and comments
Now that we're able to build instructions efficiently from Rust, we should be able to add instructions to the
Target(or the python'sBaseTarget). The following commits add the following Rust-native methods:add_instructionAdds an instance of
PackedOperationto theTarget.An addition to the Target from Python typically involves using an instance of
TargetOperation, which has two variants representing variadics and regular instructions. However, since we don't yet supportVariadicsin Rust, this method is only able to work withNormalOperationand therefore accepts the Rust native typesPackedOperationand a slice ofParam(at least until #13267 is resolved).update_instruction_propertiesModifies the
InstructionPropertiesassigned of a collection ofqargsfor a certain instruction in theTarget. This should be the only way of modifying instruction properties, at least until we can figure out a way of synchronizing the Python and Rust gate maps (see #14173).These additions rely on the additions described below:
Pre-requisites
OnceLockto cache aNormalOperationinTarget#13995Qargsimplementation forTarget#14210