Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify OpenQASM 2 exceptions #9956

Merged
merged 3 commits into from
Jul 14, 2023
Merged

Conversation

jakelishman
Copy link
Member

Summary

Since gh-9784 added a new qiskit.qasm2 package and suitable exceptions for both import and export of OpenQASM 2 programs, this commit now unifies the exception handling through the rest of the library to use these exceptions.

To avoid breaks in compatibility, the old qiskit.qasm.QasmError is now a re-export of qiskit.qasm2.QASM2Error. This is the base error of both the exporter and importers, despite the old documentation of QasmError claiming to be specifically for parser errors. In practice, the exporter also emitted QasmError, so having that be QASM2Error allows people catching that error to get the same behaviour in these new forms. The actual exporter and importer are updated to emit the precise subclasses.

Details and comments

Probably this will clash a little with #9955 (and this PR technically probably wants to merge before that one) and potentially #9953. I'll fix any merge conflicts as they arise.

This PR is preparing for a later total deprecation and removal of the qiskit.qasm namespace, likely starting in Terra 0.26, since we've begun the process of moving to qiskit.qasm2 and qiskit.qasm3 as explicitly versioned packages, to avoid confusion in the future.

@jakelishman jakelishman added mod: qasm2 Relating to OpenQASM 2 import or export Changelog: None Do not include in changelog labels Apr 12, 2023
@jakelishman jakelishman added this to the 0.25.0 milestone Apr 12, 2023
@jakelishman jakelishman requested a review from a team as a code owner April 12, 2023 23:15
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Apr 12, 2023

Pull Request Test Coverage Report for Build 5548013092

  • 6 of 6 (100.0%) changed or added relevant lines in 3 files are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.006%) to 86.007%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
crates/qasm2/src/lex.rs 3 90.89%
crates/qasm2/src/parse.rs 6 96.65%
Totals Coverage Status
Change from base Build 5547947273: 0.006%
Covered Lines: 71891
Relevant Lines: 83587

💛 - Coveralls

@jakelishman
Copy link
Member Author

The problems in the QPY backwards compatibility tests are likely because it uses a slightly unusual ordering and import structure for its objects, and there are nasty circular dependencies between qiskit.qasm and qiskit.circuit. Once those circular imports are solved, it should not be an issue for the import structure in the QPY backwards compatibility tests if there's a temporary dependency between qiskit.qasm and qiskit.qasm2, since both will be fully at the same level of the hierarchy.

See #9959 for the fix on that.

Since Qiskitgh-9784 added a new `qiskit.qasm2` package and suitable exceptions
for both import and export of OpenQASM 2 programs, this commit now
unifies the exception handling through the rest of the library to use
these exceptions.

To avoid breaks in compatibility, the old `qiskit.qasm.QasmError` is now
a re-export of `qiskit.qasm2.QASM2Error`.  This is the base error of
both the exporter and importers, despite the old documentation of
`QasmError` claiming to be specifically for parser errors.  In practice,
the exporter also emitted `QasmError`, so having that be `QASM2Error`
allows people catching that error to get the same behaviour in these new
forms.  The actual exporter and importer are updated to emit the precise
subclasses.
@jlapeyre
Copy link
Contributor

Arg. I made every effort to ensure no conflicts... Anyway, a blank line and an unwanted import

@jakelishman
Copy link
Member Author

John: what are you trying to do to this PR? It was already up-to-date and rebased completely on top of main before your force push.

@jlapeyre
Copy link
Contributor

jlapeyre commented Jul 13, 2023

I convinced myself that it was not correct (because it was rebased on most recent main, and things disappeared). But, I agree that your rebase on main was already correct. I also think that what I force pushed was also the same as what you did.

@jlapeyre
Copy link
Contributor

I fixed or avoided the two errors above here: jakelishman#14

The slight change in import order means that two new `Gate` class
bodies got executed during the imports for this particular part of the
test suite.  These gates were internal to the `qasm2.load`
functionality and should be exempt from the automated
definition-equivalence testing.
@jakelishman
Copy link
Member Author

Ok, I've reverted it back to the previous state and put in the correct test fix - the issue is that a change in some of the imports caused qiskit.qasm2 to be loaded when previously it wasn't, which caused two "new" Gate subclasses to be picked up by an automated discovery in a place that they shouldn't have been.

@jlapeyre
Copy link
Contributor

jlapeyre commented Jul 13, 2023

I was just comparing your last branch to mine. I think I rebased on top of main that was less up to date than yours. So I agree reverting was correct.

picked up by an automated discovery in a place that they shouldn't have been.

I assumed it was intended. I may have tried harder with DefinedGate. But I ran into an opaque compiled extension when I wanted to look at OpCode. Of course, I could have looked into that source.

Copy link
Contributor

@jlapeyre jlapeyre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ready

@jlapeyre jlapeyre added this pull request to the merge queue Jul 14, 2023
Merged via the queue into Qiskit:main with commit 4409f68 Jul 14, 2023
@jakelishman jakelishman deleted the qasm2/unify-exceptions branch July 19, 2023 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: qasm2 Relating to OpenQASM 2 import or export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants