Fix bug in QPY symengine payload version handling #13259
Conversation
This commit fixes a bug that slipped into the sole fix that made up the now yanked 1.2.3 release, Qiskit#13251. The fix logic had a typo/mistake when evaluating the major version in the symengine payload that meant if you were trying to load a QPY payload generated with a different symengine release it would always error. This only got through CI because there was no test coverage of the edge case we were trying to fix. This commit addresses both issues. It first fixes the typo in the QPY parsing (which is a 2 character change) and then updates the QPY compat tests to explicitly test using multiple symengine versions with QPY to make sure we're exercising this code path moving forward (and validating this fix).
mtreinish
added
priority: high
stable backport potential
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11150886319Details
💛 - Coveralls |
There was a problem hiding this comment.
This looks good to me.
For others, the output of how the tests look for this can be found here: https://dev.azure.com/qiskit-ci/qiskit/_build/results?buildId=61264&view=logs&j=a15d9aca-d75c-55b1-7f76-889cfe884cf0&t=5ecd479e-a958-503d-d481-b374b73669a6&l=5164
There was a problem hiding this comment.
I verified this PR locally by generating the QPY backwards-compat test suite with both symengine 0.11 and symengine 0.13, and I was able to use this PR to load both sets, with both symengine 0.11 and 0.13 (so I tried all four combinations of generator and loader). Further, I was able to reproduce the loading failures (using the same files) with the parent of this commit.
I did these tests locally twice on #13251, but crucially failed to repeat them locally for the changes made after the second review (as did Matt), because we were rushing too much. That's how it wasn't caught by humans. The bigger failure is that we didn't think through a good way to add it to CI to automate the process - we thought we'd need a whole other CI job, and were in a rush to get out the release so thought we'd do it later.
I agree with Kevin that the CI runs show that this executed as expected. I also checked the CI logs to verify that symengine 0.13.0 was installed in qiskit_venv, and symengine 0.11.0 was installed in symengine_11_venv.
| # symengine is pinned to 0.13 to explicitly test the migration path reusing the venv | ||
| "$qiskit_venv/bin/pip" install -c "$our_dir/../../constraints.txt" "$our_dir/../.." packaging "symengine~=0.13" |
There was a problem hiding this comment.
This isn't actually a pin on symengine 0.13 - it's equivalent to symengine>=0.13,==0.*, so it'd allow 0.14, but I think that's intended, given the comment lower in the file about needing an update when symengine 0.14 is released?
There was a problem hiding this comment.
Oh I actually meant to remove this. I just checked it's in my local diff but I didn't add that to the commit. I started down the path of thinking about pinning here, but figured it'd be better to handle it explicitly at that future date than protect against it in the test setup preemptively. Especially since we have the cap in the requirements file. I can remove this if you'd prefer.
There was a problem hiding this comment.
I think it's ok as written - it's good to be explicit that it's specifically something newer than the 0.11 series, which this pin does.
It's just the comment that's a bit misleading, but I strongly suspect we're going to need to re-do this file a little bit as part of making the cache keys for the QPY jobs more reliable - things like generation script requirements will need to be separated off to become part of the cache key - and we'll be able to expand the inter-symengine-version testing once that's fixed as well, because the jobs will run faster.
|
Oh, also for posterity: I verified that the only differences between the files |
This commit fixes a bug that slipped into the sole fix that made up the now yanked 1.2.3 release, #13251. The fix logic had a typo/mistake when evaluating the major version in the symengine payload that meant if you were trying to load a QPY payload generated with a different symengine release it would always error. This only got through CI because there was no test coverage of the edge case we were trying to fix. This commit addresses both issues. It first fixes the typo in the QPY parsing (which is a 2 character change) and then updates the QPY compat tests to explicitly test using multiple symengine versions with QPY to make sure we're exercising this code path moving forward (and validating this fix). (cherry picked from commit e5ec413) # Conflicts: # qiskit/qpy/__init__.py
This commit fixes a bug that slipped into the sole fix that made up the now yanked 1.2.3 release, Qiskit#13251. The fix logic had a typo/mistake when evaluating the major version in the symengine payload that meant if you were trying to load a QPY payload generated with a different symengine release it would always error. This only got through CI because there was no test coverage of the edge case we were trying to fix. This commit addresses both issues. It first fixes the typo in the QPY parsing (which is a 2 character change) and then updates the QPY compat tests to explicitly test using multiple symengine versions with QPY to make sure we're exercising this code path moving forward (and validating this fix).
…13260) * Fix bug in QPY symengine payload version handling (#13259) This commit fixes a bug that slipped into the sole fix that made up the now yanked 1.2.3 release, #13251. The fix logic had a typo/mistake when evaluating the major version in the symengine payload that meant if you were trying to load a QPY payload generated with a different symengine release it would always error. This only got through CI because there was no test coverage of the edge case we were trying to fix. This commit addresses both issues. It first fixes the typo in the QPY parsing (which is a 2 character change) and then updates the QPY compat tests to explicitly test using multiple symengine versions with QPY to make sure we're exercising this code path moving forward (and validating this fix). (cherry picked from commit e5ec413) # Conflicts: # qiskit/qpy/__init__.py * Fix merge artifact --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
This commit fixes a bug that slipped into the sole fix that made up the now yanked 1.2.3 release, Qiskit#13251. The fix logic had a typo/mistake when evaluating the major version in the symengine payload that meant if you were trying to load a QPY payload generated with a different symengine release it would always error. This only got through CI because there was no test coverage of the edge case we were trying to fix. This commit addresses both issues. It first fixes the typo in the QPY parsing (which is a 2 character change) and then updates the QPY compat tests to explicitly test using multiple symengine versions with QPY to make sure we're exercising this code path moving forward (and validating this fix).
Summary
This commit fixes a bug that slipped into the sole fix that made up the now yanked 1.2.3 release, #13251. The fix logic had a typo/mistake when evaluating the major version in the symengine payload that meant if you were trying to load a QPY payload generated with a different symengine release it would always error. This only got through CI because there was no test coverage of the edge case we were trying to fix. This commit addresses both issues. It first fixes the typo in the QPY parsing (which is a 2 character change) and then updates the QPY compat tests to explicitly test using multiple symengine versions with QPY to make sure we're exercising this code path moving forward (and validating this fix).
Details and comments