Base pass manager bug fixes

[nkanazawa1989]

Summary

Newly introduced pass manager module relies on the transpiler module for its test, and there is no direct test code for another implementation of pass manager subclass.

I found several bugs that the transpiler test doesn't discover and fixed them in this PR.

Details and comments

Own test for the pass manager module is added.

Regarding the property set in the GenericPass, I remember I didn't want to make the property set an instance attribute of the GenericPass. Since property set is sort of a global variable that must be shared among all tasks in the pass manager, making it an attribute of a particular object might confuse developers. I was thinking of the following interface

class GenericPass(Task):
    ...
    def run(passmanager_ir, property_set):

but indeed this is a breaking API change for existing circuit passes and I hesitated to add this change. In this PR, I just added property_set to GenericPass because this is a minimal change. I'd like to hear the reviewer's opinion :)

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[jakelishman]

For the most part, this looks ok to me, though I think the problems with string handling are rather more general. I'm also vaguely worried that we've got some baked-in assumption of idempotence, which I don't think we've assumed before.

[jakelishman]

I kind of get why we're doing this, but the need also potentially points to a data-model problem in the idea of "completed passes" to me; the pass still completely after a loop iteration, and it's not immediately clear to me that starting a new loop should throw that information away.

What part of the logic is causing a pass to get skipped if it was completed? Passes aren't required to be idempotent, so a pass having been run shouldn't be sufficient to cause it to be skipped on a second attempt.

[nkanazawa1989]

This line. Change to this logic causes many test failures in test.python.transpiler.test_pass_scheduler

qiskit/qiskit/passmanager/base_tasks.py

Lines 95 to 99 in dabc5e6

	if self not in state.workflow_status.completed_passes:
	ret = self.run(passmanager_ir)
	run_state = RunState.SUCCESS
	else:
	run_state = RunState.SKIP

I guess this protection is added because of GenericPass.required. i.e.

pm = PassManager([PassB, PassA[required=PassB]])

runs PassB twice.

[jakelishman]

Ok, but I think that's potentially indicative of buggy handling: pass_ = MyPass(); PassManager([pass_, pass_]) should run pass_ twice unless the pass specifically marks itself as being idempotent somehow. It's a different story if the pass is inserted into the pipeline by requires, because that's an implicit add and it's fine to re-use a previous run if we know that the output from it is still valid.

[jakelishman]

Ah, I just tried it on 0.25.3, and I realise now that what I'm complaining about is pre-existing behaviour. I think that's bad behaviour, but it being pre-existing puts it out of scope of this PR. The fix you've got in this comment is fine.

[nkanazawa1989]

I don't really like the required mechanism. Since dependencies are needed to be instantiated within the target pass, the target pass may require extra constructor args if passes have different interface. If we remove this mechanism, I think we can drop completed_passes and awkward pass equality check.

[jakelishman]

We definitely do use the required handling in some places in the existing pipelines, so it won't be as easy as completely stripping it out. I think the intent of it is good, it's just that it uses referential pass equality to implicitly mean "idempotent", which isn't generally true, and that's the knock-on problem here.

[nkanazawa1989]

Actually circuit transform pass is not idempotent (so pass_ = MyPass(); PassManager([pass_, pass_]) should work as you expect), but it also removes all passes because .preserves is usually empty at least in Qiskit passes.

qiskit/qiskit/transpiler/basepasses.py

Lines 229 to 237 in dabc5e6

	def update_status(
	self,
	state: PassManagerState,
	run_state: RunState,
	) -> PassManagerState:
	state = super().update_status(state, run_state)
	if run_state == RunState.SUCCESS:
	state.workflow_status.completed_passes.intersection_update(set(self.preserves))
	return state

[jakelishman]

Yeah, that's what I mean - I think PassManager([pass_, pass_]) should run pass_ twice in the general case, because there's nothing marking _pass as idempotent, but the "has this pass run?" handling that existed in 0.25.3 would mean it only runs once. Since it was in 0.25.3 and the 0.45.0 passmanager just maintains the same behaviour, there's no need to rush in a fix now.

[jakelishman]

Is this code copied from somewhere? The exception handling within this function shouldn't be necessary; wrapping a "standard" call to this function in the self.assertRaises context manager in the regular test should all work correctly.

It'd also likely be better if this function was a context-manager wrapper around unittest.TestCase.assertLogs - it's much easier to read tests written in context-manager form than "exploded function call" (assertRaises(f, arg0, arg1, ...)).

[nkanazawa1989]

I needed this to check

qiskit/qiskit/passmanager/base_tasks.py

Lines 103 to 115 in dabc5e6

	finally:
	ret = ret or passmanager_ir
	if run_state != RunState.SKIP:
	running_time = time.time() - start_time
	logger.info("Pass: %s - %.5f (ms)", self.name(), running_time * 1000)
	if callback is not None:
	callback(
	task=self,
	passmanager_ir=ret,
	property_set=state.property_set,
	running_time=running_time,
	count=state.workflow_status.count,
	)

The assertRaises context manager will catch the error as you say, but the assertLogEqual function returns before checking the log.

[jakelishman]

Right, but if they're both used as context managers, the clean-up code from both (the bit that contains the assertions) will still run in both correctly; we can use the context-manager stack to manage things arbitrarily, rather than needing hard-coded functions.

[jakelishman]

In very very very rough form, what I'm meaning is something like:

import contextlib]
import unittest

class MyTestCas(unittest.TestCase):
    @contextlib.contextmanager
    def assertLogContains(self, expected_lines):        
        with self.assertLogs() as cm:
            yield cm
        recorded_lines = cm.output
        for i, (expected, recorded) in enumerate(zip(expected_lines, recorded_lines)):
            # ... your existing handling

then I think it should work in test cases as:

def test_logs_while_raising(self):
    with self.assertLogContains(["blah blah"]):
        getLogger().log("blah blah")
        with self.assertRaises(Exception):
            raise Exception

and that test would pass.

[jakelishman]

(possibly the log-checking code should be in a finally block, so there's no spurious passes if a test author accidentally puts the assertLogContains and assertRaises calls the wrong way round)

[nkanazawa1989]

Ah, that's neat. I'll try

[nkanazawa1989]

Done in 846e416. I needed some ugly hack because of this logic.

[nkanazawa1989]

Hmmm it didn't work. The default formatter is bit ugly but maybe okey.

Expected:
#00: Pass: TaskC - (\d*\.)?\d+ \(ms\)
#01: Pass: TaskB - (\d*\.)?\d+ \(ms\)

Recorded:
#00: INFO:qiskit.passmanager.base_tasks:Pass: TaskA - 0.00596 (ms)
#01: INFO:qiskit.passmanager.base_tasks:Pass: TaskB - 0.00691 (ms)

[jakelishman]

Oh, that's unfortunately gross of the standard library. Unfortunately, I think we're going to have problems with the private method: the unittest/_log.py was only split out from unittest/case.py in Python 3.9, so if we're going to go the way of accessing private internals, we'll need to have an import catch for Python 3.8 to pull it from unittest.case instead. The actual private code didn't meaningfully change, though, so it'll just about hold up.

Is there any way we can write the test enforcing the default logging format, so we can avoid needing the private access?

[jakelishman]

This looks a lot neater now, thanks so much Naoki. Assuming the tests and lint pass, I'm happy to merge this now - if there's any further tweaks to be made, we can do them post release anyway.