One or more of the following people are relevant to this code:

• @Qiskit/terra-core

Pull Request Test Coverage Report for Build 14881523118

Details

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• 4 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.02%) to 87.847%

Totals
Change from base Build 14848101443:	0.02%
Covered Lines:	74531
Relevant Lines:	84842

💛 - Coveralls

I think it would be safer to just invalidate the cache if the basis gate set changes. Storing multiple basic approximations could become quite memory-heavy and I'm not sure this is actually a use case we even need. Users could also explicitly load the basic approximations they care about, if they need to change in between them.

So, unless we have an explicit case for caching multiple decompositions, I think just invalidating the cache is safer and simpler 🙂

As discussed offline, to be fully correct, we should also cache the depth (in addition to basis_gates) used to generate the basic approximations.

In practice the number of gate sequences we need to store to is surprisingly small: for instance, we only need to store 812 sequences for the default basis set [h, t, tdg] and the default depth 10, and we only need to store 6844 sequences for depth 16. This is because we do not store different gate sequences that lead to the same SO(3)-matrix. So we either need to cache the basic approximations both on basis_gates and depth, or to re-compute basic approximation if either of the two changes. I don't have a strong opinion which is better, but I am happy to go with the re-compute approach.

My only mild concern is that since _sk is a class-level attribute, we should never ever run two SolovayKitaevSynthesis plugins in parallel threads. @Cryoris, do you think this is a safe assumption? (On the other hand, this does not make the current behavior worse, and we can think more about this in the new amazing Rust code).

Hm yes I guess that if you would have multiple processes with different depths or basis gates then that could be inefficient, since it would have to regenerate the sets a lot. I don't know if Python's multiprocessing has some kind of lock on used variables or if that could also result in wrong behavior... I don't think we can assume that no one will ever run in parallel, given that the pass manager tries running multiple processes (though here it'll use the same settings for the plugin).

Hm yes I guess that if you would have multiple processes with different depths or basis gates then that could be inefficient, since it would have to regenerate the sets a lot. I don't know if Python's multiprocessing has some kind of lock on used variables or if that could also result in wrong behavior... I don't think we can assume that no one will ever run in parallel, given that the pass manager tries running multiple processes (though here it'll use the same settings for the plugin).

Multiprocessing doesn't have shared memory, that's kind of the point for Python and is how you get around GIL contention. Each process space has separate memory and when we do the parallel dispatch we make a copy of the pass manager (via pickle that's sent via the IPC to the subprocess). So the cache will be separate when running in parallel. There is potentially some reuse if you have more circuits than cpus the pass managers will potentially get reused per worker, but the execution is serialized in those cases.

@Mergifyio backport stable/1.4

backport stable/1.4

✅ Backports have been created