Clarify the purpose of execute() function and deprecate transpile kwargs#7888
Clarify the purpose of execute() function and deprecate transpile kwargs#7888mtreinish wants to merge 9 commits into
Conversation
This commit updates the documentation of the execute() function to clarify its purpose and intent. The execute() function is provided as a higher level abstraction and convenience function for use cases where you just want to execute a circuit and not worry about how it gets compiled for a particular backend. The set of options the function is minimal by design because it should just be about running circuits with no options needed and if you need to exert more control over the compilation you should use transpile() and backend.run() together instead. As part of this a large number of legacy kwargs defined on the function are deprecated as they are mostly holdovers from before the purpose of the function was clear and really are just needless duplication of transpile(), schedule(), and assemble() (even though this isn't used anymore after Qiskit#7886). Closes Qiskit#7640
| DEPRECATED: Properties returned by a backend, including information on gate | ||
| errors, readout errors, qubit coherence times, etc. Find a backend | ||
| that provides this information with: | ||
| ``backend.properties()`` |
There was a problem hiding this comment.
Below this line, how come initial_layout got to stay non-deprecated? That feels like a transpiler option as well, perhaps?
There was a problem hiding this comment.
I was on the fence about it. It felt like maybe high level enough to say run this circuit on these qubits. But, I agree it's probably better to deprecate it here too and rely on transpile() if you need this.
| if shots is not None: | ||
| if not hasattr(backend.options, "shots"): | ||
| warnings.warn( | ||
| "The shots argument is deprecated as of Qiskit Terra 0.21.0, " | ||
| "and will be removed in a future release. The backend you are running on does " | ||
| "not support setting the number of shots so this option will have no effect.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
If the shots keyword is deprecated, I think we need the warning in both paths? It'll just have a different message. (Similar comment for memory and seed_simulator as well.)
On the other hand, shots may be one of the few options that could still be appropriate for execute.
edit: I just saw the extra comment in the release note about the first point here. In this case, perhaps we might want to make the distinction in the docstring as well, since at the moment the documentation looks the same for a completely deprecated argument.
There was a problem hiding this comment.
Sure, I'll update the docstring some more. I honestly just got annoyed/overwhelmed by all the options here (you might notice the deprecation warnings get a bit less detailed the further down you go).
There was a problem hiding this comment.
Yeah, I totally understand that - there's hundreds of them.
|
I suspect we may need to invest a bit of time updating tutorials and other documentation after this - I feel like we probably have a fair amount of extant code on qiskit.org that uses |
| if scheduling_method is not None: | ||
| warnings.warn( | ||
| "The scheduling_method argument is deprecated as of Qiskit Terra 0.21.0, " | ||
| "and will be removed in a future release. If you require scheduling your circuit " | ||
| "prior to execution you should use the :func:`~.schedule` function explicitly with " | ||
| "prior to running the circuit.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| if init_qubits is not None: | ||
| warnings.warn( | ||
| "The init_qubits argument is deprecated as of Qiskit Terra 0.21.0, " | ||
| "and will be removed in a future release. If you require scheduling your circuit " | ||
| "prior to execution you should use the :func:`~.schedule` function explicitly with " | ||
| "prior to running the circuit.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Users with several arguments in execute will a lot get a lot of warning (maybe flooding the terminal?). I suggest this style:
| if scheduling_method is not None: | |
| warnings.warn( | |
| "The scheduling_method argument is deprecated as of Qiskit Terra 0.21.0, " | |
| "and will be removed in a future release. If you require scheduling your circuit " | |
| "prior to execution you should use the :func:`~.schedule` function explicitly with " | |
| "prior to running the circuit.", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| if init_qubits is not None: | |
| warnings.warn( | |
| "The init_qubits argument is deprecated as of Qiskit Terra 0.21.0, " | |
| "and will be removed in a future release. If you require scheduling your circuit " | |
| "prior to execution you should use the :func:`~.schedule` function explicitly with " | |
| "prior to running the circuit.", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| if init_qubits is not None: | |
| deprecated_arg.append('init_qubits') | |
| if scheduling_method is not None: | |
| deprecated_arg.append('scheduling_method') | |
| if deprecated_arg: | |
| warnings.warn( | |
| "The following arguments are deprecated as of Qiskit Terra 0.21.0, " | |
| "and will be removed in a future release: {", ".join(deprecated_arg)}. If you require scheduling your circuit " | |
| "prior to execution you should use the :func:`~.schedule` function explicitly with " | |
| "prior to running the circuit.", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) |
kdk
left a comment
There was a problem hiding this comment.
I'm highly in favor of cleaning up the execute interface, but I think I still have some reservations about deprecating all the kwargs and directing users to use a combination of transpiler/schedule/backend.run. There are often cases where a user wants to set one option e.g. shots or initial_layout and this approach asks users to add more boilerplate to achieve the same end.
I wonder if there's a middle ground which might ease the transition. Something like allowing execute to accept {transpile,schedule,runtime}_options objects which can then be passed to their respective functions. This would help to remove the large number of kwargs on execute, help users understand the machinery behind execute and where those options will be applied, and will align better with the defined Options interfaces on the primitives.
This is actually a bit of the confusion with the current state of execute() imo.
Heh, the |
Heh, I actually need to remember my own PR (granted it has been ~8 months since I looked at it in detail). The options allowed that are passed through to |
Pull Request Test Coverage Report for Build 3953571855
💛 - Coveralls |
Summary
This commit updates the documentation of the execute() function to
clarify its purpose and intent. The execute() function is provided as a
higher level abstraction and convenience function for use cases where
you just want to execute a circuit and not worry about how it gets
compiled for a particular backend. The set of options the function is
minimal by design because it should just be about running circuits with
no options needed and if you need to exert more control over the
compilation you should use transpile() and backend.run() together
instead. As part of this a large number of legacy kwargs defined on the
function are deprecated as they are mostly holdovers from before the
purpose of the function was clear and really are just needless
duplication of transpile(), schedule(), and assemble() (even though this
isn't used anymore after #7886).
Details and comments
Closes #7640