Move QuantumCircuit.assign_parameters to Rust
#12794
Conversation
jakelishman
added
performance
Changelog: None
jakelishman
requested review from
alexanderivrii,
ShellyGarion and
a team
as code owners
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10188851464Details
💛 - Coveralls |
This is (as far as I could tell), the last really major performance regression in our asv suite compared to 1.1.0, so with this commit, we should be at _not worse_ for important utility-scale benchmarks. This largely rewrites `ParamTable` (renamed back to `ParameterTable` because I kept getting confused with `Param`) to have more Rust-friendly interfaces available, so that `assign_parameters` can then use them. This represents a 2-3x speedup in `assign_parameters` performance over 1.1.0, when binding simple `Parameter` instances. Approximately 75% of the time is now spent in Python-space `Parameter.assign` and `ParameterExpression.numeric` calls; almost all of this could be removed were we to move `Parameter` and `ParameterExpression` to have their data exposed directly to Rust space. The percentage of time spent in Python space only increases if the expressions to be bound are actual `ParameterExpression`s and not just `Parameter`. Most changes in the test suite are because of the use of internal-only methods that changed with the new `ParameterTable`. The only discrepancy is a bug in `test_pauli_feature_map`, which was trying to assign using a set.
jakelishman
force-pushed
the
rust-assign-parameters
branch
from
059fb2d to
0e2bd68
Compare
|
I've tagged this as high priority because there are some bugs in parameter binding that @woodsp-ibm identified on main that this branch fixes. So we shouldn't tag 1.2.0rc1 without this included. |
|
Oh, what bugs? edit: the point being: let's add unit tests. |
|
I linked the algorithms PR I have which was to address issues arising when testing against Qsikit main that showed the bug being referred to. In that PR one was due to use of some internals that are no longer there, the other, where I had added a .data copy as a workaround was the bug being referred to. I had some standalone code as a sample if its of help - I showed it to Matthew when discussing the issue so he has it too. |
|
The example Steve shared was: from qiskit.circuit import QuantumCircuit, Parameter, ClassicalRegister, QuantumRegister
from qiskit.transpiler.passes import TranslateParameterizedGates
SUPPORTED_GATES = [
"rx",
"ry",
"rz",
"rzx",
"rzz",
"ryy",
"rxx",
"cx",
"cy",
"cz",
"ccx",
"swap",
"iswap",
"h",
"t",
"s",
"sdg",
"x",
"y",
"z",
]
a = Parameter("a")
qc = QuantumCircuit(1)
qc.h(0)
qc.p(a, 0)
qc.h(0)
print(qc)
print(qc.parameters)
translator = TranslateParameterizedGates(SUPPORTED_GATES)
qc = translator(qc)
print(qc)
print(qc.parameters)
qc1 = qc.copy()
qr_aux = QuantumRegister(1, "qr_aux")
cr_aux = ClassicalRegister(1, "cr_aux")
qc1.add_register(qr_aux)
qc1.add_register(cr_aux)
qc1.h(qr_aux)
qc1.data.insert(0, qc1.data.pop()) # This line seems to mess things up
print(qc1)
print(qc1.parameters)
cct = qc1.assign_parameters({a: 0.44})
print(cct)
print(cct.parameters)on main it looked like the final circuit printed was not binding |
|
Yeah, I'm pretty sure I know exactly where that bug had come in, and I remember moving a line of code that I imagine is what fixed it. In the insert code, I think the new data is written in before we called untrack on it, so it untracked the wrong thing. |
This catches a bug that was present in the parent commit, but this PR fixes.
|
I added a test that catches Steve's case in 1cd8d08. It turned out to be a bit more complex than I'd originally thought - it's a particular interaction with the parametric global-phase tracking in addition to the pop/insert retracking required, and somewhere along the line, that was causing the mess-up. |
| } | ||
|
|
||
| /// Backup entry point for appending an instruction from Python space, in the unusual case that | ||
| /// one of the instruction parameters contains a cyclical reference to the circuit itself. |
There was a problem hiding this comment.
To make sure I understand: you mean something like this with cyclical?
circuit = # some circuit with free parameters
circuit.append(circuit.to_gate(), circuit.qubits)
There was a problem hiding this comment.
This is actually most copied from the original source - I just rearranged the functions a bit. Matt originally wrote it, and I think the original problem was somehow we had a test that had a control-flow operation that put the same circuit to be one of its blocks?? I don't entirely remember.
There was a problem hiding this comment.
Yeah, I don't remember exactly where it was happening, but there was a case with a control flow op was causing an error in the runtime borrow checking because the same CircuitData object was being passed in here.
| @@ -160,172 +164,73 @@ impl CircuitData { | |||
| #[cfg(feature = "cache_pygates")] | |||
| py_op: RefCell::new(None), | |||
| }); | |||
| res.track_instruction_parameters(py, res.data.len() - 1)?; | |||
There was a problem hiding this comment.
This is technically an oversight because I guess it's possible for someone in rust space to create a parameter object and use that here. But, in practice I didn't think anyone would. Do you think it's worth the overhead of doing this for the rare case?
There was a problem hiding this comment.
I think as long as we're accepting general Param instances, I think it's a mistake not to. At the moment, the way we use it makes it very unlikely for the Params to be anything other than floats, but I don't think it'd be too hard to have someone call this on a circuit they got from Python space after applying some filter / join of the iterables or something.
The ParameterTable isn't a public field (and imo absolutely shouldn't be), so unless the typing of this function is changed so that it only accepts f64 as the params, I think we need to do this.
The overhead shouldn't be much, since we can determine that it's a no-op without any Python-space calls.
| } | ||
|
|
||
| /// Backup entry point for appending an instruction from Python space, in the unusual case that | ||
| /// one of the instruction parameters contains a cyclical reference to the circuit itself. |
There was a problem hiding this comment.
Yeah, I don't remember exactly where it was happening, but there was a case with a control flow op was causing an error in the runtime borrow checking because the same CircuitData object was being passed in here.
|
Ok, comments should be addressed, and the Fwiw they've been pre-existing in some form or another ever since |
mtreinish
added
the
stable backport potential
| op.getattr(params_attr)?.set_item(parameter, new_param)?; | ||
| let mut new_op = op.extract::<OperationFromPython>()?; | ||
| previous.op = new_op.operation; | ||
| previous.params_mut().swap_with_slice(&mut new_op.params); |
There was a problem hiding this comment.
I didn't know about this method, I like it. It's uses are a bit niche, but when you need it like here it is handy.
There was a problem hiding this comment.
Yeah, I learned about when writing this as well. It felt like a method that would exist in Rust, so I had a quick look.
| impl<'py> FromPyObject<'py> for ParameterUuid { | ||
| fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult<Self> { | ||
| if ob.is_exact_instance(UUID.get_bound(ob.py())) { | ||
| ob.getattr(intern!(ob.py(), "int"))?.extract().map(Self) | ||
| } else { | ||
| Err(PyTypeError::new_err("not a UUID")) | ||
| } |
There was a problem hiding this comment.
We might want to make a note for the future that pyo3 has an open PR adding uuid conversion support for:
https://docs.rs/uuid/latest/uuid/
that might be something we want to leverage in the future here.
| self.order.reserve(self.by_uuid.len()); | ||
| self.order.extend(self.by_uuid.keys()); |
There was a problem hiding this comment.
extend() doesn't do the allocation all at once for us?
There was a problem hiding this comment.
I think it's possible that this is left over from some point when the internal data structures were different, and I was getting the keys from something that didn't necessarily have an accurate size_hint. I don't entirely remember.
| self.uuid_map.insert(uuid, parameter); | ||
| self.order.reserve(self.by_uuid.len()); | ||
| self.order.extend(self.by_uuid.keys()); | ||
| self.order.sort_unstable_by_key(|uuid| { |
There was a problem hiding this comment.
I wonder if there is a threshold where rayon's par_sort_unstable_by_key() would be useful here. We can look at that in the future.
There was a problem hiding this comment.
Quite possibly yes, unless it interferes with the fastest paths for data that's already sorted. The cases we need this to have the highest performance correspond to IBM backends' fast parametric updates, where all the parameters are likely to be in the circuit in order to begin with.
| # During normalisation, be sure to reference 'parameters' and related things from 'self' not | ||
| # 'target' so we can take advantage of any caching we might be doing. | ||
| if isinstance(parameters, dict): | ||
| if isinstance(parameters, collections.abc.Mapping): |
There was a problem hiding this comment.
Are there cases people are passing dict likes to assign_parameters? Also I assume you're not worried about the extra runtime overhead of the registration hooks of the collections types here.
There was a problem hiding this comment.
No, and in fact it would have been impossible before because of the type hint. I'd loosened this because a) that matches the documentation and b) I think I was passing the ParameterBindsDict custom object recursively into subcalls at some point (though I think I changed the logic subsequently).
* Move `QuantumCircuit.assign_parameters` to Rust This is (as far as I could tell), the last really major performance regression in our asv suite compared to 1.1.0, so with this commit, we should be at _not worse_ for important utility-scale benchmarks. This largely rewrites `ParamTable` (renamed back to `ParameterTable` because I kept getting confused with `Param`) to have more Rust-friendly interfaces available, so that `assign_parameters` can then use them. This represents a 2-3x speedup in `assign_parameters` performance over 1.1.0, when binding simple `Parameter` instances. Approximately 75% of the time is now spent in Python-space `Parameter.assign` and `ParameterExpression.numeric` calls; almost all of this could be removed were we to move `Parameter` and `ParameterExpression` to have their data exposed directly to Rust space. The percentage of time spent in Python space only increases if the expressions to be bound are actual `ParameterExpression`s and not just `Parameter`. Most changes in the test suite are because of the use of internal-only methods that changed with the new `ParameterTable`. The only discrepancy is a bug in `test_pauli_feature_map`, which was trying to assign using a set. * Add unit test of parameter insertion This catches a bug that was present in the parent commit, but this PR fixes. * Update crates/circuit/src/imports.rs * Fix assignment to `AnnotatedOperation` * Rename `CircuitData::num_params` to match normal terminology * Fix typos and 🇺🇸 * Fix lint (cherry picked from commit a68de4f)
* Move `QuantumCircuit.assign_parameters` to Rust This is (as far as I could tell), the last really major performance regression in our asv suite compared to 1.1.0, so with this commit, we should be at _not worse_ for important utility-scale benchmarks. This largely rewrites `ParamTable` (renamed back to `ParameterTable` because I kept getting confused with `Param`) to have more Rust-friendly interfaces available, so that `assign_parameters` can then use them. This represents a 2-3x speedup in `assign_parameters` performance over 1.1.0, when binding simple `Parameter` instances. Approximately 75% of the time is now spent in Python-space `Parameter.assign` and `ParameterExpression.numeric` calls; almost all of this could be removed were we to move `Parameter` and `ParameterExpression` to have their data exposed directly to Rust space. The percentage of time spent in Python space only increases if the expressions to be bound are actual `ParameterExpression`s and not just `Parameter`. Most changes in the test suite are because of the use of internal-only methods that changed with the new `ParameterTable`. The only discrepancy is a bug in `test_pauli_feature_map`, which was trying to assign using a set. * Add unit test of parameter insertion This catches a bug that was present in the parent commit, but this PR fixes. * Update crates/circuit/src/imports.rs * Fix assignment to `AnnotatedOperation` * Rename `CircuitData::num_params` to match normal terminology * Fix typos and 🇺🇸 * Fix lint (cherry picked from commit a68de4f) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
* Move `QuantumCircuit.assign_parameters` to Rust This is (as far as I could tell), the last really major performance regression in our asv suite compared to 1.1.0, so with this commit, we should be at _not worse_ for important utility-scale benchmarks. This largely rewrites `ParamTable` (renamed back to `ParameterTable` because I kept getting confused with `Param`) to have more Rust-friendly interfaces available, so that `assign_parameters` can then use them. This represents a 2-3x speedup in `assign_parameters` performance over 1.1.0, when binding simple `Parameter` instances. Approximately 75% of the time is now spent in Python-space `Parameter.assign` and `ParameterExpression.numeric` calls; almost all of this could be removed were we to move `Parameter` and `ParameterExpression` to have their data exposed directly to Rust space. The percentage of time spent in Python space only increases if the expressions to be bound are actual `ParameterExpression`s and not just `Parameter`. Most changes in the test suite are because of the use of internal-only methods that changed with the new `ParameterTable`. The only discrepancy is a bug in `test_pauli_feature_map`, which was trying to assign using a set. * Add unit test of parameter insertion This catches a bug that was present in the parent commit, but this PR fixes. * Update crates/circuit/src/imports.rs * Fix assignment to `AnnotatedOperation` * Rename `CircuitData::num_params` to match normal terminology * Fix typos and 🇺🇸 * Fix lint
When calling `assign_parameters` on a heavily parametric circuit with the unusual access pattern of binding off a single parameter at a time, Qiskit 1.2 had a severe performance regression compared to Qiskit 1.1 stemming from Qiskitgh-12794. The calls to `unsorted_parameters` on each iteration were creating a new `set`, which could be huge if the number of parameters in the circuit was large. In Qiskit 1.1 and before, that object was a direct view onto the underlying `ParameterTable` (assuming the input circuit did not have a parametric global phase), so was free to construct.
…s` (#13337) * Fix performance regression in looped `QuantumCircuit.assign_parameters` When calling `assign_parameters` on a heavily parametric circuit with the unusual access pattern of binding off a single parameter at a time, Qiskit 1.2 had a severe performance regression compared to Qiskit 1.1 stemming from gh-12794. The calls to `unsorted_parameters` on each iteration were creating a new `set`, which could be huge if the number of parameters in the circuit was large. In Qiskit 1.1 and before, that object was a direct view onto the underlying `ParameterTable` (assuming the input circuit did not have a parametric global phase), so was free to construct. * Improve documentation
Summary
This is (as far as I could tell), the last really major performance regression in our asv suite compared to 1.1.0, so with this commit, we should be at not worse for important utility-scale benchmarks.
This largely rewrites
ParamTable(renamed back toParameterTablebecause I kept getting confused withParam) to have more Rust-friendly interfaces available, so thatassign_parameterscan then use them.This represents a 2-3x speedup in
assign_parametersperformance over 1.1.0, when binding simpleParameterinstances. Approximately 75% of the time is now spent in Python-spaceParameter.assignandParameterExpression.numericcalls; almost all of this could be removed were we to moveParameterandParameterExpressionto have their data exposed directly to Rust space. The percentage of time spent in Python space only increases if the expressions to be bound are actualParameterExpressions and not justParameter.Most changes in the test suite are because of the use of internal-only methods that changed with the new
ParameterTable. The only discrepancy is a bug intest_pauli_feature_map, which was trying to assign using a set.Details and comments
Built on #12730, so will need rebasing over it.
I think this first commit might accidentally have introduced a small (10%) regression to parametric-circuit construction time over its parent. That's a mistake if so - I should be able to fix that later.edit: on retiming, I couldn't reproduce a problem - if anything, this commit is a minor improvement.Timings for parametric circuit benchmarks compared to 1.1.0 (the different SHA1 is because I hadn't written the commit message when I took the benchmark):