Use stable Python C API for building Rust extension

[mtreinish]

Summary

This commit tweaks the rust extension code to start using the PyO3 abi3 flag to build binaries that are compatible with all python versions, not just a single release. Previously, we were building against the version specific C API and that resulted in needing a binary file for each supported python version on each supported platform/architecture. By using the abi3 feature flag and marking the wheels as being built with the limited api we can reduce our packaging overhead to just having one wheel file per supported platform/architecture.

The only real code change needed here was to update the memory marginalization function. PyO3's abi3 feature is incompatible with returning a big int object from rust (the C API they use for that conversion isn't part of the stable C API). So this commit updates the function to convert to create a python int manually using the PyO3 api where that was being done before.

Details and comments

TODO:

• Release note
• Test cibuildwheel with this change
• Run Benchmarks to see if there is a performance impact

[qiskit-bot]

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

• @Eric-Arellano
• @Qiskit/terra-core
• @kevinhartman
• @mtreinish

[jakelishman]

fwiw I'm happy with the code changes in here, but we need another reviewer (and the release note + cibuildwheel checks) because I was involved in writing the patch.

[Eric-Arellano]

Excellent. We were very sad when we had to stop using abi3 in Pantsbuild because of some IO stuff we were doing. This is a good change.

+1 to setting to abi3-py38

[mtreinish]

Hmm, the test failures (both unit test and neko) look like there is an issue using Complex64 and abi3 in pyo3. This will be a blocker for this because we need to be able to pass complex numbers back and forth to rust code. I guess we could refactor the code to manually pass 2 floats for the real and imaginary parts for each complex number, and build Complex64s in the rust from that. But that doesn't seem like a great solution

[jakelishman]

It looks like the problem here is actually a little split between us and PyO3, but fortunately enough of the bug is on our end that we should be able to get a working version. PyO3 has (for some reason) special handling to allow conversion of length-1 arrays to scalars, but the conversion into Complex64 via this scalarisation path doesn't appear to work correctly when it's in limited-API mode.

For example, I made a dummy PyO3 project with a Rust function:

use num_complex::Complex64;

fn noop_complex(a: Complex64) -> Complex64 {
    a
}

I wrapped that into a PyO3 module pyo3_test, and then used the Python test script:

import numpy as np
from pyo3_test import noop_complex

noop_complex(np.array([1j]))

If you don't set the limited API, the return result is 1j. If you do, you get

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[1], line 4
      1 import numpy as np
      2 from pyo3_test import noop_complex
----> 4 noop_complex(np.array([1j]))

TypeError: argument 'a': float() argument must be a string or a real number, not 'complex'

That's the same error in the Neko test. It's not identical to the one in the regular test suite, but I'm guessing/hoping it's related.

[coveralls]

Pull Request Test Coverage Report for Build 5179802304

• 8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
• 22 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.07%) to 85.923%

---

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/sabre_swap/mod.rs	1	99.77%
crates/qasm2/src/lex.rs	3	90.89%
crates/qasm2/src/parse.rs	18	96.65%

Totals
Change from base Build 5177933484:	0.07%
Covered Lines:	71353
Relevant Lines:	83043

---

💛 - Coveralls

[jlapeyre]

the problem here is actually a little split between us and PyO3 ...

"All checks have passed". But I don't see any commits address the conversion of complex problem. Am I missing something?

[jakelishman]

py_limited_api="auto" is the default, and my experiments locally were that pip install . will not cause --py-limited-api to be passed to setup.py bdist_wheel and that the versioned API will be generated. I suspect that in the most recent test round, we didn't actually build against the limited API. Certainly when building from the sdist (doing so builds a wheel from the sdist then installs) we didn't:

  Created wheel for qiskit-terra: filename=qiskit_terra-0.25.0-cp311-cp311-linux_x86_64.whl size=5896032 sha256=5be4381592c21e5cbf9b422fdd8adffe7b16dc78c84ae6e3b42cab099df56f4b

I think we'll either need to enforce that all instances in our CI that build Terra from source specify --py-limited-api in the build options to pip for that wheel, or we might just be safer setting py_limited_api=True to force it from the setuptools_rust.Extension interface.

John: the test successes may be because of the above, but also commit 7e5b027 fixes the badly typed values that were incorrectly being passed, in the way I suggested in my comment above. That should sidestep this PyO3 bug.

edit: On rereading, I realise that I didn't include the context in the previous comment that Matthew and I had tracked down the issue to badly typed values in the quantum_info module, sorry. I thought I'd mentioned that, but I didn't.

[mtreinish]

Oops, I didn't actually mean to commit "auto" I set that locally for testing and came to the same conclusion that we needed to set it explicitly. I forgot to undo my local edits before running git commit -a when I made the phase type fixes. Let me revert that and go back to True

[jakelishman]

I'm happy with this, though we ought to have a second in-depth review.

Fwiw, I filed the PyO3 inconsistency we found (via our own bug) as PyO3/pyo3#3164.

Forgot we still need a release note.

[kevinhartman]

LGTM once a release note is added.

[Eric-Arellano]

LGTM once a release note is added.

Most people won't care about this. Pip will do its thing just fine. But I agree it's still worth documenting, e.g. for people with lockfiles that write down the checksum of every artifact for the package.

[mtreinish]

This should be ready for final review now. I've added the missing release note and also tested that cibuildwheel works and is producing a single binary and testing across all python versions.

[jakelishman]

Aside from the release-note comments, I'm happy to merge this at this point.

I left a couple of comments inline for posterity. Another one Matt and I discussed more offline: it's not technically necessary to specify abi3-py38 in the Cargo.toml files when passing --py-limited-api=cp38 to bdist_wheel as we do universally in setup.py (it's inferred by setuptools-rust), but we chose to do it this way so that the build_rust command to setup.py and direct cargo-based commands will still build in limited API mode, since these are common workflows for our Rust developers. The number of times we'll need to change the flag is very small, and once we're at an MSRV of 1.64, we can inherit one rule from the workspace's Cargo.toml anyway, reducing the duplication.

[jakelishman]

For posterity: the pre-&& components of the repair commands for Linux and macOS are copied from the cibuildwheel defaults. Windows has no default command.

[jakelishman]

I'll leave this unmerged for a little while to see if any of the other people interested in the Rust/Python API layer want to comment on the final form, since I was very involved with this PR.

[jakelishman]

Merging now, since it's been a week.