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

Investigate Python Stable ABI #891

Closed
mtreinish opened this issue Jun 7, 2023 · 5 comments · Fixed by #1064
Closed

Investigate Python Stable ABI #891

mtreinish opened this issue Jun 7, 2023 · 5 comments · Fixed by #1064
Milestone

Comments

@mtreinish
Copy link
Member

What is the expected enhancement?

We should look into building rustworkx against the stable c abi instead of the version specific one that we use currently. I did this for qiskit in: Qiskit/qiskit#10120 and at least there it didn't look like there was any performance regression associated with the change. Doing this will greatly reduce the number of binaries we'd have to publish at each release.

@IvanIsCoding
Copy link
Collaborator

Just a reminder to add musllinux images to the supported binaries. This was the cibuildwheel config that we used in 0.13.2 to launch it:

env:
          CIBW_BEFORE_ALL_LINUX: "apk add --no-cache curl gcc && curl https://sh.rustup.rs -sSf | sh -s -- -y && source $HOME/.cargo/env && rustup install stable && rustup default stable"
          CIBW_ENVIRONMENT_LINUX: 'PATH="$PATH:$HOME/.cargo/bin:$HOME/.cargo/env" CARGO_NET_GIT_FETCH_WITH_CLI="true"'
          CIBW_MUSLLINUX_X86_64_IMAGE: quay.io/pypa/musllinux_1_1_x86_64:latest
          CIBW_SKIP: cp36-* pp* *win32 *macosx* *manylinux* *686* *s390x* *aarch64*
          CIBW_BEFORE_BUILD: pip install -U setuptools-rust
          CIBW_TEST_REQUIRES: networkx testtools fixtures
          CIBW_BUILD_FRONTEND: "pip"
          CIBW_TEST_COMMAND: python -m unittest discover {project}/tests/rustworkx_tests
          CIBW_TEST_SKIP: cp312-* cp37-* cp38-*

@tuananh
Copy link

tuananh commented Oct 16, 2023

@IvanIsCoding any particular reason we can't build for musl-aarch64?

@IvanIsCoding
Copy link
Collaborator

IvanIsCoding commented Oct 16, 2023

@IvanIsCoding any particular reason we can't build for musl-aarch64?

Currently the reasons are (in order):

  1. NumPy is our only dependency and doesn’t provide musl aarch64 wheels. So you need at least one C compiler to install rustworkx, regardless
  2. Currently we emulate aarch64 using QEMU, which takes a lot of time on CD
  3. aarch64 is not Tier 1 support by Rust (https://doc.rust-lang.org/rustc/platform-support.html). We support Tier 2 with host targets but they are not on CI

I think with the ABI implemented we can support musl aarch64 with tests on build but without CI. Without it, best we can do is publishing wheels without testing it because compiling NumPy on QEMU takes a long time on CD

@IvanIsCoding
Copy link
Collaborator

I have been playing with the ABI and here are some thoughts:

  • Our code seems to be compatible, just enabling the abi-py38 PyO3 feature and the setup-tools Rust py_limited_api option should do the job
  • We will need to completely redesign our CI for building wheels, cibuildwheel was not design with ABI in mind. Of course it works and they have examples, but building once and testing in many architectures is not what they originally had in mind

@mtreinish
Copy link
Member Author

The cibuildwheel configuration isn't actually bad at all, it will detect that the wheels are abi3 and skipping building for > 1 python version. The only extra thing that'll be good to do is run abi3audit on the wheels to just double check things are compatible. I've done a setup with abi3 on qiskit already so we can borrow the configuration from there: https://github.com/Qiskit/qiskit/blob/main/pyproject.toml#L135-L158

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants