Skip to content
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

Merged

Conversation

MozammilQ
Copy link
Contributor

@MozammilQ MozammilQ commented Nov 3, 2023

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 of
    qiskit.pulse.builder.build function.

  • QuantumCircuit type of the target argument in qiskit.pulse.builder.call function

Modifies:

  • Module doc of qiskit.pulse.builder.py (example code with circuit elements)
  • The example code 3 of function doc for qiskit.pulse.builder.call

and, modifies related tests in

  • test/python/pulse/test_builder.py
  • test/python/pulse/test_block.py
  • test/python/pulse/test_builder_v2.py
  • test/python/transpiler/test_calibrationbuilder.py

Details and comments

Refer to Issue: #11152

@MozammilQ MozammilQ requested review from eggerdj, wshanks and a team as code owners November 3, 2023 13:28
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 3, 2023
@qiskit-bot
Copy link
Collaborator

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:

  • @Qiskit/terra-core
  • @nkanazawa1989

@coveralls
Copy link

coveralls commented Nov 3, 2023

Pull Request Test Coverage Report for Build 7127436955

Warning: 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.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 879 unchanged lines in 94 files lost coverage.
  • Overall coverage increased (+0.5%) to 87.42%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/sabre_layout.rs 1 98.31%
crates/accelerate/src/stochastic_swap.rs 1 87.96%
crates/qasm2/src/error.rs 1 97.56%
crates/qasm2/src/lex.rs 1 92.17%
qiskit/circuit/classical/types/types.py 1 96.43%
qiskit/circuit/controlflow/control_flow.py 1 95.0%
qiskit/circuit/controlflow/for_loop.py 1 98.55%
qiskit/circuit/controlflow/while_loop.py 1 98.18%
qiskit/circuit/library/generalized_gates/pauli.py 1 91.18%
qiskit/circuit/library/generalized_gates/unitary.py 1 92.11%
Totals Coverage Status
Change from base Build 6745236401: 0.5%
Covered Lines: 59820
Relevant Lines: 68428

💛 - Coveralls

@nkanazawa1989 nkanazawa1989 added Changelog: Deprecation Include in "Deprecated" section of changelog mod: pulse Related to the Pulse module labels Nov 6, 2023
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a 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 the target argument in qiskit.pulse.builder.call function (you can use predicate feature of deprecate_arg decorator). The example code 3 of function doc too.

@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@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?

Copy link
Contributor

@ElePT ElePT Nov 6, 2023

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.

Copy link
Member

@mtreinish mtreinish Nov 6, 2023

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).

@nkanazawa1989 nkanazawa1989 self-assigned this Nov 6, 2023
@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , please see if this is ok :)

@nkanazawa1989
Copy link
Contributor

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 main branch doesn't accept code deprecation, and they should go to stable/0.46 branch instead. Guide is here: Qiskit/feedback#36

Since this PR targets the main branch, you can immediately remove all code to deprecate, and move the deprecation code you wrote for the PR to another PR targeting stable/0.46. If you feel this workflow is troublesome, I'll merge this PR as-is and take care of the rest of procedure. What do you prefer?

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , I will first move the deprecation code which is currently in this PR to another PR against stable/0.46,
and then, remove the deprecated code here in this PR.

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , PR #11249 , is against stable/0.46,
now, I will remove the deprecated code here, (against main)

@MozammilQ MozammilQ changed the title Deprecate legacy pulse builder command Remove deprecated legacy pulse builder commands Nov 16, 2023
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a 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 nkanazawa1989 added Changelog: Removal Include in the Removed section of the changelog and removed Changelog: Deprecation Include in "Deprecated" section of changelog labels Nov 20, 2023
@MozammilQ
Copy link
Contributor Author

MozammilQ commented Nov 20, 2023

@nkanazawa1989 , please see if this is good enough :)

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a 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.

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , please see if this is good enough :)

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a 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.

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Dec 8, 2023
Merged via the queue into Qiskit:main with commit ace6ba6 Dec 8, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Removal Include in the Removed section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: pulse Related to the Pulse module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants