Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add target support to basis, optimization, scheduling, and util passes #9343

Merged
merged 10 commits into from
Jan 20, 2023

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Jan 4, 2023

Summary

This commit updates all the basis, optimization, and util passes which leverage hardware constraints to have a new optional keyword argument, target, to specify a Target object for modelling the constraints of the target backend. This option will supersede any other specified arguments for backend constraints.

With this and #9263 all the passes which take in backend constraints are made target aware. Making all the passes target aware is the first step towards making all backend constraints for the transpiler used the Target internally (see #9256). Once all passes that take hardware constraints parameters as an input are updated to have a target we can start internally using the Target only for all preset pass manager construction and start the long process of deprecating the legacy interface in these passes.

Details and comments

This commit updates all the basis, optimization, and util passes which
leverage hardware constraints to have a new optional keyword argument,
target, to specify a Target object for modelling the constraints of the
target backend. This option will superscede any other specified arguments
for backend constraints.

With this and Qiskit#9263 all the passes except for the scheduling passes
which take in backend constraints are made target aware. Making all the
passes target aware is the first step towards making all backend
constraints for the transpiler used the Target internally (see Qiskit#9256).
Once all passes that take hardware constraints parameters as an input are
updated to have a target we can start internally using the Target only for
all preset pass manager construction and start the long process of
deprecating the legacy interface in these passes.
@mtreinish mtreinish requested review from a team and nonhermitian as code owners January 4, 2023 21:21
@qiskit-bot
Copy link
Collaborator

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 the following people are requested to review this:

@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Jan 4, 2023
@mtreinish mtreinish added this to the 0.23.0 milestone Jan 4, 2023
@mtreinish mtreinish added the mod: transpiler Issues and PRs related to Transpiler label Jan 4, 2023
@coveralls
Copy link

coveralls commented Jan 4, 2023

Pull Request Test Coverage Report for Build 3970101828

  • 87 of 109 (79.82%) changed or added relevant lines in 16 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 84.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/calibration/rzx_builder.py 2 3 66.67%
qiskit/transpiler/passes/optimization/echo_rzx_weyl_decomposition.py 2 3 66.67%
qiskit/transpiler/passes/scheduling/base_scheduler.py 2 3 66.67%
qiskit/transpiler/passes/scheduling/padding/dynamical_decoupling.py 4 5 80.0%
qiskit/transpiler/passes/utils/check_cx_direction.py 1 2 50.0%
qiskit/transpiler/passes/optimization/crosstalk_adaptive_schedule.py 2 4 50.0%
qiskit/transpiler/passes/scheduling/dynamical_decoupling.py 1 3 33.33%
qiskit/transpiler/passes/basis/unroller.py 22 28 78.57%
qiskit/transpiler/passes/optimization/optimize_1q_gates.py 25 32 78.13%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/utils/check_cx_direction.py 1 75.0%
src/vf2_layout.rs 1 86.44%
Totals Coverage Status
Change from base Build 3970100474: -0.01%
Covered Lines: 66705
Relevant Lines: 78547

💛 - Coveralls

@mtreinish mtreinish changed the title Add target support to basis, optimization, and util passes Add target support to basis, optimization, scheduling, and util passes Jan 5, 2023
@@ -90,6 +95,8 @@ def __init__(

self._inst_map = instruction_schedule_map
self._verbose = verbose
if target:
self._inst_map = target.instruction_schedule_map()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively you can store target instead of the legacy instmap if available, or I can do it in follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking we could optimize some of these passes to work natively with a target in the future. For this PR just having the target option is sufficient I think because my goal for 0.24.0 is to update all the preset pass managers to work solely with a target so we can start to unify the transpiler passes to only need a target in the future.

Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is fine, perhaps some tests to flex the new target paths would be good (or modify some of the old tests to run with target instead of say basis_gates).

However testing this in my own environment I found that target.durations() does not handle the case of different params having different durations. For example RX(pi/2) and RX(pi/6) can have different durations in the target, but the conversion just makes an InstructionDurations object with entries rx_90 and rx_30, which masks that these are actually the same operation, just different params.

The InstructionDurations class is actually able to represent the case of different params having different durations, after #7321. So can you make a test in this PR that is able to for example schedule a circuit containing RX(pi/2) and RX(pi/6) using the ALAPSchedule pass on a Target that specifies the durations of those instructions?

@mtreinish
Copy link
Member Author

I think this PR is fine, perhaps some tests to flex the new target paths would be good (or modify some of the old tests to run with target instead of say basis_gates).

I've added some test coverage in: b1e2024 and 5fed919 I can add more if you want more in depth coverage (or if I missed any passes)

However testing this in my own environment I found that target.durations() does not handle the case of different params having different durations. For example RX(pi/2) and RX(pi/6) can have different durations in the target, but the conversion just makes an InstructionDurations object with entries rx_90 and rx_30, which masks that these are actually the same operation, just different params.

The InstructionDurations class is actually able to represent the case of different params having different durations, after #7321. So can you make a test in this PR that is able to for example schedule a circuit containing RX(pi/2) and RX(pi/6) using the ALAPSchedule pass on a Target that specifies the durations of those instructions?

I looked at this it's a bug in the Target.durations() method which generates an InstructionDurations object from a Target. Basically the target needs to figured out when we have a fixed angle version of a gate to pass that in the format InstructionDurations expects. Right now it's adding a new name to the durations object for each parameter value. I think it should be fixed in a standalone PR though (which we can do post rc1 for 0.23.0 final) since it's self contained and logically independent from this change. I'll work on this in parallel and push up a PR for it.

@mtreinish mtreinish modified the milestones: 0.23.0, 0.24.0 Jan 19, 2023
Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I agree with a standalone PR to resolve the issue in target.durations() and then adding a test to make sure we can schedule on targets with parameterized gates of different durations.

@mergify mergify bot merged commit f061be8 into Qiskit:main Jan 20, 2023
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Jan 23, 2023
In the recently merged Qiskit#9343 support for using the Target directly was
added to the RZXCalibrartionBuilder family of passes. However, test
coverage in that PR was omitted by mistake for that pass (other passes
in the PR were covered). There was a bug in the code change introduced
in that PR which caused an error on initialization if a target were
specified without an InstructionScheduleMap (which was the intent of the
new target kwarg). This commit fixes the logic bug in the pass and adds
test coverage to ensure specifying only a target continues to work
moving forward.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Jan 23, 2023
In the recently merged Qiskit#9343 support for using the Target directly was
added to the RZXCalibrartionBuilder family of passes. However, test
coverage in that PR was omitted by mistake for that pass (other passes
in the PR were covered). There was a bug in the code change introduced
in that PR which caused an error on initialization if a target were
specified without an InstructionScheduleMap (which was the intent of the
new target kwarg). This commit fixes the logic bug in the pass and adds
test coverage to ensure specifying only a target continues to work
moving forward.
mergify bot added a commit that referenced this pull request Jan 23, 2023
In the recently merged #9343 support for using the Target directly was
added to the RZXCalibrartionBuilder family of passes. However, test
coverage in that PR was omitted by mistake for that pass (other passes
in the PR were covered). There was a bug in the code change introduced
in that PR which caused an error on initialization if a target were
specified without an InstructionScheduleMap (which was the intent of the
new target kwarg). This commit fixes the logic bug in the pass and adds
test coverage to ensure specifying only a target continues to work
moving forward.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@mtreinish mtreinish deleted the more-target-transpiler branch August 10, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants