Qiskit / qiskit-ibmq-provider

Qiskit Provider for accessing the quantum devices and simulators at IBM Quantum.
Apache License 2.0
243 stars 143 forks source link

Change package warning message to FutureWarning #1157

Closed HuangJunye closed 1 year ago

HuangJunye commented 1 year ago

Summary

https://github.com/Qiskit/qiskit-ibmq-provider/pull/1140 added deprecation warning message to the package. However, we realized later that DeprecationWarning category is ignored by default. In order to show the deprecation warning message and migration guides details, we should change the warning to FutureWarning which is intended for end users and is not ignored by default.

Details and comments

kt474 commented 1 year ago

LGTM - not sure why the docs build is failing, seems like an unrelated error. Taking a look

kt474 commented 1 year ago

@jakelishman @mtreinish the docs build doesn't like having FutureWarning - would it be ok to remove the -W in sphinx-build -W?

Also, since https://github.com/Qiskit/qiskit/pull/1661 will require another release (0.20.1), we'll need to update the metapackage version again.

jakelishman commented 1 year ago

Deprecation warnings aren't ignored by default overall, they're just ignored if the "triggering" code isn't adjudged to be the user's code. Do you have an example of where the deprecation is failing to trigger for user code?

jakelishman commented 1 year ago

For example, with no change to the default warning filters:

Screenshot 2023-02-09 at 17 54 39

jakelishman commented 1 year ago

Updating the metapackage version isn't an issue at all, no worries. Terra will release a 0.23.2 in the near future too.

kt474 commented 1 year ago

Deprecation warnings aren't ignored by default overall, they're just ignored if the "triggering" code isn't adjudged to be the user's code. Do you have an example of where the deprecation is failing to trigger for user code?

I don't have an example - the deprecation should always trigger. I think the issue is that tox treats the FutureWarning as an error which is causing the docs build to fail

Warning, treated as error:
Cell printed to stderr:
/Users/kt/Documents/qiskit-ibmq-provider/qiskit/providers/ibmq/__init__.py:101: FutureWarning: The package qiskit.providers.ibmq is being deprecated. Please see https://ibm.biz/provider_migration_guide to get instructions on how to migrate to qiskit-ibm-provider (https://github.com/Qiskit/qiskit-ibm-provider) and qiskit-ibm-runtime (https://github.com/Qiskit/qiskit-ibm-runtime).
  warnings.warn(
jakelishman commented 1 year ago

Yeah sorry, my question was for Junye: why are we changing from DeprecationWarning to FutureWarning? DeprecationWarning is the right category for me; the users we're trying to warn are Python developers. FutureWarning is for consumers of applications written in Python, but Qiskit's direct users aren't that, because Qiskit isn't an application.

(I think we also sometimes use FutureWarning to mean "this thing is going to change without being deprecated".)

HuangJunye commented 1 year ago

Yeah sorry, my question was for Junye: why are we changing from DeprecationWarning to FutureWarning? DeprecationWarning is the right category for me; the users we're trying to warn are Python developers. FutureWarning is for consumers of applications written in Python, but Qiskit's direct users aren't that, because Qiskit isn't an application.

(I think we also sometimes use FutureWarning to mean "this thing is going to change without being deprecated".)

@jakelishman Thanks for the comments and verifying the warnings. We found out this issue while hacking together with @1ucian0 and we were puzzled too as why DeprecationWarning didn't work. Now I double checked and I think I found the issue.

Just to be clear, there are two DeprecationWarning here. One is from qiskit-terra, deprecating the qiskit.IBMQ entry point with message:

DeprecationWarning: The qiskit.IBMQ entrypoint and the qiskit-ibmq-provider package (accessible from 
'qiskit.providers.ibmq`) are deprecated and will be removed in a future release. Instead you should use
the qiskit-ibm-provider package which is accessible from 'qiskit_ibm_provider'. You can install it with
'pip install qiskit_ibm_provider'. Just replace 'qiskit.IBMQ' with 'qiskit_ibm_provider.IBMProvider'

And the other is from qiskit-ibmq-provider with message:

DeprecationWarning: The package qiskit.providers.ibmq is being deprecated. Please see
https://ibm.biz/provider_migration_guide to get instructions on how to migrate to 
qiskit-ibm-provider (https://github.com/Qiskit/qiskit-ibm-provider) and 
qiskit-ibm-runtime (https://github.com/Qiskit/qiskit-ibm-runtime).

We were using __qiskit_version__ to check the sub package versions and the warning from qiskit-ibmq-provider was swallowed by that.

from qiskit import __qiskit_version__
from qiskit import IBMQ

print(__qiskit_version__)
IBMQ.load_account()

Only shows the deprecation warning from qiskit-terra not qiskit-ibmq-provider. But when we change DepercationWarning to FutureWarning it triggered the message. This observation plus the warning module documentation made us we think we have been using DeprecationWarning wrongly all this time.

But now I verify that the warnings are triggered every time in a new ipython session.

from qiskit import IBMQ

IBMQ.load_account()

With that, I think we should close the PR. Sorry for the confusion!

HuangJunye commented 1 year ago

Reopen it, because @1ucian0 may still have some inputs here.

jakelishman commented 1 year ago

It's a little sub-optimal that __qiskit_version__ can be the trigger for the import (and thus swallow the deprecation warning if the user later imports qiskit-ibmq-provider), but it's correct that no warning is shown by __qiskit_version__ - that's Terra checking what's installed, not the user attempting the import.

The chance of somebody opening a Python session, first printing out __qiskit_version__ and only then import qiskit-ibmq-provider I think is super rare, and not something we need to worry much about. DeprecationWarning is the right warning, and qiskit.__qiskit_version__ shouldn't warn.