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

Native symengine serialization for QPY generation #8232

Conversation

nkanazawa1989
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 commented Jun 24, 2022

use native symengine serializer and deserializer which is implemented in C++.
see symengine/symengine.py#409

Summary

In QPY version 5 and below, symengine expression is serialized through sympy string parsing mechanism. The conversion from symengine to sympy, and parsing of expression is significant overhead, especially when a program contains multiple ParameterExpressions, or symbolic pulses containing complicated waveform equations.

Details and comments

Benchmark shows significant performance improvement. Here I chose an example of schedule with Drag pulse, which contains the most complicated envelope equation in current pulse library. In experimental programs, this pulse instance repeatedly appears, since DRAG is the most popular implementation of single qubit gates. In addition, here the pulse parameters are also parameter expression, i.e. symengine expression.

from qiskit import pulse, circuit

duration = circuit.Parameter("duration")
amp = circuit.Parameter("amp")
beta = circuit.Parameter("beta")
with pulse.build() as sched:
    pulse.play(pulse.Drag(duration, amp, duration/4, beta), pulse.DriveChannel(0))

# ---- benchmark ----

with BytesIO() as file:
    qpy.dump(sched, file)
    bout = file.getvalue()

with BytesIO(bout) as file:
    sched_load = qpy.load(file)[0]

# ---- benchmark ----

assert sched == sched_load

In QPY Version 5

image

As expected, _dumps_symbolic_expr consumes most of the execution time.

In this PR branch (Version 6)

image

Now the regex search introduced in 5a6ec94 becomes a significant overhead (this pattern should be precompiled, but this is outside the scope of this PR).

Magnifying the expression generation part

image

You can see _write_symbolic_pulse becomes significantly faster (x290 performance gain, if I remove zlib compression this becomes more faster).

Note

This is on hold until symengine/symengine.py#409 (comment) is closed. Currently symengine deserializer doesn't support complex double expressions, which is quite important to write pulse amplitude in the phasor representation.

amp = circuit.Parameter("amp")
angle = circuit.Parameter("angle")

phasor_amp = amp * np.exp(1j * angle)

use native symengine serializer and deserializer which is implemented in C++.
see symengine/symengine.py#409
@nkanazawa1989 nkanazawa1989 added on hold Can not fix yet Changelog: API Change Include in the "Changed" section of the changelog mod: qpy Related to QPY serialization labels Jun 24, 2022
@nkanazawa1989 nkanazawa1989 requested a review from a team as a code owner June 24, 2022 08:54
@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:

mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Jun 24, 2022
As pointed out by @nkanazawa1989 in Qiskit#8232 the regex construction added
to the QPY interface functions in Qiskit#8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.
mergify bot added a commit that referenced this pull request Jun 24, 2022
* Move QPY version regex construction to import time

As pointed out by @nkanazawa1989 in #8232 the regex construction added
to the QPY interface functions in #8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.

* Fix regex flags usage

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Jun 24, 2022
* Move QPY version regex construction to import time

As pointed out by @nkanazawa1989 in #8232 the regex construction added
to the QPY interface functions in #8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.

* Fix regex flags usage

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ebf800a)
mergify bot added a commit that referenced this pull request Jun 24, 2022
* Move QPY version regex construction to import time

As pointed out by @nkanazawa1989 in #8232 the regex construction added
to the QPY interface functions in #8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.

* Fix regex flags usage

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ebf800a)

Co-authored-by: Matthew Treinish <[email protected]>
@nkanazawa1989
Copy link
Contributor Author

Will be available in symengine 0.10.0

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.

Reviving because I was linked here from #9764: I'm a big fan of the performance of this, but I'm slightly concerned that we'll be writing QPY files that can only be opened if the optional dependency symengine is available on the loading machine.

The generating machine might have symengine, but the reader might not; this could easily happen when QPY is being used to transmit circuits over the wire, such as between a user and the IBM backends. The transfer in that case is bidirectional (I believe, because of the ability to retrieve old circuits), so I think we'd need some mechanism to be able to load symengine parameter objects without symengine. I don't think this PR can handle that.

@nkanazawa1989
Copy link
Contributor Author

Thanks Jake. That's good point. Indeed this breaks portability of QPY. Probably we need some mechanism (or port corresponding logic from symengine) to generate binarized expression. IIRC this is tied to C++ part of symengine and porting the logic to Qiskit would be tough. I'll close this PR if there is no objection.

@nkanazawa1989
Copy link
Contributor Author

Closed this PR for now because this PR breaks QPY portability in return for the performance. We can revive this if symengine 0.10 is released and be promoted to required in Qiskit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog mod: qpy Related to QPY serialization on hold Can not fix yet performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants