Handle single bit conditions in QPY

[mtreinish]

Summary

In #6018 initial support for adding classical conditions on a single bit
instead of a register was added. However this wasn't accounted for in
qpy because it didn't exist when qpy was first written. This commit adds
support to qpy without a file format change so it can be backported and
used with already released versions of qpy without needing a new format
version. This is accomplished by exploiting the strict register naming
in qiskit. A register can't have a name outside of regex
"[a-z][a-zA-Z0-9_]*" which we leverage in the case of single clbit
conditions the "register" name in the output QPY data is set to
str(clbit_index) which isn't a valid name. Then on the loading side we
check for a valid name, if it's outside the allowed regex we treat the
condition as a single bit and the name is a str(index). The tradeoff here
is that for a QPY file generated with 0.18.1 (assuming this is backported
and included in 0.18.1) containing single bit conditions this qpy file can not be loaded with qiskit
0.18.0. But this is fine as we only guarantee compatibility in one
direction (loading qpy files generated with older with qiskit with a
newer version of qiskit).

Details and comments

Fixes part of #6475

[jakelishman]

This seems like an eminently sensible way of doing things with the aim being to maintain as much backwards comaptibility with 0.18.0 with as little additional stuff as possible.

Is it worth considering allowing ourselves to take the process further, without breaking the ability to read and write older formats? We can effectively pack as much metadata as we like in by having the first byte be a sentinel invalid identifier character (say \0 or whatever), and follow it with additional metadata. We only need emit this in _write_instruction (or similar) if we're using newer features, so older versions will still be able to open the files if they're not in use. There's an additional nuisance here that the additional metadata would have to be interpretable as valid utf-8 to allow old versions to safely decode it, but if the metadata is just type information, we can always just pack it into the low 7-bits of a byte and 0 the msb. For example, if it was decided that the classical conditioning should allow <, > etc instead of equalities, that could be packed in.

I don't know how much effort we want to put into maintaining the compatibility, though. This is obviously a very hacky suggestion, and it'd be better to account for it directly in the instruction struct, if we're not bothered about losing ABI compatibility for all circuits. If it's likely that the next time new features are added will not be until we're in a position to make an ABI-breaking version of QPY (say for new stuff in QASM 3), then there's no probably no need to consider including more metadata.

[mtreinish]

Yeah, I feel like we should really only do this once since it's kind of hacky and I don't want to have to deal with negotiating different undocumented string formatting for metadata across versions. I opted for this here only because QPY was first released in 0.18.0 and this is something we can backport for 0.18.1 without breaking compat. If we were further away from 0.18.0 and unlikely to push a bugfix release I'd probably just opt to do this in a QPY v2 format and tweak the instruction struct. That being said my feeling here is that unless there is some clear case of something missing from the representation of conditionals in this PR I'd rather not make it more clever than needed. If in the future we add new features on conditions I'd agree we should add a V2 format and change the instruction struct (or create a new condition struct and embed that into the instruction) to represent that extra metadata. We have the qpy format versioning to enable representing new features across release boundaries for this reason (it's just not something we can/should backport to a bugfix release).

That being said I like your idea of prefixing the index int string with a null character. That seems less error prone and does technically leave us open to adding additional prefixes to represent different things if needed. (the int index string does limit this potential future use).

[jakelishman]

Ah, I didn't know that the QPY format was only in 0.18. If that's the case, before it gets any further, would it be better to use the Qiskit version section of the header struct to store the minimum version of Qiskit that would be able to load the file, rather than the version that created it? That lets us "inject" the backward compatibility into the format: newer versions of Qiskit have complete knowledge of older versions, but an older version doesn't know what the latest version it can support is. That way, the deserialiser can just check if its version is compatible with the stored minimum version, and reject the input cleanly if not. If we've never released a new version of Qiskit since, we have an opportunity to make this change without breaking any compatibility.

I'd be happy to tick this now if you want to leave it as-is. I'm also happy waiting if you're changing it to use the null byte and a slightly more defined way of adding more metadata, or if you want input from people with a bit more experience than me!

[mtreinish]

Well that's actually kind of how it's used today. We only have a guarantee for loading a qpy file from the qiskit release that generated it forward, loading it with an older version of qiskit may work but is unsupported. The qpy_serialization.load() function actually emits a warning if qpy.qiskit_version is > qiskit.__version__: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L979-L984

[jakelishman]

I think I mean something slightly different: say in this case, we know that a file generated by 0.18.1 will be successfully loaded by 0.18.0 in some cases and not in others, however at generation time, 0.18.1 knows whether it's using any features that 0.18.0 doesn't support. I'm proposing that instead of storing 0.18.1 in the header in this case, we store 0.18.0 if we don't use anything incompatible. That way 0.18.0 doesn't need to speculatively attempt the load (which, if we're unlucky, will silently "succeed" but be doing the wrong thing), it can know whether it supports the file. It'd let us upgrade the warning to an error, without limiting the forwards spec.

[jakelishman]

The write function would change from https://github.com/Qiskit/qiskit-terra/blob/265530145033bc31a92fc6989be1e5e20de92252/qiskit/circuit/qpy_serialization.py#L856-L868
to something approximately like

    minimium_version = (0, 18, 0)
    output = io.BytesIO()
    for circuit in circuits:
        minimum_version = max(_write_circuit(output, circuit), minimum_version)
    header = struct.pack(
        FILE_HEADER_PACK,
        b"QISKIT",
        1,
        *minimum_version,
        len(circuits),
    )
    file_obj.write(header)
    output.seek(0)
    file_obj.write(output)
    output.close()

where _write_circuit (and subsiduary functions) now returns the minimum feature version it's used.

edit: If storing the whole serialisation in memory is too onerous, we could always overwrite the header information of the file at the end instead, since the struct has a fixed size.

[mtreinish]

Ah, I see what you were getting at now. I'm not sure it's worth the effort though, especially in this case it's a pretty esoteric edge condition. Someone has to be running 0.18.1 to generate qpy and load it with 0.18.0. The only difference between this without that change is when they do that it will raise a warning, which doesn't seem like a big deal IMO. Especially as at least I would prefer to discourage people from trying to go backwards when loading qpy.

[jakelishman]

That's fair. I was thinking more for the future (say Terra 0.20 producing a very simple circuit that 0.18 could still load), but as you say, it's probably too much effort to worry about.

[kdk]

The overall approach here looks good to me.

Since building circuits with single-bit conditionals was not formally "released" in 0.18, is this urgent/requiring a backport? If not, and given that it looks like there will be further changes to the conditional formats coming soon ( #6761 ) that likely will need a file format change, does it might makes sense to introduce this now as an additional case that will live on in the loader?

[mtreinish]

Since building circuits with single-bit conditionals was not formally "released" in 0.18, is this urgent/requiring a backport? If not, and given that it looks like there will be further changes to the conditional formats coming soon ( #6761 ) that likely will need a file format change, does it might makes sense to introduce this now as an additional case that will live on in the loader?

That's a good point, I don't know if it's urgent. I put this together because @taalexander went to use a single clbit condition and serialize it with qpy and hit an error. So there is at least 1 person who tried it. I weighed that against the extra 10-15 lines (although it was more like 2 in the original incarnation of the patch) we'd have to carry around in the V1 format reader and figured it was worth it. But I don't feel super strongly about that.

[mtreinish]

@kdk I added a note to the release note about the single clbit condition feature not being supported yet in: 4246c19

[jakelishman]

@kdk are we happy to merge this now? It's one of only a couple of PRs left blocking 0.18.1.