Primitives support the dynamic circuits with control flow

[ikkoham]

Summary

Primitives support the dynamic circuits with control flow.
If backend supports it, users can run the circuits using BackendSampler
and BackendEstimator.

Details and comments

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

• @Qiskit/terra-core
• @ajavadia
• @ikkoham
• @levbishop
• @t-imamichi

[coveralls]

Pull Request Test Coverage Report for Build 3950150312

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.003%) to 84.871%

---

Totals
Change from base Build 3950148285:	-0.003%
Covered Lines:	66042
Relevant Lines:	77815

---

💛 - Coveralls

[jakelishman]

The change looks fine to me, thanks. I had a couple of questions about scope and testing, but they should hopefully be straightforwards.

[jakelishman]

Does this change apply to only the BackendSampler and BackendEstimator, or is this a general improvement that affects all BaseSampler and BaseEstimator subclasses?

[ikkoham]

You are right. When I wrote this document, I thought that if a backend did not support control flow, primitives also did not support, but I was wrong.

[jakelishman]

Aer supports control flow - would it be possible to try executing a simple Sampler and Estimator run on a BackendX backed by AerSimulator (with the appropriate @unittest.skipUnless(optionals.HAS_AER)).

[ikkoham]

Thanks. I'll try it.

[jakelishman]

I reworded the release note a little, but other than that I'm largely fine. The Aer test looks good, thanks.

I'm a bit concerned by some of the testing of _circuit_key, especially in that it seems to be implying it's being accessed over an API boundary by downstream packages. That particular issue hasn't come from this PR, but it may warrant following up on.

[jakelishman]

This json line doesn't look like it belongs in Terra - JSON serialisation of the internal object _circuit_key shouldn't be something Terra needs, and no package downstream of us should be relying on a private function having any particular behaviour.

Using a dummy primitive whose _run method (or whatever) just makes assertions / leaks the received circuit back to the caller somehow could be a cleaner way of making a public-API test.

[jakelishman]

I'm not a big fan of these tests of internal features, because generally they make it harder for the internal implementation to evolve without changing the tests. Ideally we should only be testing the outward public behaviour, and not the particulars of how that's achieved. If the tests are too tightly coupled, then any change to the internal implementation has to change the tests, and that makes it much easier for bugs and regressions to sneak in; we have to verify that the new tests are equivalent, and cover the same ground.

That said, this is a single function, and if you think it's much better to test this way, I won't block this PR on it.

I'd potentially attempt to rewrite the tests by using a custom subclass of the primitives for testing that has its _run (or whatever it's called) method defined to make assertions about the normalised circuits it gets back from the base class's handling. That's testing public parts of the subclassing API, rather than this internal detail of how that's done.

edit: I also see now that these tests are just moved from one file to another, rather than actually written new. I'm still not a fan, but it does move this change out-of-scope of this PR.

[ikkoham]

Yes, it is true. I think this should have been changed to a public function rather than a private function. (At first it was fine to keep it private, but the use of it has increased.)

[ikkoham]

I made _circuit_key public since this function has been already used from external (i.e. qiskit/algorithms/gradients).

[jakelishman]

I'm not a fan of making _circuit_key public, because of the concerns I've expressed (before on Slack, and on other primitives issues) about the management of circuits internally to enable the primitives' API and circuit_key's part in that, but that's not my call to make.

If you're going to do that, though, I'd much rather it was in a separate PR, which would now need to be targetted to Terra 0.24, rather than 0.23 whose release candidate is due on Thursday.

[jakelishman]

@ikkoham: by the way, we tagged this as a release blocker for 0.23rc1, because I think it's something you need very strongly? Let us know if it's not the case and we can downgrade it.

[ikkoham]

This PR is needed in 0.23. I understand that private testing is not good, so I will create a separate PR and make it public with deprecation in 0.24.

[jakelishman]

I'm still not convinced by why the _circuit_key should be tested as JSON-serialisable with the default serialiser, but given that private function seems to be used elsewhere in the stack, I'm guessing it's something that needs to become part of its contract if/when it becomes public.

At any rate, the actual bulk of this PR I'm fine with, and my testing concerns are more about the risk of increased maintenance effort, which is relatively minor and not in my domain anyway.