Deprecate Pulse package and dependencies #13164
Conversation
eliarbel
added
Changelog: Deprecation
Pull Request Test Coverage Report for Build 11468453533Warning: 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 |
Added deprecations in `dag_circuit.rs` Added more deprecations in `builder.py` Fixed some deprecations comments
This commit adds `warnings.catch_warnings` blocks to some methods which are not directly deprecated but otherwise use classes or methods which are being deprecated. Adding this filter to specific methods which are used in common workflow, e.g. transpilation and for which we don't want pulse deprecations to be emitted if pulse is not used directly by the user.
|
I think this PR is ready for review now as it contains all the deprecations, unit-test assertions, warning filtering and release notes I thought we should have for deprecating the pulse package. A couple notes worth highlighting and getting feedback on:
|
There was a problem hiding this comment.
Thanks @eliarbel!! This was a very large task to tackle. I added a few suggestions regarding deprecation messages and the filtering strategy. As for the use of custom decorators to reduce code repetition, we don't commonly do it but I see how in this case they are handy. Technically, as long as they don't show up in the docs, they wouldn't be part of the documented interface, so we would be able to remove them in 2.0 without issues.
releasenotes/notes/deprecate-pulse-package-07a621be1db7fa30.yaml
Outdated
Show resolved
Hide resolved
| def ignore_pulse_deprecation_warnings(func): | ||
| """Ignore deprecation warnings emitted from the pulse package""" | ||
|
|
||
| @functools.wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings( | ||
| "ignore", category=DeprecationWarning, message="The (.*) ``qiskit.pulse" | ||
| ) | ||
| return func(*args, **kwargs) | ||
|
|
||
| return wrapper | ||
|
|
||
|
|
||
| def decorate_test_methods(decorator): | ||
| """Put a given decorator on all the decorated class methods whose name starts with `test_`""" | ||
|
|
||
| def cls_wrapper(cls): | ||
| for attr in dir(cls): | ||
| if attr.startswith("test_") and callable(object.__getattribute__(cls, attr)): | ||
| setattr(cls, attr, decorator(object.__getattribute__(cls, attr))) | ||
|
|
||
| return cls | ||
|
|
||
| return cls_wrapper |
There was a problem hiding this comment.
In my experience, we usually tackle these large deprecations adding filters to the qiskit test base class rather than defining a custom decorator for this:
Lines 171 to 177 in fbfe738
There was a problem hiding this comment.
There is a tradeoff here... It seems that the preferred way in Qiskit to handle deprecation warnings in testing is by using unittest.assertWarns and that QiskitTestCase-level filtering is meant to be used mainly for messages emitted by 3rd party packages or otherwise in very special circumstances like deprecating a key core element which makes the use of assertWarns really problematic.
In this case, I wanted to avoid putting assertWarns everywhere in test/python/pulse but still use it on deprecated functions outside of Pulse like QuantumCircuit.calibrations. So I took a hybrid approach where I used the @decorate_test_methods(ignore_pulse_deprecation_warnings) only in test/python/pulse and test/python/visualization_v2 which both are being deprecated as a whole, as well as on the calibration builder passes in test/python/transpiler which are being deprecated as well. I like this balance but I'm open to other views or ideas about how to achieve this in a different way.
BTW, FWIW, these decorators are useful as markers for things to be removed in 2.0.
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
|
Here is the result of a performance comparison of Qiskit benchmarks, showing only the change w.r.t the default ASV factor. There isn't a clear trend and I'm curious about what these changes mean really. Anyway for now I think we should wait a bit until the PR is stable and then rerun this and decide whether or not it's worthwhile to dive into this in more detail.
|
There was a problem hiding this comment.
Overall, the deprecation looks good, and I think it is in a mergeable state. The only missing point I think could be improved is my previous comment on adding a link to Qiskit Dynamics for the deprecated pulse dependencies. I suggested always mentioning the link for simplicity (even for dependencies that are not being ported to Dynamics), but of course having a dedicated decorator for the elements that are being ported would be even better. I don't think it's a deal breaker though, so I will approve, and can re-approve if you decide to implement the suggestion.
|
And regarding the benchmarks, I see what you mean, and in my experience the variability of the asv results is quite high, which makes certain improvements difficult to track (I have seen this with Unitary Synthesis, where the tests that didn't run it would also change quite a lot when benchmarking the PR). |
Thanks @ElePT . I have implemented your suggestion already but haven't pushed it yet since I'm waiting for input from @nkanazawa1989 and @DanPuzzuoli regarding the exact dependencies which should be moved to Dynamics. Putting the list here so we can discuss in a wider forum if needed:
|
The inner class `TestAddCalibration` in `test_transpiler.py::TestTranspilerParallel` calls a deprecated functionality. In an attempt to use `self.assertWarns` inside the inner class, a reference to the outer class was stored as a class attribute of `TestAddCalibration`. dill got confused as to how to serialize the outer class which derives from `QiskitTestCase`. So switched to using `warnings.catch_warnnigs` instead in the inner class to avoid this reference of the outer class in the serialized inner class.
- Use `QuantumCircuit._has_calibration_for()` when trying to obtain calibrations from a `QuantumCircuit` due to the deprecation of the `Pulse` package.
…Rust. (#13237) * Initial: Move the rest of the `BasisTranslator` to Rust. Fixes Port `BasisTranslator` to Rust #12246 This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.) Methodology: The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (#12292), `EquivalenceLibrary`(#12585) and the `BasisTranslator` methods `basis_search` (#12811) and `compose_transforms`(#13137). All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design. By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made. Changes: - Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation. - Convert the `target_basis` into a set manually from python before sending it into the Rust space. - Remove the exposure of `basis_search` and `compose_transforms` to python. - Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance. - Change inner method's visibility for `basis_search` and `compose_transform` modules in rust. - Expose the exception imports from `Target` to the `accelerate` crate. - Expose `DAGCircuit::copy_empty_like` to the rest of the crates. - Remove all of the unused imports in the Python-side `BasisTranslator`. Blockers: - [ ] #12811 * Fix: Redundancies with serialization methods - Remove extra copies of `target`, `target_basis`, `equiv_lib`, and `min_qubits`. - Remove unnecessary mutability in `apply_transforms` and `replace_node`. * Refactor: Remove `BasisTranslator` struct, use pymethod. - Using this method avoids the creation of a datastructure in rust and the overhead of deserializing rust structures which can be overly slow due to multiple cloning. With this update, since the `BasisTranslator` never mutates, it is better to not store anything in Rust. * Lint: Ignore too_many_args flag from clippy on `basis_translator::run()` * Fix: Remove redundant clone * Fix: Leverage using `unwrap_operation` when taking op_nodes from the dag. - Add function signature in `base_run`. * Update crates/accelerate/src/basis/basis_translator/mod.rs Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * Adapt to #13164 - Use `QuantumCircuit._has_calibration_for()` when trying to obtain calibrations from a `QuantumCircuit` due to the deprecation of the `Pulse` package. --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
This commit deprecates `inst_map` in `PassManagerConfig.__init__`. It should have been part of the pulse deprecation PR Qiskit#13164 but I somehow missed to covere it. We're already in our feature freeze period for version 1.3 but since this is not a new feature but rather an addendum to the pulse deprecation PR we should squeeze this in as well.
This commit deprecates `inst_map` in `PassManagerConfig.__init__`. It should have been part of the pulse deprecation PR #13164 but I somehow missed to covere it. We're already in our feature freeze period for version 1.3 but since this is not a new feature but rather an addendum to the pulse deprecation PR we should squeeze this in as well.
Summary
This PR deprecates the Pulse package and any non-pulse package code within Qiskit that uses it.
Closes #13063
Details and comments