-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Remove deprecated legacy pulse builder commands #11191
Remove deprecated legacy pulse builder commands #11191
Conversation
…ngs, transpiler_settings of qiskit.pulse.builderand modified related tests
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7127436955Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
releasenotes/notes/Deprecate_legacy_pulse_builder_command-47651b395940e687.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MozammilQ ! Could you please deprecate/update following places too?
- Module doc of
qiskit.pulse.builder.py
(example code with circuit elements) - Several args (default_transpiler_settings, default_circuit_scheduler_settings) in
qiskit.pulse.builder.build
function QuantumCircuit
type of thetarget
argument inqiskit.pulse.builder.call
function (you can use predicate feature ofdeprecate_arg
decorator). The example code 3 of function doc too.
qiskit/pulse/builder.py
Outdated
@@ -1197,6 +1197,7 @@ def _qubits_to_channels(*channels_or_qubits: Union[int, chans.Channel]) -> Set[c | |||
return channels | |||
|
|||
|
|||
@deprecate_func(since="1.0.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deprecate_func(since="1.0.0") | |
@deprecate_func(since="0.46.0") |
@mtreinish Is there any guide for deprecation? Do we need to open two PRs? One to https://github.com/Qiskit/qiskit/tree/stable/0.46 with deprecation warning and another one to main branch with code removal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you will need 2 PRs, one with the deprecation (to 0.46) and one with the removal (to 1.0 == main
), and the deprecation warnings should say "since 0.46" (as suggested by @nkanazawa1989). In case you are considering it, I think that there's no need to close and re-open it against stable/0.46
, we can merge it to main and backport it to 0.46. (now that I think about it, this might actually be the recommended course of action, because else we would have diverging histories, @mtreinish?). It would be a good idea to write this down and have a sort of "guide" we can refer people to, because I expect it to be confusing for many contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern we've been following is just opening a PR on stable/0.46
for the deprecation paired with a removal PR on main
for 1.0.0. This was mostly done to minimize CI time, it's only 2 CI runs instead of 3 to deprecate and remove. They can also be pushed in parallel without any dependency between PRs.
But, I wouldn't say we're committed to that as a general requirement, doing both a deprecation to be backported to stable/0.46
followed by an immediate removal PR to main would also work fine. The only complication is for backport you'd need to use a comment to manually tell mergify to backport to stable/0.46
. For example, leaving a comment like: @Mergifyio backport stable/0.46
after the PR merges. Using the stable backport potential tag in this case will trigger a backport to stable/0.45 which is incorrect for new deprecations.
In the interest of documenting this I pushed up #11205 to document the branching versioning strategy and all the general deprecation procedures around this. But I didn't put in specifics around how to handle PRs for this situation. I think we can updating CONTRIBUTING.md
after #11205 merges if we feel more guidance is needed on the specifics around this. In general backporting and stable branch policy will be getting more complicated moving forward as we support > 1 stable branch at a time (this is just the first example of it).
…eduler_settings in qiskit.pulse.builder.build and modified/refacotred relevant tests
…ilder.call function.
@nkanazawa1989 , please see if this is ok :) |
Thanks @MozammilQ the PR itself looks good to me. Unfortunately (fortunately?) we are now on bit tricky period for the first major version release. This means Since this PR targets the |
@nkanazawa1989 , I will first move the deprecation code which is currently in this PR to another PR against |
@nkanazawa1989 , PR #11249 , is against stable/0.46, |
…he relevant tests have been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MozammilQ I think you need to delete removed usage of builder command from the class and module docs, rather than adding warning message in this PR. I'll merge this PR once after docs errors are resolved.
@nkanazawa1989 , please see if this is good enough :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also remove self._circuit_scheduler_settings
and command? There is no circuit input to be scheduled.
…textmanager, altered release note
@nkanazawa1989 , please see if this is good enough :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MozammilQ all looks good! I'll merge the PR.
Summary
This PR removes deprecated logic for legacy pulse builder commands.
Deprecation is done in this PR #11249
This PR
Removes
qiskit.pulse.builder.call_gate
qiskit.pulse.builder.cx
qiskit.pulse.builder.u1
qiskit.pulse.builder.u2
qiskit.pulse.builder.u3
qiskit.pulse.builder.x
qiskit.pulse.builder.active_transpiler_settings
qiskit.pulse.builder.active_circuit_scheduler_settings
qiskit.pulse.builder.transpiler_settings
arguments
default_transpiler_settings
,and
default_circuit_scheduler_settings
ofqiskit.pulse.builder.build
function.QuantumCircuit
type of thetarget
argument inqiskit.pulse.builder.call
functionModifies:
qiskit.pulse.builder.py
(example code with circuit elements)qiskit.pulse.builder.call
and, modifies related tests in
Details and comments
Refer to Issue: #11152