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

Properly check if connection can be established for importing interactive visualizations #892

Merged
merged 3 commits into from
Sep 13, 2018
Merged

Properly check if connection can be established for importing interactive visualizations #892

merged 3 commits into from
Sep 13, 2018

Conversation

nonhermitian
Copy link
Contributor

@nonhermitian nonhermitian commented Sep 13, 2018

Fix #895

Summary

As pointed out by @chriseclectic , loading qiskit with no internet connection results in a ConnectionError because the interactive visualizations max out the number of attempts to look for the needed host url. Here we replace the previous check with on that gracefully exits if no connection can be established.

Details and comments

@delapuente
Copy link
Contributor

delapuente commented Sep 13, 2018

Please, remember to file an issue and link the PR with it by adding "Fix <issue_number>".

@diego-plan9
Copy link
Member

Can you remember to remove requests as a dependency, as the only place where it was used is now replaced by a standard library call (which I think is a good move)?

@diego-plan9
Copy link
Member

Also, perhaps we could take the chance to reevaluate the need for the

from qiskit.tools.visualization import (circuit_drawer, plot_histogram)

line from the main qiskit/__init__.py, in the spirit of @ajavadia's comment at #890 (comment)?

Our top-level qiskit namespace and import qiskit is already more complicated and full of implications than ideal, and importing a member of qiskit.tools there entangles (no pun intended) it even more, which has been the root cause of two user-facing bugs (#895 and #889) already since it was introduced. Any thoughts?

@nonhermitian
Copy link
Contributor Author

Actually the bug fixed here was caught becuase of the top level visualization imports. They were not the cause. Anytime someone loaded the visualization module, the same issue would pop up.

@nonhermitian
Copy link
Contributor Author

The function added here could also be useful for having a more graceful error message if the register function cannot connect to the host urls.

Copy link
Member

@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 LGTM, I was going to fix it by just catching requests.exceptions.RequestException but switching to just checking if we can open a socket or not works fine too.

@nonhermitian nonhermitian merged commit fc221f8 into Qiskit:master Sep 13, 2018
lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019
…tive visualizations (Qiskit#892)

* check if has connection

* Removed requests and updated changelog
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.

Loading Qiskit with no internet results in a ConnectionError
4 participants