Deprecation of algorithm utils (algorithm_globals and validation)
#10905
Conversation
|
One or more of the the following people are requested to review this:
|
| the release date. These have been migrated to the ``qiskit_algorithms`` standalone | ||
| `library <https://github.com/qiskit-community/qiskit-algorithms>`_, and can instead be | ||
| imported from ``qiskit_algorithms.utils``. | ||
|
|
There was a problem hiding this comment.
Should we add some note that says if you are still using the deprecated qiskit.algorithms, which themselves use the qiskit.utils. algorithm_globals you should continue to use this, for instance setting random_seed where the algorithm uses random function. Recommending also that users migrate to using qiskit_algorithms whose random function is seeded by the qiskit_algorithm_utils.algorithm_globals there? These were always more utilities used by the algorithms, where you could use them yourself with some custom algorithm, but where users mainly interacted was with the setters there to seed the rng, and other global settings. Maybe it should be noted that only the random aspect of algorithms globals exists in qiskit_algorithms - the other, more legacy settings, were not used/needed by the primitive based algos migrated to qiskit_algorithms.
There was a problem hiding this comment.
I have added a more detailed reno and specific deprecation messages. I was trying to keep it concise but then it became hard to convey all the nuances of this move.
releasenotes/notes/deprecate-algorithm-utils-6cdc5856015a7e65.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The deprecation of the algorithm_globals looks generally fine to me, but the rest of the library code shouldn't typically suppress the warnings raised by it. It's completely expected that deprecated code will call other deprecated code that's scheduled for removal in the same release, and we can't reasonably protect every access; it slows down all call sites, and the way it's being done here is a bit overly generous, so may suppress true deprecation warnings that we need to know about.
All the catch_warnings stuff in qiskit.algorithms and qiskit.opflow can go; those already call deprecated code (themselves) all over the place, so there's no need.
If it's too much work to go back through and undo it now, it's not too big a deal.
| power = algorithm_globals.random.integers(power) | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings("ignore", category=DeprecationWarning) | ||
| power = algorithm_globals.random.integers(power) |
There was a problem hiding this comment.
We don't need to (and mostly shouldn't) do this in deprecated library code. It's ok for the deprecated code to call other deprecated code, and the stacklevel should be set on algorithms_global correctly such that this file gets the "blame", which makes the warning invisible to users.
There was a problem hiding this comment.
While it was observed that catching warnings from using deprecated code in already deprecated code was not needed, since it has already been done and would take some effort to redo, the consensus was to go with things as-is since the whole set of changed code will be be removed anyway once algorithms and opflow are removed. There is already an issue open #10991 to remove this code (algorithms and opflow) post 0.45rc. With this in mind, it fulfils the issue to deprecate the 2 utils modules that were overlooked when algorithms folder was deprecated/migrated so lets go with this PR as-is. Approved...
| @deprecate_func( | ||
| additional_msg=( | ||
| "This algorithm utility has been migrated to an independent package: " | ||
| "https://github.com/qiskit-community/qiskit-algorithms. You can run " | ||
| "``pip install qiskit_algorithms`` and import ``from qiskit_algorithms.utils`` instead. " | ||
| ), | ||
| since="0.45.0", | ||
| ) | ||
| def __init__(self) -> None: | ||
| self._random_seed = None # type: Optional[int] | ||
| self._num_processes = QiskitAlgorithmGlobals.CPU_COUNT |
There was a problem hiding this comment.
This particular part of the deprecation shouldn't ever actually be triggered against users, because users shouldn't be instantiating the class - only the library does that. If anything, the algorithms_global deprecation might have been a little neater having been done using module-level __getattr__ both here and in utils/__init__.py, but let's not change that now.
The reason I mention it is that the current system causes import qiskit.algorithms to emit 3 additional warnings - these are correctly blamed on library code so won't be visible to users, but it's indicative of the same situation we had with opflow where it turned out that downstream projects testing with all warnings as errors struggled to understand the opflow ones because of excessive errors emitted during the import.
There was a problem hiding this comment.
I don't think this will have the same trouble for downstream projects, because opflow was more commonly imported in downstream classes (mostly for type hints for the primitives!) than algorithms, but it's just something to be aware of if we have problems during the rc period.
cc: @1ucian0 since you're likely to be a point of contact if people have trouble.
Summary
This is one of the last steps of the algorithms deprecation process (fixes #10516), it deals with utils that were (almost) exclusively used in algorithms. The only exception is the use of
algorithm_globalsinQDriftto allow fixing the seed of the sampling process (probably exclusively for testing purposes). This functionality has been replaced with a newseedinput argument in theQDriftinitializer.Details and comments
Because
algorithm_globalsis intended to be used "method-by-method", the deprecation warning added to theAlgorithmGlobalsinit is never raised during the usual workflow. To ensure that the deprecation is properly communicated I've had to add warnings to all of the properties of the class.