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

Fix custom constraints in transpile with BackendV2 #12042

Merged
merged 10 commits into from
Mar 26, 2024

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Mar 19, 2024

Summary

This PR proposes a fix for the issue raised in #11994, where custom scheduling constraints would be ignored when a BackendV2 backend was provided to transpile. This happened because the underlying passes prioritize the information encoded in the target over user-provided information, which would set a clear order of priorities with BackendV1: custom target constraints>custom input constraints>backend constraints, but a not so clear one with BackendV2, where custom target constraints and backend target constraints would be treated both as "target constraints", leading to the backend constraints always overriding the custom input ones.

The proposed solution is to extend the approach already used for custom coupling_maps and basis_gates: setting the _skip_target flag to True in these cases. This way, the priority order already established for BackendV1 inputs will be maintained for target-based backends:

custom target > custom individual constraints > backend.target

Details and comments

  • The unit tests that ensure that the outputs of transpile are the same for v1 and v2 backends are in test_transpiler.py, which I believe makes sense, because this PR is at the intersection of the scheduling passes and the transpiler interface.
  • I have labelled this PR as a bugfix because I believe this discrepancy betweeen backend v1 and v2 was mostly an oversight, and users shouldn't expect the output /behavior of transpile to change when transitioning to v2, but let me know if you disagree.
  • This PR sets the stage for Update transpile() to convert BackendV1 inputs to BackendV2 with BackendV2Converter #11996

@ElePT ElePT requested a review from a team as a code owner March 19, 2024 17:18
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Mar 19, 2024

Pull Request Test Coverage Report for Build 8418045790

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.

Details

  • 14 of 14 (100.0%) changed or added relevant lines in 4 files are covered.
  • 204 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.05%) to 89.393%

Files with Coverage Reduction New Missed Lines %
qiskit/dagcircuit/collect_blocks.py 3 98.32%
qiskit/circuit/library/generalized_gates/rv.py 4 85.19%
crates/qasm2/src/lex.rs 7 92.11%
qiskit/dagcircuit/dagdepnode.py 8 78.26%
qiskit/pulse/parameter_manager.py 9 94.51%
crates/qasm2/src/parse.rs 12 97.15%
qiskit/dagcircuit/dagnode.py 14 90.07%
qiskit/dagcircuit/dagdependency.py 18 90.31%
qiskit/pulse/schedule.py 56 87.64%
qiskit/dagcircuit/dagcircuit.py 73 90.91%
Totals Coverage Status
Change from base Build 8376070119: 0.05%
Covered Lines: 59879
Relevant Lines: 66984

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, just one small question inline.

qiskit/compiler/transpiler.py Show resolved Hide resolved
@ElePT
Copy link
Contributor Author

ElePT commented Mar 21, 2024

I have updated the PR to:

  1. Allow the passes that use timing_constraints to also accept target inputs -> this makes the behavior of timing_constraints coherent with the rest of custom input options, and allows the transpiler to actually read the target constraints in the scheduling passes when not provided manually.
  2. Include backend_properties and timing_constraints in the skip_target case, so that they all behave the same

After these changes the table looks like this:

vs target backendV1 backendV2
basis_gates target basis_gates basis_gates
coupling_map target coupling_map coupling_map
instruction_durations target instruction_durations instruction_durations
inst_map target inst_map inst_map
dt target dt dt
timing_constraints target timing_constraints timing_constraints
backend_properties target backend_properties backend_properties

To test the custom backend_properties I used a vf2_layout test, there might be more direct ways to do it, but this is the pass I knew used the backend properties in a way I could track.

@ElePT ElePT requested a review from mtreinish March 21, 2024 13:36
mtreinish
mtreinish previously approved these changes Mar 22, 2024
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM now, thanks for fixing this. Before I hit automerge though do you think we should put your table in the docstring? It might be a good idea to make the priority ranking of different combinations of arguments more explicit since it seems to confuse everyone frequently.

@mtreinish mtreinish added Changelog: New Feature Include in the "Added" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog labels Mar 22, 2024
@mtreinish mtreinish added this to the 1.1.0 milestone Mar 22, 2024
@mtreinish mtreinish added the mod: transpiler Issues and PRs related to Transpiler label Mar 22, 2024
@ElePT ElePT changed the title Fix custom scheduling constraints in transpile with BackendV2 Fix custom constraints in transpile with BackendV2 Mar 22, 2024
@ElePT
Copy link
Contributor Author

ElePT commented Mar 22, 2024

Added an explanation + table in 856efc4

@ElePT
Copy link
Contributor Author

ElePT commented Mar 25, 2024

I ended up adding one more sentence in 05a8240 explaining the table, because the formatting wasn't on my side (I wanted the left column to be in bold font but it looks like I cannot choose how the table is formatted, or I don't know how to).

@ElePT ElePT requested a review from mtreinish March 25, 2024 16:42
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for updating the docs. This will be a good test of our stability policy in 1.1.0 as we've described this as a bugfix because the code behavior was different from the documented API. The backend argument docs said:

If any other option is explicitly set (e.g., coupling_map), it
will override the backend's.

But let's see if any users complain about an API change, because the exact behavior if you call transpile(qc, backend_v2, dt=5.2) differs between 1.0.x and 1.1.x. I think in this case we're fine because we've documented the API stability policy as applying to our documented API surface and this was really a consistency issue between the handling of BackendV1 and BackendV2.

@mtreinish mtreinish added this pull request to the merge queue Mar 26, 2024
Merged via the queue into Qiskit:main with commit 090b2b1 Mar 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog 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.

4 participants