Enable multi circuit submission in PennyLane IonQ, second attempt.

[radumarg]

Enable multi circuit submission. This is a second attempt to implement this functionality because the first implementation involved a lot of code duplication with respect to the PennyLane baseline code.

[codecov]

Codecov Report

Attention: Patch coverage is 80.48780% with 16 lines in your changes missing coverage. Please review.

Project coverage is 91.73%. Comparing base (e5beee9) to head (eae3bcb).
Report is 1 commits behind head on multi-circuit-submission.

Files with missing lines	Patch %	Lines
pennylane_ionq/device.py	80.24%	16 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           multi-circuit-submission     #121      +/-   ##
============================================================
- Coverage                     96.93%   91.73%   -5.21%     
============================================================
  Files                             5        5              
  Lines                           326      387      +61     
============================================================
+ Hits                            316      355      +39     
- Misses                           10       32      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Alex-Preciado]

Hi @radumarg , thank you for opening this new PR. @lillian542 will be reviewing it this week. In the meantime, could you please take a look at the formatting checks (black), CodeFactor, and codecov, as they’re currently not passing?

[lillian542]

Thanks for the submission, and for making the changes requested on the last PR! 🚀 This looks like a very useful addition to the plugin. I’ve left some initial feedback and suggestions, have a look and see what you think!

Wrt the tests, the CodeCov bot has indicated all the uncovered lines, and ideally let’s go for full test coverage - but there are some places that I would say are more significant than others. I’ve commented on a few spots where I think it’s particularly important we add more comprehensive tests (i.e. more thorough than just what will satisfy CodeCov in terms of line coverage).

I’ll give it another pass once comments are addressed and the test coverage is more complete :)

[radumarg]

@lillian542 The rework is complete. Review comments were implemented, tests were extended, black and code factor issues were fixed. Can you please make sure tests with requires_api flag are included in test code coverage? Those tests need an IonQ API key in the test virtual environment in order to run. Note that using this is not my invention, I just took as an example some pre-existing testing code :)

[lillian542]

Thanks @radumarg! I'll get started on another review 🚀

[lillian542]

Looking good 🎉 I left a couple more comments. At this point I'll find a second reviewer to have a look - we generally aim for 2 approvals for PRs adding new functionality.

[radumarg]

@lillian542 Thanks for reviewing this change. I left a few questions regarding the latest set of comments. Please have a look. Also I see that black formatter is failing but I am not sure why. When I run it locally it passes, I am not sure with what arguments is being run during integration tests.

[lillian542]

Also I see that black formatter is failing but I am not sure why. When I run it locally it passes

@radumarg, when I runblack -l 100 pennylane_ionq, it changes several split-line definitions into single lines. What version of black are you using? And is it possible that you are running it without the line length specification, and it's defaulting to something else?

[mudit2812]

Looks good. I mostly just suggested minor formatting/docs fixes. I agree with @lillian542 's comment about the generation of samples inside the estimate_probability and sample methods. We've resolved this concern for devices using the new API, but it would be a breaking change to use the current behaviour as per the old device API. For me, that is a blocker for an approval.

Other than that, the PR looks quite complete. The tests are very thorough. I'm happy to approve once the above comment and the test failures are resolved 🚀

[radumarg]

@lillian542 and @mudit2812 Review comments were implemented as requested. I have added requires_api to failing tests, hopefully this time the test will pass.

[mudit2812]

Looks like all the tests are passing now 😄 . I'm quite happy with the state of this PR, and am ready to approve once code coverage is fixed. We can ignore the coverage issue for the logger.debug line by adding a # pragma: no cover to that line.

[radumarg]

@mudit2812 I have added the pragma as suggested. Also, I have removed checks for BasisState, QubitStateVector and StatePrep because these operations are not supported by IonQ devices so there is no way to test those lines. Regarding the code coverage: this now should be complete, however the code coverage tool does not take into account tests with requires_api which require an IonQ API key in order to run. For example the code lines under "if self.tracker.active" are reported as uncovered but those lines are covered by this test: test_recording_when_pennylane_tracker_active The same applies for other code lines reported as uncovered.

[mudit2812]

Thanks, I'm approving, and I've reached out to the rest of the team to figure out the best strategy to address the coverage issues.

[lillian542]

Thanks for all your work here, this looks good! The tests aren’t running with the API token because this is on a fork of the repo - we’ll merge it to a new branch on the main repo to get those running.

I'm approving this before merging, it looks like everything is in order to me. If any issues come up once all the tests run, we’ll let you know; we can merge these changes to master as soon as the tests are passing with the API token on the new branch :)

[radumarg]

Thanks, please let me know in case more tests will be necessary.