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

Add env variable and user config option to suppress packaging warnings #3362

Merged
merged 12 commits into from
Mar 31, 2020

Conversation

ThePrez
Copy link
Contributor

@ThePrez ThePrez commented Oct 30, 2019

Summary

This PR adds a new env variable QISKIT_SUPPRESS_PACKAGING_WARNINGS and a
matching user config file option suppress_packaging_warnings which can be used to optionally
suppress the regular warnings about aer and/or ibmq-provider missing at import time. This strikes
a balance between telling users who potentially have a packaging issue that would otherwise go
by unnoticed and giving users who are intentionally (or unable to) install the components the ability to disable the warnings.

Details and comments

Some platforms (in my case IBM i) cannot reliably build newer versions qiskit-aer due to new dependencies, etc. In my case, I want to pursue qiskit use with only terra (and show others), but the constant RuntimeWarning is an annoyance easily (incorrectly) perceived as an error.

In short, I'd like a way to suppress that warning, and I think an environment variable is a cheap and easy way to do so.

Co-authored-by: Matthew Treinish mtreinish@kortar.org

@mtreinish
Copy link
Member

mtreinish commented Oct 30, 2019

I'm not opposed to having a flag to do this. But before we add it I'm curious does the example from the documentation at the end of the terra from source section (https://qiskit.org/documentation/contributing_to_qiskit.html#installing-terra-from-source work) for your use case?

That being said I think if we want to make this more easily configurable. You raise a good point about compiling aer, aer's build system makes it hard to build from source in a lot of environments. I think we should change the flag to be QISKIT_SUPPRESS_WARNINGS or QISKIT_SUPPRESS_PACKAGING_WARNINGS and apply it to ibmq too. We also should add the option to the qiskit user config file: https://github.com/Qiskit/qiskit-terra/blob/master/qiskit/user_config.py so people can set once.

@ThePrez
Copy link
Contributor Author

ThePrez commented Oct 30, 2019

Yes, that example code using the warning filter does address my use case. I'm happy with that since it is documented (I didn't want to blindly apply a warnings filter and didn't read the doc).

I do like both of your suggestions. QISKIT_SUPPRESS_PACKAGING_WARNINGS seems a good choice, and adding to the user_config certainly makes sense. Of course, I'm not sure how many people are out there building terra and aer from source. If I'm the only one who cares, we can table the idea. Or, I can clean it up and make the necessary changes.

@mtreinish
Copy link
Member

Yeah, I think it's worthwhile even if it's not for the majority of users I think that there is a big enough set of users where this matters.

I actually hit this personally because on one of my systems I'm fighting a packaging issue with ibmq and the import doesn't ever work which always emits a warning. So having a way to filter it easily outside of the calling script would be handy.

I think that this will make the developer experience a bit more friendly. The warnings were mostly added as a stopgap against frequent bug filings because of the fragility of python packaging, especially around namespace packages. But, that was a conscious tradeoff of annoying people with more specialized use cases. So having a builtin workaround for that is a good idea.

@ajavadia
Copy link
Member

@ThePrez is there any update on this?

@ThePrez
Copy link
Contributor Author

ThePrez commented Feb 17, 2020

Sorry. I haven't had time to circle back around and finish this up in other places. I probably won't be able to dig back into this for another couple weeks. We can let simmer, or we can merge the trivial changes in this PR and keep a work item open for the qiskit user config file and also ibmq if needed.

This commit continues where the previous one left off, it makes 2
changes to functionality of the option to suppressing the packaging
warnings. First, it changes the environment variable to be
QISKIT_SUPPRESS_PACKAGING_WARNINGS and applies it to both aer and ibmq.
Then it also adds an option for supressing these warnings to the user
config file. This will enable users to disable the warnings globally
locally without needing to ensure an env var is set. At the same time
the test jobs are updated so that we suppress the warnings there because
we don't install aer in ci (since it's not used).
@mtreinish mtreinish marked this pull request as ready for review March 19, 2020 13:55
@mtreinish mtreinish added this to the 0.13 milestone Mar 19, 2020
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Mar 19, 2020
@mtreinish
Copy link
Member

mtreinish commented Mar 19, 2020

@ThePrez I went ahead and updated this, it's been sitting idle for several months and we'd really like to get this feature in the next release. It has come up several times that using just filter warnings isn't flexible enough for several use cases.

@mtreinish mtreinish changed the title Add environment variable to suppress aer warning Add environment variable to suppress packaging warning Mar 19, 2020
@mtreinish mtreinish changed the title Add environment variable to suppress packaging warning Add environment variable to suppress packaging warnings Mar 19, 2020
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Is it worth specifying the behavior if the user provides conflicting config and an env values? (Right now, I think if either are 'Y'/'y'/True, the message will be suppressed, but the ENV variable could take precedence over the config value or vice versa. I don't know if there's precedent for other config values.)

qiskit/__init__.py Outdated Show resolved Hide resolved
mtreinish and others added 2 commits March 19, 2020 17:12
@mtreinish
Copy link
Member

That's a fair point, the env variable should take precedence over the user config file. We don't have it with env vars anywhere but it does exist for the parameters (like visualization backend)

@mtreinish
Copy link
Member

@kdk ok I updated the logic in 4f5f8ea the env var set to N will override the user config file setting

qiskit/__init__.py Outdated Show resolved Hide resolved
@mtreinish mtreinish changed the title Add environment variable to suppress packaging warnings Add env variable and user config option to suppress packaging warnings Mar 31, 2020
@kdk kdk added the automerge label Mar 31, 2020
@mergify mergify bot merged commit 9fdadff into Qiskit:master Mar 31, 2020
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
Qiskit#3362)

* Add environment variable to suppress aer warning

* Rename env var and add user config file option

This commit continues where the previous one left off, it makes 2
changes to functionality of the option to suppressing the packaging
warnings. First, it changes the environment variable to be
QISKIT_SUPPRESS_PACKAGING_WARNINGS and applies it to both aer and ibmq.
Then it also adds an option for supressing these warnings to the user
config file. This will enable users to disable the warnings globally
locally without needing to ensure an env var is set. At the same time
the test jobs are updated so that we suppress the warnings there because
we don't install aer in ci (since it's not used).

* Fix lint

* Update qiskit/__init__.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Let env var override user config file

* Adjust logic for printing warnings

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants