qiskit
qiskit copied to clipboard
Qiskit
Make fewer accesses, `instruction.operation`, in exporter.py
EDIT: To clarify the motivation. This fixes a bug in the exporter that is exposed by #12459 .
Code in qiskit/qasm3/exporter.py uses the object id to track processing gates. But with upcoming changes in #12459 a new object is created every time an operation is accessed within an instruction. Python allows an object's id to be reused after its reference count goes to zero. As a result, sometimes a stored id refers to two different gates (one of them no longer exists as an object) This will cause errors to be raised.
This PR replaces several repeated accesses with a single access. This is also more efficient since each access actually constructs an object.
In particular when testing https://github.com/Qiskit/qiskit/pull/12459 a code path is found in which a gate is accessed, its id is cached, it is freed, and another gate is accessed; all with no intervening statements. This PR prevents the first gate object from being freed until after the second gate is accessed and analyzed.
However, this PR does not remove all possible recycling of ids during a code section that uses them to track gate processing. This design should be replaced with a new approach.
One or more of the following people are relevant to this code:
@Qiskit/terra-core
Pull Request Test Coverage Report for Build 9304610456
Details
- 45 of 45 (100.0%) changed or added relevant lines in 1 file are covered.
- 20 unchanged lines in 3 files lost coverage.
- Overall coverage decreased (-0.02%) to 89.578%
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| crates/qasm2/src/expr.rs | 1 | 94.02% |
| crates/qasm2/src/lex.rs | 7 | 91.6% |
| crates/qasm2/src/parse.rs | 12 | 96.69% |
| <!-- | Total: | 20 |
| Totals | |
|---|---|
| Change from base Build 9291000954: | -0.02% |
| Covered Lines: | 62317 |
| Relevant Lines: | 69567 |
💛 - Coveralls
I don't think this PR is solving the underlying problem
Right, this is what I meant to say with the last line of the opening comment.
I was working kind of quickly, so I didn't think yet of a better solution. But maybe it's not too difficult. I agree that merging this bandaid is not anything near ideal.
#12776 implements the type of fix I was talking about above by rewriting how the symbol table works to remove the assumption that instruction.operation has a stable ID for standard-gate objects.
The other components of this might still be a performance upgrade - I haven't checked - so this might still be something we want to merge.
Should this PR be closed in favor of https://github.com/Qiskit/qiskit/pull/12776 ?
@1ucian0, https://github.com/Qiskit/qiskit/pull/12776#discussion_r1683579441 is a comment on the utility of this PR.
- #12776 heavily edits the same code that I touch here. So trying to merge this would not be clean.
- I doubt the performance benefit would be significant. Given this and the previous item, I don't want to invest time in this at present (if ever).
But I'd prefer to wait til #12776 lands in case @jakelishman wants to weigh in.
In the meantime, I converted this to a draft to backburner it.
Sounds good! Let's do that: wait for the merge of https://github.com/Qiskit/qiskit/pull/12776 and reevaluate this one.