Make fewer accesses, instruction.operation, in exporter.py
#12481
Conversation
Code in qiskit/qasm3/exporter.py uses the object id to track processing gates. But with upcoming changes in Qiskit#12495 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 Qiskit#12495 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 then 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:
|
Pull Request Test Coverage Report for Build 9304610456Details
💛 - Coveralls |
There was a problem hiding this comment.
Given that the exporter is tracking objects by using the id and objects are no longer referentially identical for standard gates (but should be for user-defined gates), I don't think this PR is solving the underlying problem, so there's a pretty high chance that there's paths through, or simple changes we might make in the future that will invalidate it.
If we need the exact id objects to hang around, then we need to be storing at least a ref to the operation in the symbol table, so that the lifetimes of the use of the id (in the table) and the object itself are bound together. That said, given that the symbol tables uses id at all, and that's no longer stable for standard gates, we need to change the symbol table to handle the storage of standard gates without use of the id.
It might be good to look at how hard it is to do that immediately - if we need to merge this as a stop-gap measure, we can, but there's a clear underlying logic bug in the OQ3 exporter that still needs addressing.
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 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 #12776 ? |
|
@1ucian0, #12776 (comment) is a comment on the utility of this PR.
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 #12776 and reevaluate this one. |
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 #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.