mtreinish / bqskit-qiskit-synthesis-plugin

A PoC demonstration of the Qiskit unitary synthesis plugin interface using bqskit
Apache License 2.0
4 stars 1 forks source link

Out-of-date BQSKit #1

Open edyounis opened 2 years ago

edyounis commented 2 years ago

I think it is very cool how Qiskit now supports synthesis plugins, and they seem very easy to implement. However, the version of BQSKit being used here is slightly out-of-date from the one on PyPI now. We have since released a full version of BQSKit. This is a completely new software infrastructure that has unified all of our synthesis research into a full-scale circuit optimization engine. If you are interested in learning more, you can check out our documentation here.

I am more than happy to make a PR to the plugin to have it working again, but before I do that I wanted to raise one concern. Our BQSKit infrastructure now uses Dask to parallelize circuit optimization and unitary synthesis. Will this be an issue with Qiskit's plugin framework? Should I keep everything running in serial for that?

mtreinish commented 2 years ago

This repo is actually completely out of date, the terra component is out of date too as there were many changes made prior to the plugin interface getting merged. I originally created this repo as just a demonstration to show people how https://github.com/Qiskit/qiskit-terra/pull/6124 could be used in practice. After the next qiskit-terra release (which will include the plugin interface) I was actually planning to reach out to you about how we can integrate BQSKit with the plugin interface. I think it should be fairly straightforward to include a plugin directly in BQSKit as it should be trivial to add a plugin class and add the entrypoint to the package metadata.

As for dask usage I think that it's fine to include in the synthesis plugin just expects to pass a unitary matrix to run() and have it return a DAGCircuit object representing the synthesized circuit how that happens under the covers doesn't actually matter. The only potential issues I can see with it are that the plugin interface doesn't have a provision for users to provide configuration directly to synthesis plugins as the plugins are stateless objects so if you need to add configuration for dask (for example to set a cluster config to dispatch to multiple nodes) you'd have to do that out of band (probably with a global or something ugly like that). The second is that a qiskit transpilation potentially runs in multiple processes if >1 circuit is being processed at once (this is platform dependent though so not in every case) which is just something to keep in mind if you're using dask to execute things in parallel locally.

edyounis commented 2 years ago

Yeah, I would be more than happy to have that discussion when the time comes.

Is it possible to have a parameter in **options that would let the synthesis tool know if Qiskit is running in parallel? This way we could run in serial when Qiskit is parallel so we don't oversubscribe.

This might not be the place to have this discussion, but it maybe helpful to have a synthesis_options parameter added to UnitarySynthesis or transpile, i.e.:

tqc = transpile(qc, backend, unitary_synthesis_method='bqskit', synthesis_options={'dask_options': dask_options})

This could potentially be useful for all synthesis plugins, as I imagine most will not be completely optionless.

mtreinish commented 2 years ago

There is an env variable set QISKIT_IN_PARALLEL which will be set to "TRUE" when things are running in parallel (this is used to prevent the same problem from nested parallel execution. Since they're multiple processes it's not easy to do this in process because of how things get split and sent to the child processes (from the pass perspective it doesn't see whether it's in a child process or not).

As for the options dictionary there isn't anything like that now, but the other plugin that people are working on I know about had a similar request. I was trying to avoid a free form field like that because it makes documenting it and the expected input hard to discover (the other fields are all well defined). But I think you're right it might be a necessity for all the plugins to have some kind of tunables. I'll try to throw together a patch before the release and I'll ping you there when it's up to make sure it'll work for your use case.