Internally importing qiskit.providers.backend.Backend should not raise DeprecationWarning
#12906
Conversation
|
One or more of the following people are relevant to this code:
|
qiskit/providers/__init__.py
Outdated
|
|
||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings( | ||
| "ignore", | ||
| category=DeprecationWarning, | ||
| message=r"qiskit\.providers\.models.+", | ||
| module="qiskit", | ||
| ) | ||
| from qiskit.providers.backend import Backend |
There was a problem hiding this comment.
The fact that we're doing this means that the warning will not be firing correctly. If import qiskit.providers causes an import of qiskit.providers.models internally, then user code subsequently calling import qiskit.providers.models will not re-execute the module (since it'll already be in sys.modules), and therefore they'll not see the warning.
A correct fix to this should prevent the bad import from happening at all, unless the user specifically does the bad thing.
Pull Request Test Coverage Report for Build 10406261159Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
1ucian0
added
stable backport potential
|
@jakelishman I took a different approach, but also not sure if it is the right one. Have a look and let me know. |
|
Extended the PR because |
There was a problem hiding this comment.
It's good that you've been very specific with the warning filters, but in general, I don't think we should be using warnings.catch_warnings like this in library code. Seems like there's two cases we're currently using it:
- when we're calling deprecated functionality from a class that is itself deprecated
- when an eager import involves calling deprecated code
Imo, case 1 doesn't need suppression at all. The functionality is deprecated already; it's entirely expected that deprecated functions call other deprecated functions, and correctly configured stack levels in the warnings should mean that the deprecated code gets "blamed", not the user / downstream library. Using the filter makes the code slower to run, harder to read, and runs more risk of inadvertently suppressing a true warning, for (imo) no benefit - users won't see it, and downstream tests that need to test deprecated code paths should understand how to configure their test suite correctly to say "we expect to be blamed for this warning, and we understand that internal deprecation warnings will be issued from qiskit.providers.model" - it's a standard part of testing.
In case 2, in general, importing Qiskit should not use deprecated code at all - deprecated code should only run when specifically triggered by a user. If it's still necessary for us to run, it indicates we're not ready to be deprecating it. In rare cases where it remains necessary to exist in imports that don't directly touch deprecated code, we can use lazy attribute getters like module-level __getattr__ to only call the deprecated functionality if/when it's actually used by something.
Context-managers like catch_warnings are also inherently non-thread-safe, which means that any downstream libraries that use internal threading (like several of the primitives implementations) can cause breakages to the warning filters, such as permanent application of one of these catch filters, if we have them in library code.
qiskit/providers/backend.py
Outdated
| with warnings.catch_warnings(): | ||
| # BackendV1 is deprecated along qiskit.providers.models.backendstatus.BackendStatus | ||
| # They both need to be removed at the same time | ||
| warnings.filterwarnings( | ||
| "ignore", | ||
| category=DeprecationWarning, | ||
| message=r"qiskit\.providers\.models.+", | ||
| module="qiskit", | ||
| ) | ||
| from qiskit.providers.models.backendstatus import BackendStatus | ||
|
|
||
| return BackendStatus( | ||
| backend_name=self.name(), | ||
| backend_version="1", | ||
| operational=True, | ||
| pending_jobs=0, | ||
| status_msg="", | ||
| ) |
There was a problem hiding this comment.
I personally wouldn't put this warning catch in - it's expected that deprecated code will call other deprecated code, and the stacklevel of the BackendStatus warning should be set such that qiskit is "blamed" for the warning, which will mean it's suppressed for users by default (which is good - we want that). It would show up if all warnings are set to display, but that's expected behaviour.
| if typing.TYPE_CHECKING: | ||
| from qiskit.providers.models import PulseDefaults |
There was a problem hiding this comment.
TYPE_CHECKING guards are conventionally put up at the top with the rest of the imports.
…m for internal generic_backend_v2 usage
There was a problem hiding this comment.
I'm not fully convinced that it's a great idea for us to be switching our imports to the inner modules - I haven't had chance to check in full detail - but at this point in the release cycle, I'm happy to just do it and go ahead like this. The fix to the deprecation for GenericBackendV2 looks like the most important bit - we're now doing what we'd expect others to do, and defining "our own" calibration things, if that's something we're going to expose to users.
We ought to take a note to remove those fake calibrations from GenericBackendV2 in the future as well, if it's something we're generally dropping support for; GenericBackendV2 is supposed to the lowest common denominator backend support, so when pulse things are no longer considered part of that, we'll want to drop them there.
…ise `DeprecationWarning` (#12906) * better module deprecation * _pulse_library * __getattr__ * copy qiskit.qobj.PulseQobjInstruction and qiskit.qobj.PulseLibraryItem for internal generic_backend_v2 usage * lint * Aer models * skip * more generic catch * specific imports (cherry picked from commit 713ab38)
…ise `DeprecationWarning` (#12906) (#12965) * better module deprecation * _pulse_library * __getattr__ * copy qiskit.qobj.PulseQobjInstruction and qiskit.qobj.PulseLibraryItem for internal generic_backend_v2 usage * lint * Aer models * skip * more generic catch * specific imports (cherry picked from commit 713ab38) Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Fixes #12902