Fix PropertySet re-use in BasePassManager.run
#11787
Conversation
Since the genericised `PassManager`, the `PropertySet` used in the `WorkflowState` of a pass-manager pipeline was taken directly from the internal state of the `BasePassManager`. This is set to a clean `PropertySet` during the pass-manager initialisation (similar to how it was in the previous version), but is not reset on subsequent runs. This didn't cause problems in the old form because the "iterator" over tasks in the old form was a new `RunningPassManager` instance. Failing to generate a clean property set could lead to passes getting fed old analysis data when the pass manager was used more than once, leading to miscompilations.
jakelishman
added
stable backport potential
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7888745483Details
💛 - Coveralls |
There was a problem hiding this comment.
Thanks @jakelishman this PR looks good to me. The comment below is not a blocker.
| @@ -47,6 +47,8 @@ def __init__( | |||
| """ | |||
| self._tasks = [] | |||
| self.max_iteration = max_iteration | |||
| # This empty property set never gets used; it gets overridden at the completion of a | |||
There was a problem hiding this comment.
Do you want to nullify this attribute? This used to be None in < 0.45. This update disallows users to manually update property_set (e.g. warm-start), but I think this is reasonable. This constructor may mislead the users if they don't read the comment.
There was a problem hiding this comment.
That's an interesting question. I think allowing a warm state might be better to enable by making it an explicit argument to PassManager.run, if we were going to go that route, though I'm not clear there's a real use-case for that - did you have one in mind?
For None vs the empty property set: I'm loosely in favour of having PropertySet() as the default value, because it means that strictly typed code doesn't need to worry about PassManager.property_set possibly being None on every single access, when it can be known that a well behaved runtime should never see that behaviour. PropertySet() is what you'd get if you ran an empty pass manager, and I don't think PassManager needs to use the typing to distinguish the cases of "has never been run" vs "ran and did nothing"; that's a program-ordering concern that the user can always easily resolve themselves.
There was a problem hiding this comment.
Oh also, given that it was PropertySet in 1.0.0, we can't backport the PR if we change the dummy property set to None. That actually means we wouldn't be able to accept that change to this PR, so maybe if you want to talk about it more, we should make a new issue for Qiskit 1.1.
|
@Mergifyio backport stable/0.46 |
✅ Backports have been created
|
Since the genericised `PassManager`, the `PropertySet` used in the `WorkflowState` of a pass-manager pipeline was taken directly from the internal state of the `BasePassManager`. This is set to a clean `PropertySet` during the pass-manager initialisation (similar to how it was in the previous version), but is not reset on subsequent runs. This didn't cause problems in the old form because the "iterator" over tasks in the old form was a new `RunningPassManager` instance. Failing to generate a clean property set could lead to passes getting fed old analysis data when the pass manager was used more than once, leading to miscompilations. (cherry picked from commit 9c1accb)
Since the genericised `PassManager`, the `PropertySet` used in the `WorkflowState` of a pass-manager pipeline was taken directly from the internal state of the `BasePassManager`. This is set to a clean `PropertySet` during the pass-manager initialisation (similar to how it was in the previous version), but is not reset on subsequent runs. This didn't cause problems in the old form because the "iterator" over tasks in the old form was a new `RunningPassManager` instance. Failing to generate a clean property set could lead to passes getting fed old analysis data when the pass manager was used more than once, leading to miscompilations. (cherry picked from commit 9c1accb)
Since the genericised `PassManager`, the `PropertySet` used in the `WorkflowState` of a pass-manager pipeline was taken directly from the internal state of the `BasePassManager`. This is set to a clean `PropertySet` during the pass-manager initialisation (similar to how it was in the previous version), but is not reset on subsequent runs. This didn't cause problems in the old form because the "iterator" over tasks in the old form was a new `RunningPassManager` instance. Failing to generate a clean property set could lead to passes getting fed old analysis data when the pass manager was used more than once, leading to miscompilations. (cherry picked from commit 9c1accb) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Since the genericised `PassManager`, the `PropertySet` used in the `WorkflowState` of a pass-manager pipeline was taken directly from the internal state of the `BasePassManager`. This is set to a clean `PropertySet` during the pass-manager initialisation (similar to how it was in the previous version), but is not reset on subsequent runs. This didn't cause problems in the old form because the "iterator" over tasks in the old form was a new `RunningPassManager` instance. Failing to generate a clean property set could lead to passes getting fed old analysis data when the pass manager was used more than once, leading to miscompilations. (cherry picked from commit 9c1accb) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
Since the genericised
PassManager, thePropertySetused in theWorkflowStateof a pass-manager pipeline was taken directly from the internal state of theBasePassManager. This is set to a cleanPropertySetduring the pass-manager initialisation (similar to how it was in the previous version), but is not reset on subsequent runs. This didn't cause problems in the old form because the "iterator" over tasks in the old form was a newRunningPassManagerinstance.Failing to generate a clean property set could lead to passes getting fed old analysis data when the pass manager was used more than once, leading to miscompilations.
Details and comments
Fix #11784. This was a bug introduced in Qiskit 0.45.0, so I don't think it's critical to merge ahead of 1.0.0final, but can go into 1.0.1 and is suitable for backport to 0.46.1 as well.