Skip to content

Commit

Permalink
Fix PropertySet re-use in BasePassManager.run (#11787)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jakelishman committed Feb 22, 2024
1 parent 9cb0a54 commit 9c1accb
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
14 changes: 12 additions & 2 deletions qiskit/passmanager/passmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
# workflow run.
self.property_set = PropertySet()

if tasks:
Expand Down Expand Up @@ -287,14 +289,22 @@ def _run_workflow(
input_program=program,
**kwargs,
)
passmanager_ir, _ = flow_controller.execute(
passmanager_ir, final_state = flow_controller.execute(
passmanager_ir=passmanager_ir,
state=PassManagerState(
workflow_status=initial_status,
property_set=pass_manager.property_set,
property_set=PropertySet(),
),
callback=kwargs.get("callback", None),
)
# The `property_set` has historically been returned as a mutable attribute on `PassManager`
# This makes us non-reentrant (though `PassManager` would be dependent on its internal tasks to
# be re-entrant if that was required), but is consistent with previous interfaces. We're still
# safe to be called in a serial loop, again assuming internal tasks are re-runnable. The
# conversion to the backend language is also allowed to use the property set, so it must be set
# before calling it.
pass_manager.property_set = final_state.property_set

out_program = pass_manager._passmanager_backend(
passmanager_ir=passmanager_ir,
in_program=program,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
fixes:
- |
:meth:`.BasePassManager.run` will no longer leak the previous :class:`.PropertySet` into new
workflows when called more than once. Previously, the same :class:`.PropertySet` as before
would be used to initialize follow-on runs, which could mean that invalid property information
was being given to tasks. The behavior now matches that of Qiskit 0.44. Fixed `#11784 <https://github.com/Qiskit/qiskit/issues/11784>`__.
21 changes: 21 additions & 0 deletions test/python/passmanager/test_passmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,24 @@ def run(self, passmanager_ir):
with self.assertLogContains(expected):
out = pm.run(data)
self.assertEqual(out, data)

def test_reruns_have_clean_property_set(self):
"""Test that re-using a pass manager produces a clean property set for the state."""

sentinel = object()

class CheckPropertySetClean(GenericPass):
def __init__(self, test_case):
super().__init__()
self.test_case = test_case

def run(self, _):
self.test_case.assertIs(self.property_set["check_property"], None)
self.property_set["check_property"] = sentinel
self.test_case.assertIs(self.property_set["check_property"], sentinel)

pm = ToyPassManager([CheckPropertySetClean(self)])
pm.run(0)
self.assertIs(pm.property_set["check_property"], sentinel)
pm.run(1)
self.assertIs(pm.property_set["check_property"], sentinel)

0 comments on commit 9c1accb

Please sign in to comment.