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

Update transpile() and generate_preset_pass_manager to convert loose input of constraints to a Target with Target.from_configuration() #12185

Merged
merged 18 commits into from
May 31, 2024

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Apr 16, 2024

Summary

This PR is a step towards a full target-based transpiler pipeline as specified in #9256. It builds up on #11996 and #12183 (should be merged first). The main difficulty of this PR is handling the edge cases found in the legacy model that don't translate well to the target-based model, documented below. For these edge cases, backwards compatibility in the near term is ensured with the "_skip_target" flag, but long term support would require in many cases changes in the target model. The cases are documented in detail for future reference.

Details and comments

Migration to target-based transpiler. Edge Cases.

1. No basis gates but a coupling map

  • Current Workaround: _skip_target
  • Examples:
  1. test_symmetric_coupling_map in test/python/transpiler/test_preset_passmanagers.py:

2. No coupling map but basis gates

  • Current Workaround: allow the Target to skip the coupling map build if num_qubits is None, instead of failing (here and here).
  • Examples:
  1. test_no_coupling_map in test/python/transpiler/test_preset_passmanagers.py:
  2. test_layout_3239 in test/python/transpiler/test_preset_passmanagers.py:
  3. test_unitary_is_preserved_if_in_basis in test/python/transpiler/test_preset_passmanagers.py:

3. No coupling map but basis gates

  • Current Workaround: _skip_target.
  • Examples:
  1. test_no_coupling_map_with_sabre in test/python/transpiler/test_preset_passmanagers.py
  2. test_no_basis_gates in test/python/transpiler/test_preset_passmanagers.py
  3. test_level0_keeps_reset in test/python/transpiler/test_preset_passmanagers.py:
  4. test_initial_layout_fully_connected_cm in test/python/transpiler/test_preset_passmanagers.py:
  5. test_partial_layout_fully_connected_cm in test/python/transpiler/test_preset_passmanagers.py:

Other Edge Cases

1. Unitary synthesis with parametrized gates

  • Description: Unitary synthesis has separate methods for when target is provided/not provided. When target is provided, it requires gate re-parametrization, but so far it only re-parametrizes RXX and RZX gates.

  • Workaround: This issue was a consequence of the previously proposed workaround. "Fixed" after implementation of _skip_target.

    (snippet from unitary_synthesis.py line 693)

     # 2q gates sent to 2q decomposers must not have any symbolic parameters.  The
     # gates must be convertable to a numeric matrix. If a basis gate supports an arbitrary
     # angle, we have to choose one angle (or more.)
     def _replace_parameterized_gate(op):
         if isinstance(op, RXXGate) and isinstance(op.params[0], Parameter):
           op = RXXGate(pi / 2)
         elif isinstance(op, RZXGate) and isinstance(op.params[0], Parameter):
           op = RZXGate(pi / 4)
         return op
  • Examples:

test_translation_method_synthesis in test/python/compiler/test_transpiler.py:

 out = transpile(
   qc,
   translation_method="synthesis",
   basis_gates=basis_gates,
   optimization_level=optimization_level,
   seed_transpiler=42,
   )

