Add fast transpilation method to BaseExperiment
#1459
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wshanks
force-pushed
the
base-fast-transpile
branch
from
1ebfb0d
to
2806a04
Compare
|
Current status:
I added the following code to import inspect
import unittest
test_frame = next(f[0] for f in inspect.stack() if any(isinstance(l, unittest.TestCase) for n, l in f[0].f_locals.items()))
test = next(v for v in test_frame.f_locals.values())
print(f"full_transpile={full_transpile} for {test.id()}")Here is the full output from running the tests. I have not gone through yet to check that it makes sense which tests are using |
wshanks
force-pushed
the
base-fast-transpile
branch
from
2806a04
to
662f19b
Compare
There was a problem hiding this comment.
I like new transpile module and how it's implemented :) We can get more performance boost once after Qiskit core moves to Rust and the .compose method is reimplemented in Rust too. I'm curious what happens if we just set _layout and dump it into oq3 string as a payload, instead of expanding the circuit object (of course this will break simulator workflow and not realistic).
| transpilation_needed = False | ||
|
|
||
| if getattr(backend, "version", 0) <= 1: | ||
| # Fall back to transpilation for BackendV1 |
There was a problem hiding this comment.
Why this always requires full transpile?
There was a problem hiding this comment.
We could relax this condition if we want. For simplicity, I just wanted to use a Target below. Since the fall back case is just the current status, it is not a regression to ignore BackendV1. We could instead do the BackendV1 to BackendV2 conversion or some other approach. Partly I was thinking that that backend conversion occasionally hits edge cases and also Qiskit wants to drop BackendV1 in 2.0, I think, so it seemed more future proof not handle BackendV1, but I don't feel strongly.
| target = backend.target | ||
|
|
||
| for circ in circuits: | ||
| for inst in circ.data: |
There was a problem hiding this comment.
Do you have performance profile for something like RB which is ISA but circuit is extremely deep? (RB overwrites the transpile method and probably not a good test case though). The most of experiment instances in our library are with short depth circuit and this double loop should be acceptable considering the transpiler overhead, but I'd like to see some extreme case.
Alternatively we can let experiment class report dependency (e.g. BaseExperiment.__required_gates__) and we can test against it, which seems very performant but not really maintainable.
There was a problem hiding this comment.
Yes, I wish Qiskit maintained something like the equivalent of count_ops in QuantumCircuit as it is built, so it would not be necessary to iterate through the circuit to find its instructions. I wonder if that has been considered before?
I did not feel too bad about this double loop because I think that transpile has to do it as well for every transpiler pass that operates on individual instructions, like BasisTranslator, so I don't think this could be slower.
RB is a good example of where _transpiled_circuits can still be overridden when necessary for performance.
I don't like BaseExperiment.__required_gates__ unless there is a convenient way to manage it since most experiments have small circuits like you mentioned. I have also wondered about transpiling on the fly -- instead of doing circ.x(0), doing circ.compose(self.get("x")) (needs the backend to already be set), but I don't like the way it breaks from idiomatic way of building circuits.
| transpile_opts = copy.copy(self.transpile_options.__dict__) | ||
| transpile_opts["initial_layout"] = list(self.physical_qubits) | ||
| transpiled = transpile(self.circuits(), self.backend, **transpile_opts) | ||
| circuits = [map_qubits(c, self.physical_qubits) for c in self.circuits()] |
There was a problem hiding this comment.
Why you don't take actual qubit size from the backend when available?
There was a problem hiding this comment.
Just because I had never seen it matter that the circuit size matched the backend. We can set it if it matters (for qasm3?).
| inplace=True, | ||
| copy=False, | ||
| ) | ||
| return p_circ |
There was a problem hiding this comment.
Perhaps you need to set ._layout since this is how the Qiskit qasm3 module distinguishes between physical and virtual circuits (our primary payload is qpy which doesn't care layout but in case someone prefers qasm3).
There was a problem hiding this comment.
Interesting. I was not familiar with that property. It does seem like it should be set. I would want to check that it is right to set that private attribute for this case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.