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

Squashed commit including basis update and proxy support. #11

Merged
merged 6 commits into from
Jul 29, 2020

Conversation

dlucchetti
Copy link
Contributor

Summary

  • Update allowed basis gates to enable submission.
  • Added proxy support for the backend

Details and comments

The set of basis gates was insufficient to allow submission to our system. By adding additional allowed basis gates we were able to get past the compilation and submit the QASM to our API.

Proxies are now supported with the limitation that they will revert from websockets to polling of the restful interface. For long-running/long-queued jobs this may impact the response time somewhat, but it should be limited to a small fraction of the overall wait-time.

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2020

CLA assistant check
All committers have signed the CLA.

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.

This looks good for the most part. Just a couple inline comments, but it's mostly about not removing 3.5 support yet (qiskit still supports it) and the syntax for proxy configs. I also think this needs documentation in the README about how the new proxy option is used

azure-pipelines.yml Outdated Show resolved Hide resolved
qiskit/providers/honeywell/credentials/__init__.py Outdated Show resolved Hide resolved
qiskit/providers/honeywell/api/rest/job.py Outdated Show resolved Hide resolved
@dlucchetti
Copy link
Contributor Author

Ready for merge. Please advise if more changes are necessary.

@dlucchetti
Copy link
Contributor Author

@mtreinish Any update on acceptance/merger? There is now another PR waiting after this one...

@dlucchetti dlucchetti requested a review from mtreinish July 15, 2020 23:06
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 apologize on the slow response time here, I have been busy with some other work the past couple of weeks. This looks better just some small comments inline.

The one big thing I want to understand better before merging this though is the basis gates change. There isn't a lot detail in the PR summary about why this was needed, is it actually the basis set of the backend or just a limit with the current terra release's transpiler?

qiskit/providers/honeywell/honeywelljob.py Outdated Show resolved Hide resolved
qiskit/providers/honeywell/credentials/__init__.py Outdated Show resolved Hide resolved
@dlucchetti dlucchetti requested a review from mtreinish July 21, 2020 16:53
@mtreinish mtreinish merged commit f7c4ef7 into qiskit-community:master Jul 29, 2020
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.

3 participants