Skip to content

Adds cirq-rigetti #4302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 32 commits into from
Jul 20, 2021
Merged

Conversation

erichulburd
Copy link
Contributor

@erichulburd erichulburd commented Jul 9, 2021

Adds integrations to the Rigetti QVM and Forest SDK in a new module: cirq-rigetti.
Includes integration testing with docker compose jobs and documentation.

RFC

@erichulburd erichulburd requested review from cduck, vtomole and a team as code owners July 9, 2021 00:54
@erichulburd erichulburd requested a review from mpharrigan July 9, 2021 00:54
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 9, 2021
@erichulburd erichulburd force-pushed the add_rigetti_integration branch from 249114a to e206be4 Compare July 9, 2021 00:58
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@balopat
Copy link
Contributor

balopat commented Jul 9, 2021

To be resolved:

  • The tests in cirq-rigetti/tests require running a couple of docker containers (as defined in docker-compose.test.yaml). Should I create a completely separate Github workflow to run those tests?

I was just thinking to add it as a job on the same ci.yml workflow. Is there a benefit of adding a separate workflow?

  • I notice that cirq-rigetti is the only package that contains pytest fixtures in its conftest.py. Is this any sort of anti-pattern in Cirq?

Hmmm....I wouldn't call it an antipattern, we don't really use pytest fixtures, instead rely on local functions to create test data and pytest.parametrize to inject combinations of test inputs. If a certain method is used as part of multiple tests, we write tests for those too in a dedicated testing package, e.g. cirq.testing in cirq-core. Having said that, the type of dependency injection that is used in conftest.py looks pretty neat, so I would be okay to have it - even think about adopting the pattern to simplify some of our more complicated mock test setups.

I'm going to get other's views on this too.

  • I am in the process of plugging this integration into a couple of existing Cirq notebooks (QAOA, VQE). Should I include those notebooks as examples in this repository or provide an external link to them?

We will need two docs similar to google, aqt, etc.:

  • docs/rigetti/access.ipynb
  • docs/tutorials/rigetti/getting_started.ipynb

I'm thinking about adding the "multi-platformness" to only one of the notebooks. It could be VQE, but let's discuss that.

  • I will polish the markdown documentation as we resolve these other items.

Sounds good.

Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the quil converters in cirq-core to here?

vtomole
vtomole previously requested changes Jul 9, 2021
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments.

Also we typically ask our implementors to create a Device instance that helps with validation of circuits upfront against allowed operations, and adjacency and scheduling constraints. For the adjacency constraints we might want to consider adding OctogonalQubit to cirq-core/cirq/devices, with two dimensions: octogon number (0 to n) and qubit location (0 to 7) - if I understand https://qcs.rigetti.com/qpus/ correctly.

@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@balopat
Copy link
Contributor

balopat commented Jul 9, 2021

For formatting errors, the easiest way to fix is check/format-incremental --apply and then commit.

@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@erichulburd erichulburd force-pushed the add_rigetti_integration branch from 3c0b5c2 to 64db623 Compare July 9, 2021 20:45
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@erichulburd erichulburd force-pushed the add_rigetti_integration branch from 64db623 to b065827 Compare July 9, 2021 21:55
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

six==1.16.0
sniffio==1.2.0
toml==0.10.2
typing-extensions==3.10.0.0
Copy link
Contributor

@balopat balopat Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way Cirq uses requirements.txt is a bit unorthodox - we should remove as much of the constraints as possible as we are using requirements.txt as the install_requires instruction in setup.py. This does mean that our continuous integration keeps changing as new versions are released of our dependencies, but it didn't cause too many problems just yet. See #3552 for detailed discussions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've dropped some dependencies (I see pyQuil itself was already a dependency for the Quil conversion) and loosened constraints to ~=. Let me know if that suffices of if there is something additional that'd facilitate management.

@balopat balopat self-assigned this Jul 9, 2021
@balopat balopat removed the request for review from mpharrigan July 9, 2021 23:11
@google-cla
Copy link

google-cla bot commented Jul 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@erichulburd
Copy link
Contributor Author

@vtomole

Should we move the quil converters in cirq-core to here?

I don't think so. Quil is now separate from Rigetti. See https://github.com/quil-lang/.

If anything, we'd want to break the Quil converters into their own separate package. Something I'd consider working on time permitting.

@erichulburd
Copy link
Contributor Author

@balopat

Also we typically ask our implementors to create a Device instance that helps with validation of circuits upfront against allowed operations, and adjacency and scheduling constraints. For the adjacency constraints we might want to consider adding OctogonalQubit to cirq-core/cirq/devices, with two dimensions: octogon number (0 to n) and qubit location (0 to 7) - if I understand https://qcs.rigetti.com/qpus/ correctly.

Yes, you are understanding correctly.

tldr; I deliberately did not implement a device as of yet because our stack is tied to the Quil compiler pretty closely. This enables users to execute circuits using an arbitrary gate set against a hardware device of another native gate set. Circuit decomposition is on the periphery of my competencies, so after an attempt at native Cirq decomposition and validation, I decided it best to not implement those devices quite yet.

As things stand, we have three circuit executors and I describe circuit validation below:

  • with_quilc_compilation_and_cirq_parameter_resolution (default) and with_quilc_parametric_compilation - if a circuit is topologically invalid, the Quil compiler may attempt to re-assign the qubits according to a default rewiring strategy. If the circuit contains operations that the hardware does not support, the Quil compiler will decompose those operations to native gate instructions. Note, in some circumstances, this may result in a compilation error during execution. After this compilation process, we pass the valid circuit to our api server for binary translation.
  • without_quilc_compilation - will attempt to translate the circuit Quil to binary without any gate decomposition or topological corrections. This will could result in an error from our servers when invoking execution.

The ergonomics here aren't quite as clean as Cirq's decomposition and validation, however, fairly certain the Quil compiler can decompose all of Cirq's gates to our native gate set.

@erichulburd
Copy link
Contributor Author

To be resolved:

  • The tests in cirq-rigetti/tests require running a couple of docker containers (as defined in docker-compose.test.yaml). Should I create a completely separate Github workflow to run those tests?

I was just thinking to add it as a job on the same ci.yml workflow. Is there a benefit of adding a separate workflow?

No, I misspoke and meant separate job. For now, I've added services to the pytest job in the ci workflow. Looks like that the repo blocks me from running that change without approval.

Also, I'm less familiar with github workflows. Looks like they don't expose a command on the service, so I just set the options. For example:

      quilc:
        image: rigetti/quilc
        ports:
          - 5555:5555
        options: >-
          --entrypoint './quilc -S'

Let me know if you're aware of a more straightforward way of executing a command.

  • I notice that cirq-rigetti is the only package that contains pytest fixtures in its conftest.py. Is this any sort of anti-pattern in Cirq?

Hmmm....I wouldn't call it an antipattern, we don't really use pytest fixtures, instead rely on local functions to create test data and pytest.parametrize to inject combinations of test inputs. If a certain method is used as part of multiple tests, we write tests for those too in a dedicated testing package, e.g. cirq.testing in cirq-core. Having said that, the type of dependency injection that is used in conftest.py looks pretty neat, so I would be okay to have it - even think about adopting the pattern to simplify some of our more complicated mock test setups.

I'm going to get other's views on this too.

  • I am in the process of plugging this integration into a couple of existing Cirq notebooks (QAOA, VQE). Should I include those notebooks as examples in this repository or provide an external link to them?

We will need two docs similar to google, aqt, etc.:

* docs/rigetti/access.ipynb

* docs/tutorials/rigetti/getting_started.ipynb

Ok I'm working on this.

I'm thinking about adding the "multi-platformness" to only one of the notebooks. It could be VQE, but let's discuss that.

  • I will polish the markdown documentation as we resolve these other items.

Sounds good.

@balopat
Copy link
Contributor

balopat commented Jul 10, 2021

Looks like that the repo blocks me from running that change without approval.

Yeah, it's quite annoying that they introduced this feature for first time users. After you have a commit in the repo it's automated. I'll look into whether I can turn this off...but probably it's an anti-spam measure, which is good.

I think you might be able to test your workflows if you raise a PR within your fork against your master branch. That will allow iterating without me having to press Approve on every commit.

@erichulburd erichulburd force-pushed the add_rigetti_integration branch from c1d491e to 1a26798 Compare July 10, 2021 01:20
@erichulburd erichulburd force-pushed the add_rigetti_integration branch from 5ba6c2f to a4cdfd0 Compare July 19, 2021 17:02
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits

@balopat balopat changed the title draft: integrate rigetti package Adds cirq-rigetti Jul 19, 2021
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (I fixed the last nits)

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 20, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 20, 2021
@CirqBot
Copy link
Collaborator

CirqBot commented Jul 20, 2021

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 20, 2021
@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 20, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 20, 2021
@CirqBot
Copy link
Collaborator

CirqBot commented Jul 20, 2021

Automerge cancelled: A required status check is not present.

Missing statuses: ['Build docs', 'Build protos', 'Changed files test', 'Coverage check', 'Doc test', 'Format check', 'Lint check', 'Misc check', 'Notebook formatting', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Type check', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage', 'cla/google']

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 20, 2021
@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 20, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 20, 2021
@CirqBot CirqBot merged commit 912110f into quantumlib:master Jul 20, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 20, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Adds integrations to the Rigetti QVM and Forest SDK in a new module: cirq-rigetti.
Includes integration testing with docker compose jobs and documentation.

[RFC](https://docs.google.com/document/d/16h5qe5Ljf4Cgws6C0PDJ2a2M7Z-KYKq7N_2iT9XV9Lo/edit#heading=h.mckr847qq60k)
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Adds integrations to the Rigetti QVM and Forest SDK in a new module: cirq-rigetti.
Includes integration testing with docker compose jobs and documentation.

[RFC](https://docs.google.com/document/d/16h5qe5Ljf4Cgws6C0PDJ2a2M7Z-KYKq7N_2iT9XV9Lo/edit#heading=h.mckr847qq60k)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants