-
Notifications
You must be signed in to change notification settings - Fork 151
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
Build Python ABI3 wheels instead of a wheel per Python version #1064
Conversation
Pull Request Test Coverage Report for Build 8649595792Details
💛 - Coveralls |
I still need to remove |
@mtreinish this should be ready to review, all the wheels build in https://github.com/IvanIsCoding/rustworkx/actions/runs/8400723506 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and thanks for manually validating the builds. I just had 2 quick inline questions but not a blocker per say.
The other question I had was did you do any performance testing to see if there was a measurable impact from moving to abi3?
@@ -91,5 +92,6 @@ def readme(): | |||
"mpl": mpl_extras, | |||
"graphviz": graphviz_extras, | |||
"all": mpl_extras + graphviz_extras, | |||
} | |||
}, | |||
options=RUST_OPTS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember having to set this flag, for building abi3 wheels what is requiring this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied from https://github.com/Qiskit/qiskit/blob/bee2b95f6f790831ef1675b311988bb5ce6d2d6d/setup.py#L54, I just used a variable instead
@@ -29,14 +29,15 @@ jobs: | |||
- uses: actions/setup-python@v5 | |||
name: Install Python | |||
with: | |||
python-version: '3.8' | |||
python-version: '3.10' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's good to bump this to 3.10 since 3.8 goes eol in october (which is a good reminder for us to start emitting a deprecation warning on 3.8 in 0.15.0). But I'm wondering if there was something that required us to move to 3.10 here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was that macOS Arm only runs 3.10+ and I wanted to make it consistent
I did not benchmark it, maybe we should run https://github.com/mtreinish/retworkx-comparison-benchmarks against the main branch vs this PR. Unfortunately the performance won't get better, but maybe it will not get that much worse? |
Let's go ahead and merge it now. We can benchmark it after it merges pretty easily. At least when we made this change with qiskit it wasn't really measurable and we can offset it potentially by leveraging PGO or something. But even if there is a small performance regression, the portability benefits are worth it. |
@BastianZim @wshanks FYI I don’t know how this affects https://github.com/conda-forge/rustworkx-feedstock but for 0.15.x we will be distributing less binaries. I hope it simplifies your work in conda too |
Thanks, @IvanIsCoding. Distributing less binaries doesn't help us directly because we build from source any way, but I hope we can make use of the fact that RustworkX is abi3 compatible in the future. Currently, changes to conda are needed because conda doesn't know where |
Closes #891
This PR switches rustworkx wheels to build against the Python stable ABI. For 0.15, we choose Python 3.8 as the minimum required version.
Most notably, we also remove our hard-coded splits for less common architectures. This simplifies our setup.
We also update
cibuildwheel
to 2.17.0,actions/download-artifact
to v4 (which had breaking changes), and bumped the Python used in the tasks to 3.10.