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

Fail gracefully on bad SabreDAG construction #10724

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

jakelishman
Copy link
Member

Summary

This is a private, internal Rust type, but it doesn't cost us anything (meaningful) to bounds-check the accessors and ensure we fail gracefully rather than panicking if we ever make a mistake in Python space. This more faithfully fulfills the return value of SabreDAG::new, which previously was an effectively infallible Result.

Details and comments

Stems from #10712 (comment). We might not need to bother with this, but if we did, this is probably what it'd look like.

The extra test shim stuff is just because PyO3/Rust can't link statically against CPython for the test suite; usually extension-modules link dynamically against Python with the particular shared library getting linked during Python's initialisation of the module.

@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Aug 29, 2023
@jakelishman jakelishman requested a review from a team as a code owner August 29, 2023 12:07
@qiskit-bot
Copy link
Collaborator

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

kevinhartman
kevinhartman previously approved these changes Aug 29, 2023
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looks great.

Is this the first bit of Rust-based unit testing we've got in accelerate?

@kevinhartman kevinhartman enabled auto-merge August 29, 2023 15:43
@jakelishman
Copy link
Member Author

It is the first bit of testing (I didn't want to write a Python test that's testing an internal Rust interface), but weirdly it works completely fine locally for me and fails in CI. I'll have to look into the setup.

@jakelishman
Copy link
Member Author

On closer inspection of that error message, I'm guessing/hoping the problem is just that Azure's setup Python action doesn't add Python's dynamic libraries path to the default linker path, but if we get to that point from within a Python interpreter session then it's obviously already loaded into memory.

@jakelishman jakelishman force-pushed the sabre/verify-dag branch 6 times, most recently from d3d2eb8 to 64c1982 Compare August 30, 2023 11:38
This is a private, internal Rust type, but it doesn't cost us anything
(meaningful) to bounds-check the accessors and ensure we fail gracefully
rather than panicking if we ever make a mistake in Python space. This
more faithfully fulfills the return value of `SabreDAG::new`, which
previously was an effectively infallible `Result`.
@jakelishman
Copy link
Member Author

Alright, the issue in the end was that PyO3 was finding the correct place to link in the library and on Linux (or at least the CI machines) that's a dynamic library. However, for reasons that I don't fully understand, it doesn't write out the library path into the rpath of the test executable, so dlopen subsequently fails to load it during the execution.

Since I don't 100% understand why PyO3 doesn't write out the rpath in this case, and that's presumably a deliberate decision on their part, I elected not to do weird hacky build overrides that might have an impact on developers, and instead just made sure that a suitable dynamic library is locatable when loading the executable.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6025520902

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.004%) to 87.246%

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/accelerate/src/sabre_swap/sabre_dag.rs 8 86.21%
Totals Coverage Status
Change from base Build 6023513300: -0.004%
Covered Lines: 74367
Relevant Lines: 85238

💛 - Coveralls

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for enabling Rust-only testing 😄.

@kevinhartman kevinhartman added this pull request to the merge queue Sep 4, 2023
@jakelishman
Copy link
Member Author

In retrospect, I'm fairly sure that PyO3 doesn't automatically write out the path of the dynamic Python library detected during the build into the rpath because that would couple the output executable to the directory structure of the machine that built it, which isn't useful during packaging as that's not a fixed point. So what I did of ensuring that the library path is set up correctly during the run is (I think) the correct thing to do.

Merged via the queue into Qiskit:main with commit 1606ca3 Sep 4, 2023
mergify bot pushed a commit that referenced this pull request Sep 4, 2023
* Fail gracefully on bad `SabreDAG` construction

This is a private, internal Rust type, but it doesn't cost us anything
(meaningful) to bounds-check the accessors and ensure we fail gracefully
rather than panicking if we ever make a mistake in Python space. This
more faithfully fulfills the return value of `SabreDAG::new`, which
previously was an effectively infallible `Result`.

* Run `cargo fmt`

(cherry picked from commit 1606ca3)

# Conflicts:
#	Cargo.toml
#	crates/accelerate/src/sabre_layout.rs
#	crates/accelerate/src/sabre_swap/sabre_dag.rs
@jakelishman jakelishman deleted the sabre/verify-dag branch September 4, 2023 16:31
jakelishman added a commit that referenced this pull request Sep 5, 2023
* Fail gracefully on bad `SabreDAG` construction

This is a private, internal Rust type, but it doesn't cost us anything
(meaningful) to bounds-check the accessors and ensure we fail gracefully
rather than panicking if we ever make a mistake in Python space. This
more faithfully fulfills the return value of `SabreDAG::new`, which
previously was an effectively infallible `Result`.

* Run `cargo fmt`

(cherry picked from commit 1606ca3)
github-merge-queue bot pushed a commit that referenced this pull request Sep 5, 2023
* Fail gracefully on bad `SabreDAG` construction

This is a private, internal Rust type, but it doesn't cost us anything
(meaningful) to bounds-check the accessors and ensure we fail gracefully
rather than panicking if we ever make a mistake in Python space. This
more faithfully fulfills the return value of `SabreDAG::new`, which
previously was an effectively infallible `Result`.

* Run `cargo fmt`

(cherry picked from commit 1606ca3)

Co-authored-by: Jake Lishman <[email protected]>
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: transpiler Issues and PRs related to Transpiler Rust This PR or issue is related to Rust code in the repository stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants