Port `InstructionDurationCheck` to Rust #14700

phalakbh · 2025-07-07T17:54:28Z

Summary

This PR ports the InstructionDurationCheck analysis pass from Python to Rust for improved performance and integration with the Qiskit transpiler. No changes to pass logic or user-facing behavior are introduced.

Details and comments

Fixes Port InstructionDurationCheck to Rust #12267
As part of this port, I had to make the num_stretches method in DAGCircuit public to allow access from the new Rust pass.

qiskit-bot · 2025-07-07T17:54:33Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

@Qiskit/terra-core

coveralls · 2025-07-07T18:27:00Z

Pull Request Test Coverage Report for Build 16472913654

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

18 of 42 (42.86%) changed or added relevant lines in 4 files are covered.
1482 unchanged lines in 33 files lost coverage.
Overall coverage decreased (-0.03%) to 87.752%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/transpiler/src/passes/instruction_duration_check.rs	14	38	36.84%

Files with Coverage Reduction	New Missed Lines	%
crates/synthesis/src/linear_phase/mod.rs	1	90.91%
crates/circuit/src/converters.rs	2	97.33%
crates/synthesis/src/multi_controlled/mcmt.rs	2	89.19%
crates/transpiler/src/passes/basis_translator/compose_transforms.rs	2	96.61%
qiskit/transpiler/passes/optimization/commutative_inverse_cancellation.py	2	97.26%
crates/circuit/src/circuit_instruction.rs	3	88.22%
crates/circuit/src/dag_node.rs	3	70.0%
crates/transpiler/src/passes/disjoint_layout.rs	3	89.04%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py	4	94.62%
qiskit/transpiler/passes/routing/sabre_swap.py	5	92.31%

Totals
Change from base Build 16123284064:	-0.03%
Covered Lines:	81491
Relevant Lines:	92865

💛 - Coveralls

mtreinish

Thanks for getting this started, this has the potential to speed up the pass quite a bit. I left a few inline comments on how to improve it a bit, I also think there is a bug in the pass that predates this PR, but we should fix it while we're porting this, it should be simple to do.

mtreinish · 2025-07-08T18:22:26Z

crates/transpiler/src/passes/instruction_duration_check.rs

+
+    //Check delay durations
+    for (_, packed_op) in dag.op_nodes(false) {
+        if packed_op.op.name() == "delay" {


It would be more efficient to do this check using the rust types directly rather than a string comparison. This should be something like:

Suggested change

if packed_op.op.name() == "delay" {

if let OperationRef::StandardInstruction(StandardInstruction::Delay(_unit)) = packed_op.op.view() {

I haven't tested this code exactly so it's worth validating the exact syntax locally. But this is the basic idea, is use the rust type instead of the string, as this ends up be 2 integer comparisons vs a string comparison and will be faster, especially since this is being run for ever node in the dag.

mtreinish · 2025-07-08T18:23:47Z

crates/transpiler/src/passes/instruction_duration_check.rs

+    for (_, packed_op) in dag.op_nodes(false) {
+        if packed_op.op.name() == "delay" {
+            let params = packed_op.params_view();
+            if let Some(param) = params.first() {


Can this ever be None? If not I think we should probably error because the DAGCircuit is invalid.

mtreinish · 2025-07-08T18:32:30Z

crates/transpiler/src/passes/instruction_duration_check.rs

+                    _ => continue,
+                };
+
+                if !(duration % acquire_align == 0 || duration % pulse_align == 0) {


FWIW % in rust isn't the same as % in python. In this case I don't think it matters but whenever I see this I always want to double check whether % is correct or whether I should be using rem_euclid instead. It's probably worth double checking.

Thanks for letting me know. I did check, both acquire_align and pulse_align seem to accept negative integer values (though I don't know if negative alignment values make sense here) so it is probably best to use rem_euclid instead of %.

Update: Unlike what I said earlier, I think we can stick to using u32 types for the alignments and the % operator for alignment checks. While Python technically allows passing negative values for acquire_align and pulse_align, it won't be meaningful. Target's implementation too always returns unsigned integers for both acquire_align and pulse_align, so using u32 in Rust will continue to enforce this constraint.

mtreinish · 2025-07-08T18:35:55Z

crates/transpiler/src/passes/instruction_duration_check.rs

+                let duration = match param {
+                    Param::Float(val) => *val as u32,
+                    _ => continue,
+                };


Is this correct? I would expect Param::Obj when the unit is dt because we're storing an integer. This will skip that case and we won't validate the alignment.

Also, shouldn't we be validating the unit here, like if the delay is in units of ns the alignment in terms of dt being checked below doesn't work. I realize you faithfully ported this from Python, but it doesn't seem like the python logic was correct. If the unit is not in terms of dt we can't check the alignment and this pass won't be able to handle that delay.

I see, I will make the changes regarding Param::Obj. Just to clarify, should we skip on checking the alignment if the delay unit is anything other than dt, or should we convert the unit from seconds to dt first and then check the alignment?

This pass doesn't do unit conversion. There is a TimeUnitConversion transpiler pass that runs which is supposed to handle that: https://quantum.cloud.ibm.com/docs/en/api/qiskit/1.2/qiskit.transpiler.passes.TimeUnitConversion if it's not in units of dt either skipping or erroring is a appropriate. I might lean toward returning an error because the assumptions this pass works under aren't valid. But there might be backwards compatibility concerns with that, it's worth figuring out how the current python implementation behaves and work against that.

raynelfss

Good progress! I just have one comment (the rest are nitpicks)

raynelfss · 2025-07-08T19:07:48Z

crates/transpiler/src/passes/instruction_duration_check.rs

+) -> PyResult<bool> {
+    let num_stretches = dag.num_stretches();
+
+    //Rescheduling is not necessary


Minor nitpick: Add a whitespace after the // in the comment.

Suggested change

//Rescheduling is not necessary

// Rescheduling is not necessary

raynelfss · 2025-07-08T19:08:05Z

crates/transpiler/src/passes/instruction_duration_check.rs

+        return Ok(false);
+    }
+
+    //Check delay durations


See https://github.com/Qiskit/qiskit/pull/14700/files#r2193260197

Suggested change

//Check delay durations

// Check delay durations

raynelfss · 2025-07-08T19:45:36Z

crates/transpiler/src/passes/instruction_duration_check.rs

+                    Param::Float(val) => *val as u32,
+                    _ => continue,


Is this the correct way of handling this? IIRC a duration can either be a number or an Expr. So we know that brings the question of whether we can keep this rust native? I know Expr already exists in Rust but we don't yet use it within instructions.

…o accept unsigned integers

phalakbh · 2025-07-11T14:19:25Z

Thank you @mtreinish and @raynelfss for your comments! I have made the suggested changes. I think it is ready for another review, please let me know if there is anything else that I could improve upon.

raynelfss

Most of it looks good but I have some concerns about the dt check.

raynelfss · 2025-07-22T14:54:09Z

crates/transpiler/src/passes/instruction_duration_check.rs

+                return Err(TranspilerError::new_err(
+                    "Delay duration must have dt unit for checking alignment.",
+                ));
+            }


Is this correct? I know Matthew mentioned that throwing an error here might be the best option given how the pass works. Since the error didn't exist in Python, does it seem to break any of the existing pipelines? (Judging from the CI runs everything seems to run as expected, but just out of curiosity).

So far it doesn't break any of the pipelines. The TimeUnitConversion pass takes care of unifying the units before sending it to this pass. Since the alignment constraints are only meaningful when defined in dt and checking the alignment would only make sense if the unit of durations is also dt. If we are getting unit as seconds, I believe it is safe to return the error.

raynelfss · 2025-07-22T14:54:10Z

crates/transpiler/src/passes/instruction_duration_check.rs

+            let duration = match param {
+                Param::Float(val) => Some(*val as u32),
+                Param::Obj(val) | Param::ParameterExpression(val) => {
+                    val.bind(py).extract::<u32>().ok()
+                }


There are two things you can do about this match arm here depending on what you do about the comment above:

If what was mentioned above is true, and we don't operate on anything that's not on terms of dt, we should drop the Float and ParameterExpression arms since dt will always use int as a type.

If we will be removing the DT check and will continue either way instead, we should drop only the ParameterExpression arm.
If any of the dropped cases is found, either erroring or skipping should be expected. I'm leaning more towards erroring so the user knows something is wrong.

Done in f364f35!

…object

raynelfss

This is about ready to merge. Just one comment about the new error message.

raynelfss · 2025-07-23T13:48:19Z

crates/transpiler/src/passes/instruction_duration_check.rs

+            let duration = match param {
+                Param::Obj(val) => val.bind(py).extract::<u32>(),
+                _ => Err(TranspilerError::new_err(
+                    "Delay instruction parameter is not an object.",


Suggested change

"Delay instruction parameter is not an object.",

"The provided Delay duration is not in terms of dt.",

* Add rust-native instruction duration check in transpiler * Make num_stretches method public in DAGCircuit implementation * Improve error handling and parameter validation * Enforce dt unit for delay durations and update alignment parameters to accept unsigned integers * Modify match arm to return error when instruction paramter is not an object * Correct the error message for when duration is not in dt

phalakbh added 3 commits July 7, 2025 21:53

Add rust-native instruction duration check in transpiler

24b382f

Make num_stretches method public in DAGCircuit implementation

ef37ce6

Merge branch 'Qiskit:main' into port-instruction-duration-check

74aa641

phalakbh requested a review from a team as a code owner July 7, 2025 17:54

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jul 7, 2025

github-project-automation bot added this to Contributor Monitoring Jul 7, 2025

raynelfss added this to the 2.2.0 milestone Jul 7, 2025

raynelfss self-assigned this Jul 7, 2025

mtreinish reviewed Jul 8, 2025

View reviewed changes

raynelfss reviewed Jul 8, 2025

View reviewed changes

phalakbh added 2 commits July 9, 2025 04:16

Improve error handling and parameter validation

1a423d1

Enforce dt unit for delay durations and update alignment parameters t…

71ed25d

…o accept unsigned integers

phalakbh requested review from mtreinish and raynelfss July 11, 2025 19:51

raynelfss reviewed Jul 22, 2025

View reviewed changes

Modify match arm to return error when instruction paramter is not an …

f364f35

…object

raynelfss reviewed Jul 23, 2025

View reviewed changes

Correct the error message for when duration is not in dt

22a419d

raynelfss approved these changes Jul 23, 2025

View reviewed changes

raynelfss added this pull request to the merge queue Jul 23, 2025

Merged via the queue into Qiskit:main with commit 2877bf0 Jul 23, 2025
26 checks passed

github-project-automation bot moved this to Done in Contributor Monitoring Jul 23, 2025

	if packed_op.op.name() == "delay" {
	if let OperationRef::StandardInstruction(StandardInstruction::Delay(_unit)) = packed_op.op.view() {

	//Rescheduling is not necessary
	// Rescheduling is not necessary

	"Delay instruction parameter is not an object.",
	"The provided Delay duration is not in terms of dt.",

Port InstructionDurationCheck to Rust #14700

Port InstructionDurationCheck to Rust #14700

Uh oh!

Conversation

phalakbh commented Jul 7, 2025

Summary

Details and comments

Uh oh!

qiskit-bot commented Jul 7, 2025

Uh oh!

coveralls commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16472913654

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

mtreinish 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!

phalakbh Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

raynelfss 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!

phalakbh commented Jul 11, 2025

Uh oh!

raynelfss 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!

raynelfss 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!

Uh oh!

Uh oh!

Port `InstructionDurationCheck` to Rust #14700

Port `InstructionDurationCheck` to Rust #14700

coveralls commented Jul 7, 2025 •

edited

Loading

phalakbh Jul 8, 2025 •

edited

Loading