QASM3 exporter

[1ucian0]

on top of #6621 and #6492, so on hold.

Summary

This is an initial support for exporting Qiskit QuantumCircuit objects as QASM3. It's goal is not to be strictly equivalent as the Qiskit representation. In this sense, rounds like Circuit1 <QuantumCircuit> -> <QASM3> -> Circuit2 <QuantumCircuit> do not warranty Circuit1 == Circuit2. Similarly, String1 <QASM3> -> <QuantumCircuit> -> String2 <QASM3> does not imply String1==String2.

Currently, the supported features are limited to:

• dump to string or to file-like objects (thanks to @jakelishman)
• Custom identation (thanks to @jakelishman)
• Standard instructions
• Some name collision handling
• Constant handling
• Unbound circuit parameters as inputs
• Some support of composite gates
• Basic opaque instructions
• Classical register conditional
• Single bit conditional

Details and comments

Some notes on the design (mostly coming from @jakelishman, thanks again!).

There is "functional" interface with dump and dumps at module level. The Exporter instances handles the includes, the primitive gates (basis_gates) and the identation.

The AST (defined in ast.py is going to be replaced by the reference AST). The BasicPrinter (defined in printer.py) creates the dump string by visiting the AST nodes.

A gate can signal to the exporter it's definition with the by defining the method _define_qasm3 which, currently, should return an AST. (in the future, it can return a string that is going to be parsed into an AST).

Pending:

• The GlobalNamespace._data is horrendous and needs to be splitted. More here. Tracked by Some internal polishing is needed in the QASM3 exporter #7136
• ~Merge GlobalNamespace and Exporter. ~ Tracked by Some internal polishing is needed in the QASM3 exporter #7136
• Module level documentation. Tracked by QASM3 exporter needs module level documentation #7137
• Raise warning when a gate is renamed.
• Related with previous point. Namespace validation and renaming, see here and here. Tracked by Some internal polishing is needed in the QASM3 exporter #7136
• Write the calls in terms of the base register.
• A better way to handle "default type". Currently float[32]. Details here.

[kdk]

Do we parse the include files to know which gates need to have definitions written?

[1ucian0]

parsing is too much of a big word. Just searching for the gate definitions to add them into the scope.

[kdk]

I think we should check instruction.name == stdgatesinc_name here, or maintain a mapping between qiskit and "stdgates.inc" names, rather than checking isinstance(instruction, qiskit_class). instruction.name is the canonical way to identify an instruction, and some of the Instruction classes have some unusual sub-classing that could trip us up here.

[1ucian0]

About instruction.name == stdgatesinc_name, having the same name does not mean they are the same gate. If a user accidentally set an instruction with a standard name gate, the circuit will be silently exported with a different semantics.

maintain a mapping between qiskit and "stdgates.inc" names

Not fully sure what's that. You mean that qiskit_gates should be somewhere else?

Instruction classes have some unusual sub-classing that could trip us up here.

That's solved with better subclassing, I guess. :)

[kdk]

Took another pass at reviewing, mostly in grammar.py and the tests. I'm not an expert on the grammar, so a handful of questions there but on the whole this looks great so far.

[ajavadia]

add reno plz

[1ucian0]

Took another pass at reviewing, mostly in grammar.py and the tests. I'm not an expert on the grammar, so a handful of questions there but on the whole this looks great so far.

Hopefully, once there is an official AST openqasm/openqasm#220, the grammar.py file is not needed.

[jakelishman]

I didn't read all the way through grammar.py, because I started hitting bits where it looked like it wasn't complete (Expression, Input, and so on), and I'd want to look at all of it at once.

My main concern at the moment is that I think we're using a very class-based interface, but the separation of concerns / inputs to each class aren't really correct. In particular, I think that Exporter shouldn't be instantiated taking a QuantumCircuit, and I think it should be responsible for managing heavy, one-time, backend-specific parts of the export process, so they can be re-used to export many different QuantumCircuit instances.

For example, I think almost all of the current code in GlobalNamespace should be the domain of Exporter (it mostly just manages includes), and the rest of the namespace management could just be a single dict held within QasmBuilder (or a stack(list) of dict, to keep track of inner scopes). The thing which holds a namespace shouldn't really be responsible for parsing bits of the code. At most, I think it would be a bi-directional dict (i.e. a class which manages forwards- and backwards-links).

Having Exporter pre-parse the include files means it doesn't need to be repeated, though this is really where we need the internal parser package to already be in place to handle it. If that's really not feasible for 0.19, I think it might be better to architect around the idea of storing stdgates.inc in AST form, not string form, and parsing the defined gates out of that in Exporter, with a view to making it more general in 0.20 once we can rely on an external package to parse arbitrary includes.

The namespace also (I'd have thought) should just be a mapping between either instruction.name or type(instruction) (I prefer the latter, I think) and <qasm name> - as it stands, it takes the id(instruction), which means that the namespace holds many many entries for everything; RXGate will have a different entry in the namespace dict for different rotations, for example.

The AST I think has some unnecessary elements in it, but that's mostly just because it's following the grammar quite tightly. If, in a later release, we want to swap to the reference implementation (once it's ready), we may be able to cut some boilerplate elements - things like the Header node don't add anything by being separate, and could just be managed by the Program node if it kept track of version, includes and io_variables itself. I wouldn't put much effort into slimming this down, most likely, since we're expecting to switch out the AST in the future.

[jakelishman]

Minor nit: can we put the underscores in things like globalstatements here? It's pretty hard to read as it is, and the build_ prefix should still group everything nicely.

[jakelishman]

This hardcoding of the type is something we're absolutely going to need to revisit, but I don't think we'll be able to put in a proper solution until we've decided exactly what we're doing with classical expressions.

[jakelishman]

The allowed characters are actually much wider than this - it's a whole bunch of Unicode blocks (not to mention _).

[1ucian0]

that's right. The grammar is a bit complex here:

fragment ValidUnicode : [\p{Lu}\p{Ll}\p{Lt}\p{Lm}\p{Lo}\p{Nl}] ; // valid unicode chars
fragment Letter : [A-Za-z] ;
fragment FirstIdCharacter : '_' | '$' | ValidUnicode | Letter ;
fragment GeneralIdCharacter : FirstIdCharacter | Integer;

Not a problem to focus right now, but eventually. For now, removing that comment in 938b6c2. What do you think?

[jakelishman]

I'm not sure what the purpose of this property is, and given that the namespace can change from under us, it's probably better to make it a general function called something like unique_identifier() or something, and make it a clear function call.

[1ucian0]

Yeap, your are right.

[1ucian0]

I didn't read all the way through grammar.py, because I started hitting bits where it looked like it wasn't complete (Expression, Input, and so on), and I'd want to look at all of it at once.

grammar.py will be replaced by a proper AST in the future.

[jakelishman]

See 1ucian0#8 as well.

Edit: now merged.

[jakelishman]

This is pretty temporary: just like the QASM 2 exporter, we probably will want a way for gates to define their QASM 3 representation, but with QASM 3 being far more complex, the structure of exporting now goes "Terra -> AST -> string". So gates have to return something that can be made into an AST object. For the time being, while we're vendoring our own AST, we return that. In the future, once we've shifted to using the reference AST, which crucially will include a "string -> AST" parser, we'll be able to allow gates to return their QASM 3 definition as a string, and in those cases we'll be able to use the parser to raise it to a full AST node.

Having the definitions in proper AST representation is nice because we get a separation of concerns between AST generation and text output, and we can optionally do much more compiler-like things, like verifying that the instruction is valid, and that the signature is consistent with (say) another definition given in an include file.

The CX gate here is mostly just an example - we may want to move this to a different gate, or just define a new test gate in a unit test, since in reality cx should be mapped to builtin names most of the time.

[1ucian0]

The CX gate here is mostly just an example

Not sure. The problem with cx is that has no definition in Qiskit. Not having a _define_qasm3 means that cx is opaque when the standard include is missing.

[jakelishman]

The QASM 3 exporter should be able to do something sensible for it without a _define_qasm3 method - it's a simple control gate, and we probably should have some way of quickly working out that it can be defined by ctrl @ x q0, q1 by inspecting the Terra definition. It'll be a nuisance for us (we have many controlled gates) and users (Terra can't use information they've already given) if we don't handle it automatically.

[jakelishman]

That said, I'd be happy if the way we do it is to define _define_qasm3() on ControlledGate, and have the inherited to all members, but that starts to get slightly more complicated, because then ControlledGate._define_qasm3 will need to have a way to query the exporter for how the inner gate is defined. That's an example of what you were worried about when we called - there needs to be a two-way flow of information in that case. One way to do that might be by a sort of co-routine approach - we could allow _define_qasm3 to yield arbitrary Terra objects, and have the caller feed their AST representations back in with generator.send.

Something like:

class ControlledGate:
    def _define_qasm3(self):
        base_ast = yield (self.base_gate,)
        # ... modify the AST into the form we want ...
        return out

and the caller does something along the lines of (forgive me - I never get the flow right the first time with co-routines):

def handle_object(self, obj, context):
    output = obj._define_qasm3()
    if isinstance(output, ASTNode):
        self.update_context(context, object, output)
        return output
    if not isinstance(output, generator):
        raise QiskitError(...)
    to_parse = next(output)
    while True:
        parsed = []
        for inner in to_parse:
            parsed.append(self.handle_object(inner, context))
            self.update_context(context, inner, parsed[-1])
        try:
            to_parse = output.send(parsed)
        except StopIteration as result:
            break
    self.update_context(context, obj, result.value)
    return result

This allows _define_qasm3 to yield an arbitrary number of collections of objects back to the exporter, which will parse them into AST, then pass them back in parsed form. It then just loops round doing that til the original _define_qasm3 returns its final value. This would have problems with object cycles, but I'm not sure if we're allowing QASM 3 to be recursive anyway, but if so, we can do a similar thing to deepcopy.

[mtreinish]

My longer term question here is about the interface we move to. I get that this is temporary given the current state of the upstream qasm ast and the lack of a qiskit parser. But I feel longer term the place we want to be in a __qasm3__ attribute that just returns a string of the qasm representation of the gate.

The thing I want to avoid is the pattern of having _qasm() and _assemble() on every standard gate. For things in qiskit's standard lib we should have enough context to know how to deal with them without having to hard code a bunch of stuff in the classes. But, having something that's opt-in for objects where we don't have that context or in other situations makes sense.

[jakelishman]

Yeah, I don't think we should need a _qasm() method on any standard gate - we should be able to set up the types enough that this is never necessary. I didn't originally think we were going to merge with this AST still attached to CX - I thought it was mostly just for testing purposes, and we were planning to move it into a custom test gate.

We definitely do need something opt-in, that isn't just static, because there are problems when the exporter needs to munge the output names to avoid conflicts in custom gates. It's ok if the custom gate only relies on standard gates with invariant names (it's easy enough for us to regex-capture the name the gate has returned), but if it relies on other custom Terra instructions, it needs to know how to call them in QASM in order to output the correct call. If not, we might mistake it for a different instruction.

[jakelishman]

For reviewers: we fully expect that the files ast.py and printer.py will be completely removed in the future, and become the responsibility of a package hosted by the OpenQASM 3 team. There's going to be a reference QASM 3 Python AST which we will swap to targetting (openqasm/openqasm#269 for current progress), and when that's released as a package, we'll likely contribute an "unparser" (AST -> text converter) upstream, similar to BasicPrinter here, but for the new AST. The use of the visitor pattern for printing is meant to make these changes easier to handle when the time comes - that's how we'll consume the reference AST.

Similarly, there's a good chance that we can move stdlib.inc into the reference AST package as well (since it's a standard library file), including a pre-generated AST representation of it. Then we can use that in Exporter here, as well, without vendoring it in Terra (and also then we won't keep being tempted to add to it or change it!).

These things are unlikely to be possible in Terra 0.19, though, because:

• the QASM 3 reference AST may not be ready and packaged before then (though I'm trying, and the AWS guys mostly in charge make changes fast)
• even if it is ready, it uses @dataclass, which is Python 3.7+ only. Since Terra 0.19 will support Python 3.6, if we want to use the reference AST in Terra 0.19, we may have to vendor it, and swap it to an attrs-based model temporarily instead.

In Instruction._define_qasm3, Luciano and I discussed the future possibility of allowing those methods to return strings, and having us parse the result into an AST object, so we can properly handle it (e.g. check validity of the output, verify it's a consistent definition with header files, check calling signature - that sort of thing). This is probably much nicer for writers of those objects, but isn't possible until Terra can (optionally) depend on the reference AST parser - we absolutely do not want to add a QASM 3 -> AST parser directly into Terra, because it's complicated as anything, and should be another package's responsibility. We'll only handle the AST -> Terra and Terra -> AST steps.

[ecpeterson]

I played around with this and gave it a quick fuzzing. I didn't find anything wrong, but I did find a few behaviors that might surprise a user — if they were to stay, though my understanding is that these quirks may well be temporary, as code is scheduled to shift around. For instance, I was surprised that multiple invocations of the same gate (as in: a single gate definition in QASM 2 input, invoked a few times in a row) got re-exported as several identical definitions with different munged names. Copy/paste 3.5 from https://arxiv.org/pdf/1707.03429v2.pdf to reproduce. It looks like sometimes this is expected (test_composite_circuits_with_same_name) and sometimes not (test_same_composite_circuits) (which includes my use case), but in any event this has the same operational behavior so I'm not fussed.

I didn't find any wrinkle which should delay this PR. Once others' outstanding changes clear, it L G T M.

[mtreinish]

This merged before I had time to finish my review, but here are the comments I had on the pending review before this merged

[mtreinish]

My longer term question here is about the interface we move to. I get that this is temporary given the current state of the upstream qasm ast and the lack of a qiskit parser. But I feel longer term the place we want to be in a __qasm3__ attribute that just returns a string of the qasm representation of the gate.

The thing I want to avoid is the pattern of having _qasm() and _assemble() on every standard gate. For things in qiskit's standard lib we should have enough context to know how to deal with them without having to hard code a bunch of stuff in the classes. But, having something that's opt-in for objects where we don't have that context or in other situations makes sense.

[mtreinish]

Shouldn't this be OpenQASM3 to differentiate it from the qiskit.qasm module

Suggested change

	Qasm (:mod:`qiskit.qasm3`)
	OpenQASM 3 (:mod:`qiskit.qasm3`)