Output:

  File "/Users/ept/qiskit_workspace/qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py", line 869, in run
 decomposers2q = self._decomposer_2q_from_target(
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py", line 753, in _decomposer_2q_from_target
    supercontrolled_basis = {
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py", line 754, in <dictcomp>
    k: v for k, v in available_2q_basis.items() if is_supercontrolled(v)
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py", line 738, in is_supercontrolled
    operator = Operator(gate)
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/quantum_info/operators/operator.py", line 97, in __init__
    self._data = self._init_instruction(data).data
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/quantum_info/operators/operator.py", line 700, in _init_instruction
    return Operator(np.array(instruction, dtype=complex))
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/circuit/library/standard_gates/rx.py", line 125, in __array__
    cos = math.cos(self.params[0] / 2)
  File "/Users/ept/qiskit_workspace/qiskit/qiskit/circuit/parameterexpression.py", line 415, in __float__
    raise TypeError(
TypeError: ParameterExpression with unbound parameters (dict_keys([Parameter(ϴ)])) cannot be cast to a float.
  • Similar cases:

    • test_translate_ecr_basis in test/python/compiler/test_transpiler.py

         res = transpile(
             circuit,
             basis_gates=["u", "ecr"],
             optimization_level=optimization_level,
             seed_transpiler=42,
         )
    • test_unitary_is_preserved_if_basis_is_None in test/python/transpiler/test_preset_passmanagers.py:

      qc = QuantumCircuit(2)
      qc.unitary(random_unitary(4, seed=4242), [0, 1])
      qc.measure_all()
      result = transpile(
        qc, 
        basis_gates=None, 
        optimization_level=level)
    • test_unitary_is_preserved_if_basis_is_None_synthesis_transltion in test/python/transpiler/test_preset_passmanagers.py:

         qc = QuantumCircuit(2)
         qc.unitary(random_unitary(4, seed=42424242), [0, 1])
         qc.measure_all()
         result = transpile(
           qc, 
           basis_gates=None, 
           optimization_level=level, 
           translation_method="synthesis"
         )

2. Custom pulse gates

  • Description: Custom 2q+ pulse gates that are directional may be flipped during the layout step if gate errors are provided (i.e. backend properties), triggering errors in HighLevelSynthesis or GateDirection (as it doesn't know how to flip them back).

  • Workaround: _skip_target and not providing backend properties when we are skipping the target.

  • Examples:

test_backend_and_custom_gate in test/python/compiler/test_transpiler.py:

   backend = GenericBackendV2(
               num_qubits=5,
               coupling_map=[[0, 1], [1, 0], [1, 2], [1, 3], [2, 1], [3, 1], [3, 4], [4, 3]],
           )
  inst_map = InstructionScheduleMap()
  inst_map.add("newgate", [0, 1], pulse.ScheduleBlock())
  newgate = Gate("newgate", 2, [])
  circ = QuantumCircuit(2)
  circ.append(newgate, [0, 1])
  tqc = transpile(
      circ,
      backend,
      inst_map=inst_map,
      basis_gates=["newgate"],
      optimization_level=opt_level,
      seed_transpiler=42,
  )

Output:

qiskit.transpiler.exceptions.TranspilerError: "HighLevelSynthesis was unable to synthesize Instruction(name='newgate', num_qubits=2, num_clbits=0, params=[])."   ```

3. [FIXED] Control Flow:

  • Description: Should we always add control flow to created target?

  • Workaround: always add control flow instructions

  • Examples:

test_transpile_control_flow_no_backend in test/python/compiler/test_transpiler.py:

     qc = QuantumCircuit(QuantumRegister(1, "q"), ClassicalRegister(1, "c"))
     qc.h(0)
     qc.measure(0, 0)
     with qc.if_test((qc.clbits[0], False)):
         qc.x(0)
     with qc.while_loop((qc.clbits[0], True)):
         qc.x(0)
     with qc.for_loop(range(2)):
         qc.x(0)
     with qc.switch(qc.cregs[0]) as case:
         with case(case.DEFAULT):
             qc.x(0)
     qc.measure(0, 0)

     transpiled = transpile(qc, optimization_level=opt_level)

Output:

(raised by passmanager)

qiskit.transpiler.exceptions.TranspilerError: "The control-flow constructs ['for_loop', 'if_else', 'while_loop', 'switch_case'] are not supported by the backend."

4. Scheduling with no coupling map:

  • Description: Target.from_configuration() currently skips instruction durations if no coupling map is provided, which causes issues with some scheduling tests.

  • Workaround: _skip_target

  • Examples:

  1. test_circuit_with_delay in test/python/compiler/test_transpiler.py:
 qc = QuantumCircuit(2)
        qc.h(0)
        qc.delay(500, 1)
        qc.cx(0, 1)

  out = transpile(
      qc,
      scheduling_method="alap",
      basis_gates=["h", "cx"],
      instruction_durations=[("h", 0, 200), ("cx", [0, 1], 700)],
      optimization_level=optimization_level,
      seed_transpiler=42,
  )

Output:

qiskit.transpiler.exceptions.TranspilerError: 'Duration of cx on qubits [0, 1] is not found.'

5. Backend with custom target & asymmetric coupling map

  • Description: The current way we resolve the custom constraints vs. backend priorities require extracting backend (target) properties, resolving the priorities, and re-building a new target. Some special constraints like asymmetric coupling maps are lost in this conversion.

  • Workaround: added _no_loose_constraints flag, if True, the target isn't built from config but directly taken from the backend.

  • Examples:

test_transpile in test/python/providers/test_backend_v2.py:

    self.backend = GenericBackendV2(num_qubits=2, seed=42, basis_gates=["rx", "u"])
    cx_props = {
        (0, 1): InstructionProperties(duration=5.23e-7, error=0.00098115),
    }
    self.backend.target.add_instruction(CXGate(), cx_props)
    ecr_props = {
        (1, 0): InstructionProperties(duration=4.52e-9, error=0.0000132115),
    }
    self.backend.target.add_instruction(ECRGate(), ecr_props)
    tqc = transpile(qc, self.backend, optimization_level=opt_level)

@ElePT ElePT added this to the 1.1.0 milestone Apr 16, 2024
@sbrandhsn
Copy link
Contributor

sbrandhsn commented Apr 16, 2024

I have comments on 1. and 4. :-)
1.: I think the semantics on 1. would be to only route without touching the gates in the circuit. This would be violated with the current workaround when the input circuit uses a custom gate set.

4.: I think if we can schedule with the provided user-input, we should just schedule it. :-) This may be relevant for work flows where an user uses external compilers to generate a physical circuit and then just wants to use qiskit for scheduling and so on. If I understand it correctly, we can schedule if the instruction_durations is complete even in the absence of a coupling map.

@ElePT
Copy link
Contributor Author

ElePT commented Apr 17, 2024

Thanks @sbrandhsn. I see your that your points come from a "transpiler pipeline usability" perspective, which I also believe is important. I guess that my question here is how could we realize these behaviors given the current Target object design.

1.: How could we actually implement this behavior? The initial way I see is to allow for basis_gates==None in the target, and have the pass manager interpret that as a signal to skip all stages except for routing. However, having basis gates is a fundamental assumption of the Target model. The target doesn't have a way to independently store a coupling_map, the internal data model uses a gate_map that contains both gate and connectivity data. This is allows for more flexibility in the target design, because we can specify connectivity at a gate level (open case 5 shows how a coupling map can't always reflect the connectivity details). With no basis gates we have no gate map, and thus no way to store connectivity. This is an issue I honestly am not sure how to overcome.

4.: Unlike the previous example, I think that this one could be implemented by navigating the gray area created by target.num_qubits=None, it's just a matter of whether we want to do it, and potentially have an input like instruction_durations dominate the rest of the inputs in the target construction step (and I understand why @mtreinish has reservations).

The way things are now, we can create a target with no connectivity constraints, that is not an issue. However, this results in the target's num_qubits to be None, the basis gates' qargs to be None and all instructions (basis gates) to be considered global. When an instruction is global, it cannot contain instruction properties (duration, error...), because the definition of these properties requires the qubits the instruction is acting on. So let's say we have the example listed above:

  out = transpile(
      qc,
      scheduling_method="alap",
      basis_gates=["h", "cx"],
      instruction_durations=[("h", 0, 200), ("cx", [0, 1], 700)],
      optimization_level=optimization_level,
      seed_transpiler=42,
  )

Your resulting target will contain 2 instructions (pseudocode): Instruction(name=h, qargs=None, duration=None) and Instruction(name=cx, qargs=None, duration=None). So far, the instructions are global, they exist but have no connectivity or properties. To apply instruction durations, the way it's currently implemented in Target.from_configuration(), you do the following (pseudocode, the actual code tells apart 1q and 2q gates):

# only showing the 1q gate loop for simplicity
for gate in basis_gates:
    for qubit in range(num_qubits):
          ...
          if instruction_durations is not None:
              try:
                  duration = instruction_durations.get(gate, qubit, unit="s")
              except TranspilerError:
                  duration = None

              # Note that to define instruction properties (including durations) you need the qargs!!!
              gate_properties[(qubit,)] = InstructionProperties(
                            duration=duration, error=error, calibration=calibration
                        )

          target.add_instruction(name_mapping[gate], properties=gate_properties, name=gate)

As you can see, if num_qubits is None, you don't even get to run the second loop, and nothing gets added to the target. In fact, there is a previous check that completely skips the whole loop if coupling_map=None (here). Now, the only way I could see to implement your suggestion would be to, in the case where coupling_map is None, give instruction_durations priority in the target construction loop, inspect it first, construct a coupling_map out of it (in this example, the coupling_map would be [0,1]) and use this new coupling map (and consequently, new num_qubits) in the rest of the target construction logic.

I wonder how that would affect cases where targets are constructed to deliberately contain global instructions, and for example, what would we want to do in cases where information doesn't match, let's say basis_gates=['x', 'rx', 'ecr'] but instruction_durations=[("h", 0, 200), ("cx", [0, 1], 700)],. Because this solution would give priority to instruction_durations .

@sbrandhsn
Copy link
Contributor

Sure! I see your point. :-) I guess we could also fail the user when we detect an underspecified Target and point them to specialized passmanagers in that error message. For instance, something along the lines of:
Coupling map without basis gates specified in Target, please use defaultroutingpassmanager if you intended to only route your input circuit(s) or specify basis gates in the input Target.
I can imagine that a user would appreciate such an early fail more than us accidentally performing transpilation steps they did not intend to be performed (but I'm not too sure about this). :-) defaultroutingpassmanager should then do exactly what the routing stage is doing in default passmanager...

@coveralls
Copy link

coveralls commented Apr 30, 2024

Pull Request Test Coverage Report for Build 9317432966

Details

  • 100 of 111 (90.09%) changed or added relevant lines in 3 files are covered.
  • 20 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.579%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/preset_passmanagers/init.py 86 97 88.66%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
qiskit/transpiler/preset_passmanagers/init.py 2 90.3%
crates/qasm2/src/lex.rs 5 92.62%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 9291000954: -0.01%
Covered Lines: 62350
Relevant Lines: 69603

💛 - Coveralls

@ElePT ElePT changed the title [WIP] Update transpile() to convert loose input of constraints to a Target with Target.from_configuration() Update transpile() to convert loose input of constraints to a Target with Target.from_configuration() Apr 30, 2024
@ElePT ElePT marked this pull request as ready for review April 30, 2024 14:34
@ElePT ElePT requested a review from a team as a code owner April 30, 2024 14:34
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

Comment on lines 439 to 444
basis_gates=basis_gates if _skip_target else None,
coupling_map=coupling_map if _skip_target else None,
instruction_durations=instruction_durations if _skip_target else None,
backend_properties=backend_properties if _skip_target else None,
timing_constraints=timing_constraints if _skip_target else None,
inst_map=inst_map if _skip_target else None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not strictly necessary but I added it for debugging (to make sure that the target was actually being used when I wanted it to) and then thought it didn't hurt to have as an additional check. Let me know if it doesn't convince you.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this would be simpler if we moved this logic down into generate_preset_pass_manager() itself. We already expose skip_target there so internally we can change that function to generate the target instead of doing it in transpile. (it might also require moving #11996 to generate_preset_pass_manager() too).

@@ -1329,9 +1329,9 @@ def setUpClass(cls):
super().setUpClass()
cls.backend = Fake27QPulseV1()
cls.backend.configuration().coupling_map = MUMBAI_CMAP
cls.backend.configuration().basis_gates += ["for_loop", "while_loop", "if_else"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that this test wasn't actually updating the backend basis gates, and this made the new target-based path fail with a "control flow gate no found" error. One of the oversights from the old model.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@@ -985,7 +987,8 @@ def instruction_properties(self, index):

def _build_coupling_graph(self):
self._coupling_graph = rx.PyDiGraph(multigraph=False)
self._coupling_graph.add_nodes_from([{} for _ in range(self.num_qubits)])
if self.num_qubits is not None:
self._coupling_graph.add_nodes_from([{} for _ in range(self.num_qubits)])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these changes when I was trying to support the edge case with no coupling_map. This is no longer necessary to support the edge cases because we are skipping the target, but given that Target does technically suport num_qubits=None, should we keep these extra checks?

@mtreinish
Copy link
Member

Yeah, I was thinking of the former case as a step towards doing the latter. But it makes sense that we would need to move everything over in one step to fix all the edge case handling. Maybe we should wait on this final PR until 1.2 so we can look at this in a bit more depth?

def _parse_basis_gates(basis_gates, backend, inst_map, skip_target):
name_mapping = CUSTOM_MAPPING
standard_gates = get_standard_gate_name_mapping()
control_flow_gates = {
Copy link
Member

Choose a reason for hiding this comment

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

There is a static set defined for these names here: https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/controlflow/__init__.py#L27 Although we shouldn't need to match on "continue" or "break" they only appear inside control flow blocks and we shouldn't ever see them at this level.

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
ElePT added 2 commits May 2, 2024 10:38
…e_preset_pass_manager.

* Apply Matt's suggestions for custom mapping and control flow op names.

* Extend docstring, fix typos.
@ElePT
Copy link
Contributor Author

ElePT commented May 2, 2024

Yeah, I was thinking of the former case as a step towards doing the latter. But it makes sense that we would need to move everything over in one step to fix all the edge case handling. Maybe we should wait on this final PR until 1.2 so we can look at this in a bit more depth?

Yes, unfortunately I don't think it's easy to move things from transpile incrementally because of all of the constraint handling that takes place (plus I'd add it makes it harder for people to understand what is done where, there are a few not so intuitive default generations and it's easier if all the logic relies on either transpile or generate_preset_pm). I have pushed a proposal d176a1f where I moved all the target-building and constraint-parsing parts to generate_preset_pass_manager (it took me a bit to make it work).

On one hand, I think this change would make generate_preset_pass_manager more robust to different input combinations, but I am still not 100% sure that we are not changing the previous behavior in some of the edge cases. Especially when a backend and loose constraints are used (I think that the target-based case is ok). Unit tests pass with no issues, but we don't have as many unit tests using generate_preset_pass_manager directly as we have using transpile. In principle the change works, but I am ok with pushing this PR to 1.2 to make sure we are not overseeing anything important regarding backwards compatibility. Maybe add some extra unit tests using generate_preset_pass_manager directly.

I think this PR is in a way now related to #11897.

@mtreinish mtreinish modified the milestones: 1.1.0, 1.2.0 May 2, 2024
@ElePT ElePT added the Changelog: New Feature Include in the "Added" section of the changelog label May 6, 2024
@ElePT ElePT changed the title Update transpile() to convert loose input of constraints to a Target with Target.from_configuration() Update transpile() and generate_preset_pass_manager to convert loose input of constraints to a Target with Target.from_configuration() May 30, 2024
mtreinish
mtreinish previously approved these changes May 30, 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. I like how we've moved all the input handling to one place. I think we should be good from a backwards compat PoV, but if there is another edge case from the extra input normalization in generate_preset_pass_manager we can handle that as part of the normal 1.2 cycle.

@mtreinish mtreinish enabled auto-merge May 31, 2024 12:18
@mtreinish mtreinish added this pull request to the merge queue May 31, 2024
Merged via the queue into Qiskit:main with commit 6132366 May 31, 2024
15 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants