Emit a descriptive error when the QPY version is too new #11074
Conversation
This commit updates the qpy load() function to ensure we are raising a descriptive error message when a user attempts to load a qpy version that's not supported by this version of qiskit. Previously it would fail but with a hard to understand internal error message which was just confusing and not helpful. For example, when trying to load a QPY version 10 payload with qiskit 0.25.2 (which only supported up to version 9) it would raise an error message like: ``` Invalid payload format data kind 'b'p''." ``` which doesn't tell you anything meaningful unless you are intimately familiar with the QPY file format and how the load() function works. With this commit it now checks if the version number is supported immediately and if it's too new it will raise an error message that says exactly that. So in the above scenario instead of that error message it will say: ``` The QPY format version being read, 10, isn't supported by this Qiskit version. Please upgrade your version of Qiskit to load this QPY payload. ```
mtreinish
added
stable backport potential
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 6592270863
💛 - Coveralls |
There was a problem hiding this comment.
I just removed this from the queue because there's something I briefly wanted to discuss, and I'm not super convinced that this should be stable-backport behaviour.
It's not guaranteed behaviour that a newer format file will be unreadable by an older QPY, if the version changes are just in elements that aren't actually used in the file. This change could cause a patch release to stop reading files that it was previously able to. In the case of QPY version 10 vs 9 with the generic QPY header changing, obviously everything would be illegible to an older version now, but that's not necessarily the case.
I think the completeness of this error convinced me that we do need some mechanism for new Terra to export to the QPY format at the time of the release of the last minor, or something like that, even if it requires emitting warnings about potentially invalid behaviour in some cases.
Since the backport branch at the moment is the unreleased 0.45 series and QPY version 10 is completely illegible for all prior QPYs, I think we're ok to backport in this situation, but not in the general case (at the time of writing, this is tagged for the 0.25.3 milestone, which I think would be wrong).
|
In general the QPY version is to document exactly that format changes which previous versions are incompatible with. Even in the small case you highlight of an addition to a type key for something that is incompatible with previous releases. If the user generates a qpy payload using that new type code they'll get an opaque message like I think your concerns correctly raise we should not be backporting QPY version changes to stable branches (which is different than our current policy). Also, I'm totally fine with adding a version field to QPY for the next release (now is the time to do it). I was thinking along those lines too, but having a fixed set of support either version 10 or the current new (or whatever the version is for the 1.0 release). |
|
For the first paragraph: yeah, if they get an error like the type-key thing, it'll be cryptic, though that can be mitigated by us making this new error a warning instead; the user should see that instead. But my point there is that typically new type keys are for new features, and if a user isn't using those, the file will load correctly. It's almost like we might want an "auditwheel" type thing that assigns the lowest possible QPY version to a file (though that'd be hard to do performantly with our implementation). If we had that, I'd be more comfortable raising the exception here. I'm ok if you want to go ahead and merge this - I'm not 100% convinced it's the right thing to do, but I'm equally not convinced that it's not. |
|
What you're basically describing is closer to proper version negotiation. We've been very explicitly avoiding adding that kind of feature to QPY, because it doubles the maintenance burden to support multiple output format versions. But it is what I would have opted for if we were designing a more API endpoint focused format use case instead of as a save file format. But I think we would have changed a lot of QPY's design if we were thinking about that at the start. I think for backport to 0.25.3 we definitely want this, it's necessary for any 0.25.3 users because version 10 is that much different than version 9. When we introduce QPY version 11 we can revisit this on main, maybe there is a better strategy we can come up with without changing how we handle versioning too drastically. I'm also fine skipping this for 0.45.0 (I'll just directly backport it to 0.25.3) and we can always backport it for 0.45.1 later if we deem it necessary when we introduce version 11. |
|
In the meantime I pushed up #11094 to the stable/0.25 branch where this is really needed. It also turns out the branch's CI has been broken since 0.45.0rc1 was released because the backwards compat jobs have been trying to generate qpy from 0.45.0rc1 and load it with the stable branch's qiskit which emits the the type key error message described in the commit message. I put a cap in #11094 to unblock CI, but we'll probably want to do that on main too as we move to supporting >1 stable branch at a time. |
|
We agreed on #11094 to merge this to stable. I'm still in favour of us adding a It's fine to merge here for the same reasons as in #11094; all QPY 10 files are unreadable by QPY 9, and there's a chance of that happening again. We should put in place a way to make it so that newer client-side Terras can be used with older ones with explicit feature restrictions, in order to make it easier to manage server code that depends on Terra, but that's beyond the scope of this PR. |
This commit updates the qpy load() function to ensure we are raising a descriptive error message when a user attempts to load a qpy version that's not supported by this version of qiskit. Previously it would fail but with a hard to understand internal error message which was just confusing and not helpful. For example, when trying to load a QPY version 10 payload with qiskit 0.25.2 (which only supported up to version 9) it would raise an error message like: ``` Invalid payload format data kind 'b'p''." ``` which doesn't tell you anything meaningful unless you are intimately familiar with the QPY file format and how the load() function works. With this commit it now checks if the version number is supported immediately and if it's too new it will raise an error message that says exactly that. So in the above scenario instead of that error message it will say: ``` The QPY format version being read, 10, isn't supported by this Qiskit version. Please upgrade your version of Qiskit to load this QPY payload. ``` (cherry picked from commit 0de8730)
…1131) This commit updates the qpy load() function to ensure we are raising a descriptive error message when a user attempts to load a qpy version that's not supported by this version of qiskit. Previously it would fail but with a hard to understand internal error message which was just confusing and not helpful. For example, when trying to load a QPY version 10 payload with qiskit 0.25.2 (which only supported up to version 9) it would raise an error message like: ``` Invalid payload format data kind 'b'p''." ``` which doesn't tell you anything meaningful unless you are intimately familiar with the QPY file format and how the load() function works. With this commit it now checks if the version number is supported immediately and if it's too new it will raise an error message that says exactly that. So in the above scenario instead of that error message it will say: ``` The QPY format version being read, 10, isn't supported by this Qiskit version. Please upgrade your version of Qiskit to load this QPY payload. ``` (cherry picked from commit 0de8730) Co-authored-by: Matthew Treinish <[email protected]>
Summary
This commit updates the qpy load() function to ensure we are raising a descriptive error message when a user attempts to load a qpy version that's not supported by this version of qiskit. Previously it would fail but with a hard to understand internal error message which was just confusing and not helpful. For example, when trying to load a QPY version 10 payload with qiskit 0.25.2 (which only supported up to version 9) it would raise an error message like:
which doesn't tell you anything meaningful unless you are intimately familiar with the QPY file format and how the load() function works. With this commit it now checks if the version number is supported immediately and if it's too new it will raise an error message that says exactly that. So in the above scenario instead of that error message it will say:
Details and comments