Skip to content
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

Update for pyQuil v3 #215

Merged
merged 4 commits into from
Feb 5, 2022
Merged

Update for pyQuil v3 #215

merged 4 commits into from
Feb 5, 2022

Conversation

ameyer-rigetti
Copy link
Contributor

@ameyer-rigetti ameyer-rigetti commented Aug 9, 2021

Description

Update code and tests for pyQuil v3

Closes #213

Checklist

  • The above description motivates these changes.
  • There is a unit test that covers these changes.
  • The code respects the API separation discussed in .github/CONTRIBUTING.md.
  • Relevant references and equations are cited using our standard reference style.
  • All new and existing tests pass locally and on PR checks.
  • Parameters have type hints with PEP 484 syntax.
  • Functions and classes have useful sphinx-style docstrings.
  • (New Feature) The docs and example notebooks have been updated accordingly.
  • The changelog (docs/source/changes.rst) has a description of this change.

@ameyer-rigetti
Copy link
Contributor Author

ameyer-rigetti commented Aug 9, 2021

I'm not sure why, but Semaphore can't seem to find pyQuil v3.0.0 when running the pip install:

Collecting pyquil<4.0.0,>=3.0.0 (from -r requirements.txt (line 1))
  Could not find a version that satisfies the requirement pyquil<4.0.0,>=3.0.0 (from -r requirements.txt (line 1)) (from versions: 0.0.1, 0.0.2, 0.0.3, 0.1.0, 1.0.0, 1.1.1, 1.1.2, 1.1.3, 1.1.4, 1.2.0, 1.3.0, 1.3.1, 1.3.2, 1.4.0, 1.4.1, 1.4.2, 1.4.3, 1.4.4, 1.4.5, 1.4.6, 1.5.0, 1.5.1, 1.6.0, 1.6.1, 1.6.2, 1.6.3, 1.7.0, 1.8.0, 1.9.0, 2.0.0b1, 2.0.0b3, 2.0.0b4, 2.0.0b5, 2.0.0b6, 2.0.0b7, 2.0.0b8, 2.0.0, 2.1.0, 2.1.1, 2.2.0, 2.2.1, 2.3.0, 2.4.0, 2.5.0, 2.5.1, 2.5.2, 2.6.0, 2.7.0, 2.7.1, 2.7.2, 2.8.0, 2.9.0, 2.9.1, 2.10.0, 2.11.0, 2.12.0, 2.13.0, 2.14.0, 2.15.0, 2.16.0, 2.17.0, 2.18.0, 2.19.0, 2.20.0, 2.21.0, 2.22.0, 2.23.0, 2.23.1, 2.23.2, 2.24.0, 2.25.0, 2.26.0, 2.27.0, 2.28.0, 2.28.1, 2.28.2)
No matching distribution found for pyquil<4.0.0,>=3.0.0 (from -r requirements.txt (line 1))

Locally:
Tests pass (without --runslow). I've updated the slow tests too and will try to run them, though they're quite slow.

@ameyer-rigetti ameyer-rigetti marked this pull request as ready for review August 9, 2021 22:52
@ameyer-rigetti ameyer-rigetti requested a review from a team as a code owner August 9, 2021 22:52
@genos
Copy link

genos commented Aug 9, 2021

Assuming all the tests etc. pass (no CI here?) LGTM 👍

@ameyer-rigetti ameyer-rigetti force-pushed the upgrade-pyquil-v3 branch 2 times, most recently from c946c5a to 7aaa3f5 Compare August 9, 2021 23:02
@ameyer-rigetti
Copy link
Contributor Author

ameyer-rigetti commented Aug 9, 2021

Assuming all the tests etc. pass (no CI here?) LGTM 👍

@genos Thanks :) CI seems to run on Semaphore for this repo, but I don't know why it can't pick up the latest version of pyQuil...

Maybe someone who has access to the Semaphore account can take a peek?

Copy link

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Looks good to my untrained eyes 😅

"from pyquil.device import gates_in_isa\n",
"from pyquil.noise import decoherence_noise_with_asymmetric_ro, _decoherence_noise_model\n",
"# noise_model = decoherence_noise_with_asymmetric_ro(gates=gates_in_isa(qc.device.get_isa()), p00=0.92, p11=.87)\n",
"from pyquil.noise import decoherence_noise_with_asymmetric_ro, _decoherence_noise_model, _get_qvm_noise_supported_gates\n",
Copy link

Choose a reason for hiding this comment

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

Are those internal methods something that should be expose properly from pyQuil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 This is the only place I've seen these used, so I don't know if we should move them over yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great point. I'm thinking we should do a pyQuil v3 follow up for exposing some methods and processes publicly, where external dependencies use them. I've used some private methods in the Cirq integration.

Choose a reason for hiding this comment

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

Agree- a follow-up ticket after collecting all of the additional entry points sounds like the right approach.

