search

rigetti / qiskit-rigetti

Qiskit provider serving Rigetti hardware & simulator backends.
Apache License 2.0
7 stars 7 forks source link

Added support for specifying the name of the simulator #30

Open AdamWRichardson opened 2 years ago

AdamWRichardson commented 2 years ago

A trivial change which allows the user to give a name directly to the get_simulator method rather than purely constructing the name on the number of qubits. This change is useful if you want to run a on Aspen-11-qvm for example which is well supported by PyQuil

dbanty commented 2 years ago

Thanks for the contribution! I left a couple notes I think we'll want to address before merging, but I'll go ahead and approve CI so the tests can start! Probably also need to use poetry update or similar to make sure we're testing on the new version of Qiskit.

AdamWRichardson commented 2 years ago

@dbanty I also suggest moving to tox rather than makefiles and relying on poetry, shall I submit a separate PR to do this?

dbanty commented 2 years ago

@dbanty I also suggest moving to tox rather than makefiles and relying on poetry, shall I submit a separate PR to do this?

We try to keep our project setup consistent across all projects of a given language—and since we use Makefile everywhere and not tox, we wouldn't want to make that change here.

However, we are always open to evolving our tooling across the stack. Do you have a link to a good summary of the benefits of tox over Makefile + Poetry? If so, I'll spend some time evaluating and bring it to the rest of the team for discussion. The main benefit I've seen touted previously is managing testing on multiple versions of Python—but GitHub Actions does that for us.

AdamWRichardson commented 2 years ago

@dbanty I also suggest moving to tox rather than makefiles and relying on poetry, shall I submit a separate PR to do this?

We try to keep our project setup consistent across all projects of a given language—and since we use Makefile everywhere and not tox, we wouldn't want to make that change here.

However, we are always open to evolving our tooling across the stack. Do you have a link to a good summary of the benefits of tox over Makefile + Poetry? If so, I'll spend some time evaluating and bring it to the rest of the team for discussion. The main benefit I've seen touted previously is managing testing on multiple versions of Python—but GitHub Actions does that for us.

Sure of course, if make is what you are used to then fair enough. That being said there are many advantages to using tox. Here's an article about it with setuptools but it covers a lot of good reasons for using it.

The TLDR is that tox will build the package as a separate step and then install only what it has built into a new virtualenv which ensures all dependencies are resolved properly and you're actually testing the package, not an editable install. Tox can also use poetry to build the package so there's no conflict there.

Tox can do much more than a standard makefile can do but if you use tox for testing, linting etc... you may as well use it for things like licenses and to have a clean target.

AdamWRichardson commented 2 years ago

Having a more consistent experience with pyquil sounds like generally a good thing—I assumed we had split out get_simulator to follow a common Qiskit pattern. Maybe we can just have get_backend work like pyquil's get_qc? Then folks could pass arbitrary qvm strings in there for more advanced usecases.

I am 100% behind this, the change I made was the bare minimum that I needed to continue but I was going to do a follow up PR with this in it. This is then conforms in the same way as qiskits get_backend method which is also a huge bonus. I think this change would also result in generally cleaner code

dbanty commented 2 years ago

I am 100% behind this, the change I made was the bare minimum that I needed to continue but I was going to do a follow up PR with this in it. This is then conforms in the same way as qiskits get_backend method which is also a huge bonus. I think this change would also result in generally cleaner code

For the sake of not breaking backwards compatibility now or in a follow-up, I think it'd be better to add the qvm support to get_backend (via get_qc) in one swoop (and leave get_simulator as-is for now). If we hold off on this PR in favor of waiting for one which puts this feature in get_backend, will that block your work?

AdamWRichardson commented 2 years ago

I am 100% behind this, the change I made was the bare minimum that I needed to continue but I was going to do a follow up PR with this in it. This is then conforms in the same way as qiskits get_backend method which is also a huge bonus. I think this change would also result in generally cleaner code

For the sake of not breaking backwards compatibility now or in a follow-up, I think it'd be better to add the qvm support to get_backend (via get_qc) in one swoop (and leave get_simulator as-is for now). If we hold off on this PR in favor of waiting for one which puts this feature in get_backend, will that block your work?

OK sure happy to do that. It doesn't block me since we can close the PR and keep my branch until the new version. I won't be able to do much work on this for maybe a week, possibly a little bit more. How would you like to proceed?

dbanty commented 2 years ago

OK sure happy to do that. It doesn't block me since we can close the PR and keep my branch until the new version. I won't be able to do much work on this for maybe a week, possibly a little bit more. How would you like to proceed?

Happy to just leave this open so the discussion is available in case anyone else comes looking. If someone else wants this functionality before you have time to finish it up then we may be able to take it over internally. Otherwise, I think this can pause until you have time to work on it again.

Thanks for all your work on this as well as all the suggestions!