Adapt crates/qasm3 to work with recent versions of openqasm3_parser #12087
Conversation
|
One or more of the the following people are requested to review this:
|
This commit adds no new features or capabilities to the importer. But it brings the importer up to date with the latest version of openqasm3_parser as a preliminary step in improving the importer. The biggest change in openqasm3_parser is in handling stdgates.inc. Upon encountering `include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the symbol table for gates in the standard library. The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates which returns a `Vec` of information about each "declared" gate. The information is the gate's name and signature and SymbolID, which is sufficient to do everything the importer could do before this commit. Encountering `Stmt::GateDefinition` now is a no-op rather than an error. (This was previously called `Stmt::GateDeclaration`, but importantly it is really a definition.) How the standard library, and gates in general, are handled will continue to evolve. A behavioral difference between this commit and its parent: Before if the importer encountered a gate call before the corresponding definition an error would be raised. With the current commit, the importer behaves as if all gate definitions were moved to the top of the OQ3 program. However, the error will still be found by the parser so that the importer never will begin processing statements. The importer depends on a particular branch of a copy of openqasm3_parser. When the commit is ready to merge, we will release a version of openqasm3_parser and depend instead on that release. See openqasm/openqasm#517
jlapeyre
force-pushed
the
update-for-openqasm3_parser-changes
branch
from
623f59c
to
fbca320
Compare
crates/qasm3/src/build.rs
Outdated
| if name == "U" { | ||
| // The sole built in gate. `gphase` is treated specially. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Can we arrange the symbol table to make this check unnecessary? I'd expect all the built-in symbols to have been populated into the symbol table, so there's no special-case behaviour needed to handle them for a consumer; we could just fetch the symbol and get a gate.
There was a problem hiding this comment.
Are you thinking for example that SymbolTable::gates does not return "U" in its list and this check is omitted?
In the parser, it's convenient to make U a symbol of type Gate. For example, we don't need to handle checking number and type of args specially when it's called.
There was a problem hiding this comment.
Fixed (made the change in my previous comment) in b22b7d7
There was a problem hiding this comment.
Maybe I misunderstood why this check was here - I'm expecting U to be both in SymbolTable::gates and to be handled in the same way as other gates in this function.
crates/qasm3/src/build.rs
Outdated
|
|
||
| let _ = state.map_gate_ids(py, ast_symbols); | ||
|
|
There was a problem hiding this comment.
This is explicitly ignoring a Result, with the one bit of syntax that gets around the clippy lint for not "using" a Result return value. I think what's intended is state.map_gate_ids(py, ast_symbols)?; with no let. It'd be good to add a test that the exceptions are raised in Python space - we didn't do it at the initial merge because of a rush to land the PR, but we ought to back fill.
There was a problem hiding this comment.
I thought clippy complains about throwing away any return value. I agree that state.map_gate_ids(py, ast_symbols)? is much better.
There was a problem hiding this comment.
Partly fixed in 258499c
But I did not yet write a new test.
There was a problem hiding this comment.
Result as a type is marked #[must_use], so any expression that evaluates to a Result needs to be "used" in some form. Most types don't have that annotation, though some functions returning normally safe types might have it, so you may have seen it there.
| // We ignore gate definitions because the only information we can currently use | ||
| // from them is extracted with `SymbolTable::gates` via `map_gate_ids`. | ||
| asg::Stmt::GateDefinition(_) => (), |
There was a problem hiding this comment.
Doing this is fine, but leads me to check: does the backing parser correctly handle the order of definitions? For example, will it correctly reject the program
qubit q;
my_gate q;
gate my_gate q {}because my_gate isn't defined at the point of use?
I'm guessing so - I'm just checking because this swap from a single-pass iteration in the Qiskit side to relying on the AST means that the responsibility for handling it moves away from us.
There was a problem hiding this comment.
Yes it is rejected:
Error: UndefGateError
╭─[/path/to/examples/qasm/ex1.qasm:2:1]
2 │my_gate q;
│ ╰──── Near this point
There were some other doubts I had, a bit vague, about this approach. I don't recall what they are. But if they are warranted, it should become clear later.
The Qiskit importer otherwise needs to filter this gate out. Other consumers probably would do so as well. If it is useful in antother context, we can add an option to include the U gate Review comment: Qiskit/qiskit#12087 (comment)
* This requires modifying the external parser crate. * Depend temporarily on the branch of the parser with this modification. * Make another change required by other upstream improvments. * Cargo config files are changed because of above changes.
Previously this error was ignored. This would almost certainly cause an error later. But better to do it at the correct place.
|
I think all the changes requested have been made. Except one: No tests yet added. See #12087 (comment) I think that issues in this comment #12087 (comment) have been resolved. This is done: |
* Filter U gate from output of SymbolTable::gates The Qiskit importer otherwise needs to filter this gate out. Other consumers probably would do so as well. If it is useful in antother context, we can add an option to include the U gate Review comment: Qiskit/qiskit#12087 (comment) * Use impl trait for SymbolTable::gate return type So we return an iterator, not a `Vec`.
Pull Request Test Coverage Report for Build 8937145960Details
💛 - Coveralls |
There was a problem hiding this comment.
This LGTM, I'm not as familiar with the oq3 parser code but nothing looks clearly wrong and this is passing all the tests we have and is using the latest release of oq3_semantics which is the goal of the PR. We can continue to iterate on this if any issues are raised during the rc period.
…iskit#12087) * Adapt crates/qasm3 to work with recent versions of openqasm3_parser This commit adds no new features or capabilities to the importer. But it brings the importer up to date with the latest version of openqasm3_parser as a preliminary step in improving the importer. The biggest change in openqasm3_parser is in handling stdgates.inc. Upon encountering `include "stdgates.inc" openqasm3_parser reads no file, but rather adds symbols to the symbol table for gates in the standard library. The function `convert_asg` in the importer calls oq3_semantics::symbols::SymbolTable.gates which returns a `Vec` of information about each "declared" gate. The information is the gate's name and signature and SymbolID, which is sufficient to do everything the importer could do before this commit. Encountering `Stmt::GateDefinition` now is a no-op rather than an error. (This was previously called `Stmt::GateDeclaration`, but importantly it is really a definition.) How the standard library, and gates in general, are handled will continue to evolve. A behavioral difference between this commit and its parent: Before if the importer encountered a gate call before the corresponding definition an error would be raised. With the current commit, the importer behaves as if all gate definitions were moved to the top of the OQ3 program. However, the error will still be found by the parser so that the importer never will begin processing statements. The importer depends on a particular branch of a copy of openqasm3_parser. When the commit is ready to merge, we will release a version of openqasm3_parser and depend instead on that release. See openqasm/openqasm#517 * Remove unnecessary conversion `name.to_string()` Response to reviewer comment https://github.com/Qiskit/qiskit/pull/12087/files#r1586949717 * Remove check for U gate in map_gate_ids * This requires modifying the external parser crate. * Depend temporarily on the branch of the parser with this modification. * Make another change required by other upstream improvments. * Cargo config files are changed because of above changes. * Return error returned by map_gate_ids in convert_asg Previously this error was ignored. This would almost certainly cause an error later. But better to do it at the correct place. * Remove superfluous call to `iter()` * Depend on published oq3_semantics 0.6.0 * Fix cargo lock file --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This commit adds no new features or capabilities to the importer. But it brings the importer up to date with the latest version of openqasm3_parser as a preliminary step in improving the importer.
The biggest change in openqasm3_parser is in handling stdgates.inc. Upon encountering
include "stdgates.inc"openqasm3_parser reads no file, but rather adds symbols to the symbol table for gates in the standard library.The function
convert_asgin the importer callsoq3_semantics::symbols::SymbolTable.gateswhich returns aVecof information about each "declared" gate. The information is the gate's name and signature and SymbolID, which is sufficient to do everything the importer could do before this commit.Encountering
Stmt::GateDefinitionnow is a no-op rather than an error. (This was previously calledStmt::GateDeclaration, but importantly it is really a definition.)How the standard library, and gates in general, are handled will continue to evolve.
A behavioral difference between this commit and its parent: Before if the importer encountered a gate call before the corresponding definition an error would be raised. With the current commit, the importer behaves as if all gate definitions were moved to the top of the OQ3 program. However, the error will still be found by the parser so that the importer never will begin processing statements.
The importer depends on a particular branch of a copy of openqasm3_parser. When the commit is ready to merge, we will release a version of openqasm3_parser and depend instead on that release.
See openqasm/openqasm#517