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

QuantinuumJob response is sometimes JobStatus #29

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

WrathfulSpatula
Copy link

Summary

(Closes #27)

Over at https://github.com/SRI-International/QC-App-Oriented-Benchmarks, we're trying to use this provider to benchmark Quantinuum devices. However, adding the provider to that repository, it always raises an exception due to an unhandled case in the method status(), where one or more items in self._experiment_results is a JobStatus instance.

Following your logic in the same region of code, we extract the maximum information we can for the experiment_result, then we add the header and append to the results list otherwise exactly according to how it was already done.

This fixes our issue in the benchmarks repository, allowing us to get the results we expect.

Details and comments

For coverage: I can see that the original code leads to an exception and stack trace in https://github.com/SRI-International/QC-App-Oriented-Benchmarks, while the code is this branch works. I'm not sure how to encapsulate this in a unit test, but, apparently, the original code coverage didn't cover this logical case, of a JobStatus response.

@CLAassistant
Copy link

CLAassistant commented Jun 13, 2023

CLA assistant check
All committers have signed the CLA.

@vprusso
Copy link

vprusso commented Jun 13, 2023

Just want to mention that this PR is a continuation of the recent fix from #28. I've tested this on my end and would find this being merged in master a very valuable contribution!

fyi @1ucian0 / @mtreinish

Copy link
Collaborator

@mtreinish mtreinish 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 looks fine. But besides the lint job failure that needs to be fixed I just had one minor inline comment.

qiskit_quantinuum/quantinuumjob.py Outdated Show resolved Hide resolved
@vprusso
Copy link

vprusso commented Jun 28, 2023

Just following up on this PR. It looks like @WrathfulSpatula made the requested change asked by @mtreinish .

Are there any other outstanding issues you think would be worth addressing, or is this okay to be merged into master?

Thanks again!

@vprusso
Copy link

vprusso commented Aug 29, 2023

@1ucian0 @chris-theroux @mtreinish Any chance this can be merged into master?

@chris-theroux
Copy link
Collaborator

I don't have permissions to merge. Pinging @mtreinish

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.

Broken installation (both via PyPi and GitHub)
5 participants