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

Avoid configuring pulse defaults in circuit assembly unless needed #7047

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

mtreinish
Copy link
Member

Summary

This commit addresses a performance regression in assemble() introduced
in #6167. That PR added the ability for users to configure pulse
properties via assemble kwargs (such as lo frequencies). While this
should really only be exposed via a Backend's options object and terra
shouldn't have to deal with it, for legacy/backwards compat reasons we
need to expose these options via assemble(). However, that PR was a bit
aggressive in when it needed to parse the pulse defaults and properties
to determine if input was valid. The intent there was for circuits jobs
that users were configuring pulse configuration we'd be providing the
same checks as with pulse jobs. However for circuits jobs this
additional validation is only needed if the user has specified a pulse
configuration options and we need to check that they're valid. If a user
doesn't we shouldn't be touching anything pulse, especially as just
loading pulse defaults can result in a large performance regression
(roughly 2 orders of magnitude over assembly without it). To fix this is
commit adjusts the logic when we parse the pulse defaults (and all the
subsequent logic that uses them) to be if it's a pulse job, or if it's a
circuit job and one of the named pulse paramters has been set. This
returns the performance for pure circuits job to what it was prior to
PR #6167 but still retains the functionality that #6167 provided when
it's needed.

Details and comments

This commit addresses a performance regression in assemble() introduced
in Qiskit#6167. That PR added the ability for users to configure pulse
properties via assemble kwargs (such as lo frequencies). While this
should really only be exposed via a Backend's options object and terra
shouldn't have to deal with it, for legacy/backwards compat reasons we
need to expose these options via assemble(). However, that PR was a bit
aggressive in when it needed to parse the pulse defaults and properties
to determine if input was valid. The intent there was for circuits jobs
that users were configuring pulse configuration we'd be providing the
same checks as with pulse jobs. However for circuits jobs this
additional validation is only needed if the user has specified a pulse
configuration options and we need to check that they're valid. If a user
doesn't we shouldn't be touching anything pulse, especially as just
loading pulse defaults can result in a large performance regression
(roughly 2 orders of magnitude over assembly without it). To fix this is
commit adjusts the logic when we parse the pulse defaults (and all the
subsequent logic that uses them) to be if it's a pulse job, or if it's a
circuit job and one of the named pulse paramters has been set. This
returns the performance for pure circuits job to what it was prior to
PR Qiskit#6167 but still retains the functionality that Qiskit#6167 provided when
it's needed.
@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Sep 20, 2021
@mtreinish mtreinish added this to the 0.19 milestone Sep 20, 2021
@mtreinish mtreinish requested a review from a team as a code owner September 20, 2021 15:38
@mtreinish
Copy link
Member Author

Running:

    import time

    import qiskit
    from qiskit.circuit.library import QuantumVolume
    from qiskit.test.mock import FakeLegacyMontreal

    qubits = 7
    shots = 8192
    memory = False
    pulse = False
    circuits = 1
    backend = FakeLegacyMontreal()
    # Pre-cache propertie
    backend.properties()
    qv_circuit = QuantumVolume(qubits, seed=42)
    qv_circuit.measure_all()
    circs = qv_circuit
    experiments = qiskit.transpile(circs, backend=backend, seed_transpiler=42)
    assemble_start = time.time()
    qobj = qiskit.assemble(experiments, backend=backend, shots=shots, memory=memory)
    print(time.time() - assemble_start)

The assemble runtime goes from: ~0.36427 seconds without this PR to ~0.00523 seconds with this PR

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mtreinish mtreinish merged commit 9ffd69b into Qiskit:main Sep 20, 2021
@mtreinish mtreinish deleted the fix-assemble-regression branch September 20, 2021 17:45
mergify bot pushed a commit that referenced this pull request Sep 20, 2021
…7047)

* Avoid configuring pulse defaults in circuit assembly unless needed

This commit addresses a performance regression in assemble() introduced
in #6167. That PR added the ability for users to configure pulse
properties via assemble kwargs (such as lo frequencies). While this
should really only be exposed via a Backend's options object and terra
shouldn't have to deal with it, for legacy/backwards compat reasons we
need to expose these options via assemble(). However, that PR was a bit
aggressive in when it needed to parse the pulse defaults and properties
to determine if input was valid. The intent there was for circuits jobs
that users were configuring pulse configuration we'd be providing the
same checks as with pulse jobs. However for circuits jobs this
additional validation is only needed if the user has specified a pulse
configuration options and we need to check that they're valid. If a user
doesn't we shouldn't be touching anything pulse, especially as just
loading pulse defaults can result in a large performance regression
(roughly 2 orders of magnitude over assembly without it). To fix this is
commit adjusts the logic when we parse the pulse defaults (and all the
subsequent logic that uses them) to be if it's a pulse job, or if it's a
circuit job and one of the named pulse paramters has been set. This
returns the performance for pure circuits job to what it was prior to
PR #6167 but still retains the functionality that #6167 provided when
it's needed.

* Make expected logic more explicit

(cherry picked from commit 9ffd69b)
mergify bot added a commit that referenced this pull request Sep 21, 2021
…7047) (#7049)

* Avoid configuring pulse defaults in circuit assembly unless needed

This commit addresses a performance regression in assemble() introduced
in #6167. That PR added the ability for users to configure pulse
properties via assemble kwargs (such as lo frequencies). While this
should really only be exposed via a Backend's options object and terra
shouldn't have to deal with it, for legacy/backwards compat reasons we
need to expose these options via assemble(). However, that PR was a bit
aggressive in when it needed to parse the pulse defaults and properties
to determine if input was valid. The intent there was for circuits jobs
that users were configuring pulse configuration we'd be providing the
same checks as with pulse jobs. However for circuits jobs this
additional validation is only needed if the user has specified a pulse
configuration options and we need to check that they're valid. If a user
doesn't we shouldn't be touching anything pulse, especially as just
loading pulse defaults can result in a large performance regression
(roughly 2 orders of magnitude over assembly without it). To fix this is
commit adjusts the logic when we parse the pulse defaults (and all the
subsequent logic that uses them) to be if it's a pulse job, or if it's a
circuit job and one of the named pulse paramters has been set. This
returns the performance for pure circuits job to what it was prior to
PR #6167 but still retains the functionality that #6167 provided when
it's needed.

* Make expected logic more explicit

(cherry picked from commit 9ffd69b)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog performance stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants