Add `use_symengine` option to `qpy.dump` #10820

ElePT · 2023-09-12T12:11:46Z

Summary

This is a proposal to fix #10705 by adding the use_symengine flag to qpy.dump(). This will improve the serialization performance for both circuits containing unbound ParameterExpressions and pulse schedules that rely on custom symbolic expressions.

Details and comments

The use_symengine flag is now stored as part of the file header, as it is a global option used by both circuit and schedule payloads. I have updated the documentation accordingly. I am not that familiar with the schedule "side" of QPY, so let me know if you can think of more cases I can add to the test, or whether I am missing anything (@nkanazawa1989).

qiskit/qpy/binary_io/value.py

jakelishman · 2023-09-12T12:28:47Z

Did you consider modifying the header of the QPY file to include information on which symbolic library was used? Feels to me like we could avoid a lot of try-it-and-see decoding, which is a bit at the whims of Sympy/Symengine changing their encodings, but I didn't think it through hugely.

ElePT · 2023-09-12T13:04:16Z

Did you consider modifying the header of the QPY file to include information on which symbolic library was used? Feels to me like we could avoid a lot of try-it-and-see decoding, which is a bit at the whims of Sympy/Symengine changing their encodings, but I didn't think it through hugely.

Yes, it did cross my mind after all of those very specific try-catches (but for some reason was too reluctant to change it). I think that we could include the encoding in the PARAMETER_EXPR header:

    struct {
        uint64_t map_elements;
        uint64_t expr_size;
        _Bool uses_symengine;
    }

jakelishman · 2023-09-12T13:06:54Z

Does it need to be in PARAMETER_EXPR? use_symengine is a global option to qpy.dump, so could the field be specified only once in the general QPY header rather than per-parameter?

qiskit/qpy/__init__.py

qiskit/qpy/binary_io/schedules.py

qiskit/qpy/formats.py

qiskit/qpy/__init__.py

…terra into symengine-only-qpy

qiskit-bot · 2023-09-14T16:48:03Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

mtreinish

This looks great, thanks for tackling this. I just had two small inline comments (although they likely apply in more places than where I commented). But aside from those I think this is basically ready.

qiskit/qpy/binary_io/schedules.py

mtreinish · 2023-09-26T19:46:01Z

qiskit/qpy/binary_io/schedules.py


-    expr_txt = zlib.decompress(expr_bytes).decode(common.ENCODE)
-    expr = parse_expr(expr_txt)
+        expr = load_basic(zlib.decompress(expr_bytes))


Do we still need to zlib compress the symengine binary serialization? Maybe @nkanazawa1989 or @wshanks has some insight here, but I was thinking this was done for the sympy path because the string representation from sympy will get pretty verbose for some of the more involved pulse functions. But I'm not sure how large the symengine cereal serialization is and whether spending cpu time on inline compression here is worth it.

hmmm I think it is probably still worth it. I checked for the schedule in the unit tests. And I get that the uncompressed string vs compressed string sizes are:

use_symengine=False: 161 bytes uncompressed -> 101 bytes compressed (dump = 2.8 s) use_symengine=True: 429 bytes uncompressed -> 143 bytes compressed (dump = 0.00256 s)

and the dump operation is 0.00005s slower with compression vs without.

Heh, I guess I was worried about nothing since the data sizes we're talking about are sufficiently small zlib is sufficiently fast.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

mtreinish · 2023-09-27T13:38:21Z

qiskit/qpy/type_keys.py

@@ -524,3 +524,21 @@ def assign(cls, obj):
    @classmethod
    def retrieve(cls, type_key):
        raise NotImplementedError
+
+
+class Encoding(TypeKeyBase):


I'm wondering if we should name this SymExprEncoding or something a bit more descriptive. I'm just worried the name Encoding is too generic because if I saw that without the context of this PR I'd think it was like UTF-16.

Being more specific with the class name would surely not hurt (especially if we wanted to add another encoding type in the future), so changed in b479cf0.

mtreinish

This LGTM, thanks for the quick updates.

* Add use-symengine option * Fix lint * Improve error message * Apply feedback * Update docs * Move option to file header, add schedule block * Add example to docs * Fix lint * Apply second round of feedback * Fix lint, update tests * Update docs * Fix use of require_now * Fix compatibility test * Make tests optional * Fix lint * Add release note * Update exception message Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Update other message * Rename Encoding to SymExprEncoding --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

In Qiskit#10820 a test qpy file from the development of the PR accidentally got merged as part of the PR. This shouldn't have been committed and was a mistake. This commit just removes the stray file so we're not continuing to carry it around (although it will remain in the git history forever).

In #10820 a test qpy file from the development of the PR accidentally got merged as part of the PR. This shouldn't have been committed and was a mistake. This commit just removes the stray file so we're not continuing to carry it around (although it will remain in the git history forever).

In #10820 a test qpy file from the development of the PR accidentally got merged as part of the PR. This shouldn't have been committed and was a mistake. This commit just removes the stray file so we're not continuing to carry it around (although it will remain in the git history forever). (cherry picked from commit 1bb219d)

In #10820 a test qpy file from the development of the PR accidentally got merged as part of the PR. This shouldn't have been committed and was a mistake. This commit just removes the stray file so we're not continuing to carry it around (although it will remain in the git history forever). (cherry picked from commit 1bb219d) Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

ElePT added 3 commits September 12, 2023 13:55

Add use-symengine option

83b3e7f

Fix lint

4bff73b

Improve error message

d0746de

ElePT commented Sep 12, 2023

View reviewed changes

qiskit/qpy/binary_io/value.py Outdated Show resolved Hide resolved

ElePT commented Sep 12, 2023

View reviewed changes

qiskit/qpy/binary_io/value.py Outdated Show resolved Hide resolved

mtreinish added this to the 0.45.0 milestone Sep 12, 2023

mtreinish added performance Changelog: New Feature Include in the "Added" section of the changelog mod: qpy Related to QPY serialization labels Sep 12, 2023

ElePT added 5 commits September 12, 2023 18:29

Apply feedback

3526ecd

Update docs

6665390

Move option to file header, add schedule block

bcf1b97

Add example to docs

11ea847

Fix lint

ee80b4a

jakelishman reviewed Sep 13, 2023

View reviewed changes

ElePT added 2 commits September 13, 2023 17:34

Apply second round of feedback

e34b100

Fix lint, update tests

362e310

ElePT commented Sep 13, 2023

View reviewed changes

qiskit/qpy/__init__.py Outdated Show resolved Hide resolved

ElePT and others added 7 commits September 13, 2023 17:45

Update docs

b1aba77

Fix use of require_now

e08ee43

Merge branch 'symengine-only-qpy' of https://github.com/ElePT/qiskit-…

774ba9f

…terra into symengine-only-qpy

Fix compatibility test

b1f6793

Make tests optional

fb2836f

Fix lint

b83a5b9

Add release note

ec0bb10

ElePT marked this pull request as ready for review September 14, 2023 16:47

ElePT requested a review from a team as a code owner September 14, 2023 16:48

mtreinish reviewed Sep 26, 2023

View reviewed changes

ElePT and others added 2 commits September 27, 2023 10:18

Update exception message

1580b16

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Update other message

c9367b5

mtreinish reviewed Sep 27, 2023

View reviewed changes

Rename Encoding to SymExprEncoding

b479cf0

mtreinish approved these changes Sep 27, 2023

View reviewed changes

mtreinish added this pull request to the merge queue Sep 27, 2023

Merged via the queue into Qiskit:main with commit b8b3d58 Sep 27, 2023
13 checks passed

mtreinish mentioned this pull request Oct 13, 2023

Update qpy qiskit 0.45 Qiskit/qiskit-ibm-provider#736

Merged

mtreinish mentioned this pull request Oct 26, 2023

Remove stray QPY file #11121

Merged

jakelishman mentioned this pull request Oct 1, 2024

Workaround symengine serialization payload incompatibility #13251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `use_symengine` option to `qpy.dump` #10820

Add `use_symengine` option to `qpy.dump` #10820

ElePT commented Sep 12, 2023 •

edited

Loading

jakelishman commented Sep 12, 2023

ElePT commented Sep 12, 2023 •

edited

Loading

jakelishman commented Sep 12, 2023

qiskit-bot commented Sep 14, 2023

mtreinish left a comment

mtreinish Sep 26, 2023

ElePT Sep 27, 2023

mtreinish Sep 27, 2023

mtreinish Sep 27, 2023

ElePT Sep 27, 2023

mtreinish left a comment

Add use_symengine option to qpy.dump #10820

Add use_symengine option to qpy.dump #10820

Conversation

ElePT commented Sep 12, 2023 • edited Loading

Summary

Details and comments

jakelishman commented Sep 12, 2023

ElePT commented Sep 12, 2023 • edited Loading

jakelishman commented Sep 12, 2023

qiskit-bot commented Sep 14, 2023

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Sep 26, 2023

Choose a reason for hiding this comment

ElePT Sep 27, 2023

Choose a reason for hiding this comment

mtreinish Sep 27, 2023

Choose a reason for hiding this comment

mtreinish Sep 27, 2023

Choose a reason for hiding this comment

ElePT Sep 27, 2023

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Add `use_symengine` option to `qpy.dump` #10820

Add `use_symengine` option to `qpy.dump` #10820

ElePT commented Sep 12, 2023 •

edited

Loading

ElePT commented Sep 12, 2023 •

edited

Loading