WHL: enable Limited API-compliant (abi3) wheels - #511
Conversation
1d42e33 to
3d4da82
Compare
|
This appears to work as expected in CI. |
3d4da82 to
bee468d
Compare
That's the way I'm setting it up for PyWavelets too, and is generally the right strategy I believe. For distros for example, they always only compile for a single Python version, so there are no upsides to using the Stable ABI. Same for end users building from source. Wheel distribution is kinda the odd exception. |
|
Can you post the benchmark results? |
bee468d to
1c7f371
Compare
|
Ah, there are already lost to my terminal history unfortunately, but making abi3 an opt-in was trivial enough |
|
opening for review now. The last commit is temporary and intended to be trimmed before merge, but it helps seeing relevant CI in action |
1c7f371 to
a8feebd
Compare
|
I reran the benchmarks (this time on linux + x86_64) master: this branch: |
|
not sure any of these diffs are significant, but I think it's still worth making abi3 an opt-in so we can grow this space without worrying about maximising performance. I expect this will come in handy later this year when/if some version of abi3 makes it into Python 3.15t (see PEPs 803 and 809) |
It's pretty hard to tell, using I'll do a quick pass on review now. |
rgommers
left a comment
There was a problem hiding this comment.
The opt-in approach here looks good to me. The Stable ABI is typically irrelevant for distros as well as end users building from source, so opt-in seems better as the default in general.
A couple of initial comments.
| USE_PY_LIMITED_API = os.getenv( | ||
| "BN_LIMITED_API", "0" | ||
| ) == "1" and not sysconfig.get_config_var("Py_GIL_DISABLED") | ||
| ABI3_TARGET_VERSION = "".join(str(_) for _ in sys.version_info[:2]) |
There was a problem hiding this comment.
This doesn't look quite right, I think we should always target the lowest-supported Python version, rather than the version that's running the build. Or do you plan to enforce (via code comment?) to handle this in the CI workflow file?
There was a problem hiding this comment.
The problem is that when targeting a version that's older than the running interpreter, there are no guarantees that the resulting binaries will actually be backward compatible. Luckily, because cibuildwheel already runs targets in increasing Python version order, this approach works perfectly: the first (oldest) Python builds a wheel and runs tests, then all the other skip the build step and only check for forward compat by running tests.
There was a problem hiding this comment.
a8feebd to
b497e4c
Compare
4255b88 to
3c42389
Compare
3c42389 to
ea8e2af
Compare
87a5b56 to
ff00462
Compare
ff00462 to
4b061a2
Compare
|
friendly ping @rgommers |
d28c122 to
41f61fb
Compare
|
Yes, let's move forward! I'll note that with the latest releases of |
|
I'll try to review by tomorrow. |
|
Enabling wheel testing (#592) revealed some problem with this PR. Testing a |
Try dropping 3.10, it's probably this Cython bug: cython/cython#7914 |
|
you know I thought about it... but there's no Cython in bottleneck 😅 |
|
though, I'll try it nonetheless just to make sure I'm not spending time fixing and debugging a 3.10-specific problem |
58ade0d to
41f61fb
Compare
|
Same exact issue with 3.11 -> 3.12, removed the temp commit that skipped cp310 |
|
Interesting. It's going much better on the meson branch. I'll just self-merge whatever small adjusments these branch have in common and rebase both of them independently. |
41f61fb to
4f0948f
Compare
|
I'm not very familiar with |
|
right now I'm doing both in #556, though I could easily split out the enabling of abi3 if you'd like |
4f0948f to
4bc0f58
Compare
|
Ok, no worries. Let's do both and see how it goes. |
|
Ah, forgot that we didn't land
🙈 too much context switching |
I initially just wanted to experiment with this and see how far from a working state we were... turns out the existing code is pretty close already, so only a couple tweaks were needed.
PyTuple_GET_*functions are normally faster, as they skip certain checks that equivalent Limited API functions will run, so there could be a case to be made that we should still use them by default, but I'm not convinced that the impact is even measurable (I'll run the benchmarks to double check before I undraft though).