CHANGELOG.md Show resolved Hide resolved
@ameyer-rigetti ameyer-rigetti force-pushed the upgrade-pyquil-v3 branch 2 times, most recently from 96d14bf to e7ca5bc Compare August 11, 2021 17:27
@ameyer-rigetti ameyer-rigetti force-pushed the upgrade-pyquil-v3 branch 2 times, most recently from 0c4f7f6 to ec01d05 Compare August 11, 2021 22:48
Copy link
Contributor

@erichulburd erichulburd left a comment

Choose a reason for hiding this comment

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

I think this is definitely a major version update. Otherwise, looks pretty ready to go.

docs/examples/graph-state.json Outdated Show resolved Hide resolved
"from pyquil.device import gates_in_isa\n",
"from pyquil.noise import decoherence_noise_with_asymmetric_ro, _decoherence_noise_model\n",
"# noise_model = decoherence_noise_with_asymmetric_ro(gates=gates_in_isa(qc.device.get_isa()), p00=0.92, p11=.87)\n",
"from pyquil.noise import decoherence_noise_with_asymmetric_ro, _decoherence_noise_model, _get_qvm_noise_supported_gates\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great point. I'm thinking we should do a pyQuil v3 follow up for exposing some methods and processes publicly, where external dependencies use them. I've used some private methods in the Cirq integration.

@@ -1 +1 @@
__version__ = "0.7.1"
__version__ = "0.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a major version update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, in the sense that this is still in beta 👍

Choose a reason for hiding this comment

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

Yep, 0.8.0 seems right to me.

@@ -0,0 +1,29 @@
name: Tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's replace Semaphore with GitHub Actions

@ameyer-rigetti ameyer-rigetti force-pushed the upgrade-pyquil-v3 branch 2 times, most recently from 8f160bb to 5b84388 Compare August 12, 2021 19:05
requirements.txt Outdated Show resolved Hide resolved
@ameyer-rigetti ameyer-rigetti force-pushed the upgrade-pyquil-v3 branch 4 times, most recently from 14ff4da to 1886937 Compare August 12, 2021 21:49
@ameyer-rigetti
Copy link
Contributor Author

ameyer-rigetti commented Aug 12, 2021

@mhodson-rigetti I'm not sure why, but this test falls slightly out of tolerance in CI, but not locally: https://github.com/rigetti/forest-benchmarking/pull/215/checks?check_run_id=3316861010#step:4:567

Any opposition to widening the assertion's atol a bit? Alternately, I could mark the test as xfail and create an issue for it. I'd hate for this to block this PR

@@ -705,4 +705,4 @@
},
"nbformat": 4,
"nbformat_minor": 2
}
}

Choose a reason for hiding this comment

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

No newline at EOF- would normally be a flake8 violation (is flake8 used in this repo?).

Copy link

Choose a reason for hiding this comment

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

Does flake8 check notebooks by default? Thought you needed some special addon for that.

Copy link

@mhodson-rigetti mhodson-rigetti left a comment

Choose a reason for hiding this comment

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

Source reviewed. Don't see anything out of place.

@mhodson-rigetti
Copy link

@ameyer-rigetti regarding the failing test, I checked locally and both the 1Q and 2Q tests there are non-deterministic. Despite setting the seed on the QAM, the results change with every run, and I can induce failures on both tests just by running them enough times. First question is- is the seed broken? If you go back to master, is the result consistent? In which case, pyquil v3 has a defect in terms of seeding the QVM runs. If the last release has the same issue, the only options are (a) re-run the CI until it goes green, and note the issue; with (b) mitigate the error by (i) widening to 3*decay_error in both tests and also potentially increasing the 2Q num_shots=50 and num_sequences_per_depth=30 as per the 1Q case (for a better fit).

@erichulburd
Copy link
Contributor

erichulburd commented Aug 16, 2021

@mhodson-rigetti I was able to replicate non-determinism on master. Here is an example failure:

>       np.testing.assert_allclose(expected_decay, observed_decay, atol=2.5 * decay_error)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=0.0553496
E       
E       Mismatched elements: 1 / 1 (100%)
E       Max absolute difference: 0.05631447
E       Max relative difference: 0.06576378
E        x: array(0.8)
E        y: array(0.856314)

A look at this test and it seems that there is some randomness generated outside the QVM, specifically in generate_rb_experiment_sequences. Passing random_seed=1 I got 20/20 green locally running:

for run in {1..20}; do pytest forest/benchmarking/tests/test_randomized_benchmarking.py::test_2q_general_pauli_noise; done

Let me know if you think we should also increase atol in the tests assertion.

Change is here: #217

@mhodson-rigetti
Copy link

@erichulburd don't think you need to modify atol, but recommend extending fixing the seed to the 1Q test case which has the same underlying issue (and I could make it fail by running enough times). See the sub-issue.

@kalzoo
Copy link
Contributor

kalzoo commented Feb 5, 2022

Finally merging this! Just had to be unhooked from Semaphore. Thank you to @mhodson-rigetti , @erichulburd , @dbanty and of course Andrew Meyer! 🥂

@kalzoo kalzoo merged commit 4c2c3bf into master Feb 5, 2022
@kalzoo kalzoo deleted the upgrade-pyquil-v3 branch February 5, 2022 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing pyquil.gate_matrices raises a warning
6 participants