Repository navigation
Conversation
github-actions
Bot
added
io.votable
io.fits
time
io.ascii
utils
convolution
timeseries
labels
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
perhaps we should add a note in the pyproject.toml about bumping these hex versions when we bump min python? |
|
@Cadair - yes that probably makes sense, though interestingly we don't have to update the hexes |
|
@neutrinoceros - just to check, based on your comment here - adding these options doesn't stop one from doing free-threaded builds, right? It's just that in those builds the limited API stuff will be ignored and compile as if it wasn't there? |
|
thanks for asking ! Sadly, it does seem to break free-threaded builds. Here's the detailed stderr from my local attempt: Stderr |
|
That seems to be something they are going to have to support at some point? |
|
I guess to support both we will need a way to conditionally add the Personally limited API > free threading in that it's more immediately useful, though it would be nice to not break testing free threading. |
Agree this seems the best approach going forward if we can - @neutrinoceros is there a way to know if one is currently doing a free-threaded build? I guess the question is whether we will manage to make astropy use the limited API or successfully build and work for free-threaded Python first 😅 |
pllim
added
Python 3.14
Python 3.11
Build all wheels
|
Thanks! Are the exotic arch failures related? |
|
The exotic arch failures do seem related |
yes. Use |
|
Just for the record, we haven't released this yet but in the |
I would actually expect them to pass, as long as we keep the job single-threaded ! |
|
Ok well then we should definitely add a CI job. I'll make a new release of the workflows. |
|
@neutrinoceros - done: https://github.com/OpenAstronomy/github-actions-workflows/releases/tag/v1.17.0 Would you have time to set up a new CI job? It might be as simple as just adding |
|
You got it. I'll do this in an hour (maybe less) ! |
|
GAH, GIVE ME THE GITHUB MERGE/REBASE BUTTON ASTROPY WHY DO YOU HATE ME. |
Co-authored-by: Stuart Mumford <[email protected]>
c4d2003 to
9215891
Compare
|
Well it successfully failed. |
|
Re: rebase option -- You mean this?
|
|
yes |
|
OK I enabled it. 🙈 |
| kwargs = {} | ||
| if USE_PY_LIMITED_API: | ||
| kwargs["py_limited_api"] = True | ||
| kwargs['define_macros'] = [("Py_LIMITED_API", "0x030B0000")] |
There was a problem hiding this comment.
This version hex should in general be both
- kept in sync with our
python-requiressetting - match the runtime version used for the build, because it is not actually safe to build, e.g.
cp311-abi3wheels under Python 3.12
The general solution is to compute the hex dynamically.
Feel free to copy my implementation of this https://gist.github.com/neutrinoceros/eafa1cbcda6d770f8a2604476994a76a
There was a problem hiding this comment.
@neutrinoceros - ok so I think I see the logic here, but just to be clear, if we do this - e.g. define the version hex for Python 3.13 and build a wheel on Python 3.13, and if we've said the wheel should be tagged as abi3 in pyproject.toml, then we'll effectively produce an abi3 wheel that doesn't actually work on Python 3.11 for example?
There was a problem hiding this comment.
yes. Wheels always hold a python version tag (e.g. cp311), even if they also have the abi3 tag. A cp313-abi3 wheel will work for Python 3.13 and 3.14, but not 3.11 or 3.12
There was a problem hiding this comment.
... which reminds me, and for completion setuptools doesn't natively modify the final tag.
See https://cibuildwheel.pypa.io/en/stable/faq/#abi3 and the linked example repo
Of course, we don't need to set this up in this PR, or as long as our wheels are not 100% abi3-compliant.
There was a problem hiding this comment.
I don't get it though - in the past I've always just set the hex to the minimum version and it worked fine. The Python docs also say you should set it to the minimum supported version:
https://docs.python.org/3/c-api/stable.html#c.Py_LIMITED_API
Why is it not safe to build with Py_LIMITED_API set to the lowest supported version?
There was a problem hiding this comment.
Just to rephrase this another way:
Let's say you have py-limited-api set to cp311 in pyproject.toml and Py_LIMITED_API set the 3.11 hex. If you build a wheel on Python 3.11 it will work in Python 3.11+. If you build a wheel on Python 3.13, it generates a cp311-abi3 wheel but it may be that because of a subtle change (e.g. argument type) in the API in 3.13, the wheel doesn't work properly on 3.11.
Now let's say you have py-limited-api set to cp311 in pyproject.toml and Py_LIMITED_API set the to be dependent on the version being used. If you build a wheel on Python 3.11 it will work in Python 3.11+. If you build a wheel on Python 3.13, it generates a cp311-abi3 wheel but this time you may even have used functions that didn't exist in Python 3.11 but the build will work fine and still be tagged cp311-abi3.
So in fact, setting Py_LIMITED_API actually increases your chances of issues if you were to build a wheel on Python 3.13 and try it out on 3.11.
In actual practice, I don't think any typical workflows would result in the scenario of building on Python 3.13 and using the wheel on 3.11, but in case it did happen, it seems that setting Py_LIMITED_API to 3.11 will minimize issues (even if the probability of issues is non-zero).
There was a problem hiding this comment.
Yet another argument for setting the hex dynamically is that abi3 will likely not be around for ever, and abi4 might be coming as soon as next year. When that happens, we'll need to compile at least two wheels per arch (supposedly cp312-abi3 + cp315-abi4).
There was a problem hiding this comment.
Now let's say you have py-limited-api set to cp311 in pyproject.toml
How would that work when abi4 is introduced ?
There was a problem hiding this comment.
I'm totally on board with setting the hex as well as the wheel tag dynamically now that I understand about abi3 and abi4. I'll have a think about how we can adjust extension-helpers to allow this and also whether we can avoid repeating boilerplate code here until we are able to simply turn on the limited API globally.
There was a problem hiding this comment.
The general solution is to compute the hex dynamically.
Feel free to copy my implementation of this
btw, I just found out that sys.hexversion exists, so there is a much simpler solution to the one I proposed earlier:
import sys
ABI3_TARGET_HEX = hex(sys.hexversion & 0xFFFF00F0)will dynamically produce the hex of the final x.y.0 version, where x and y are the major and minor version numbers at runtime.
| if not sys.platform.startswith("win"): | ||
| extra_compile_args.append("-fPIC") | ||
|
|
||
| USE_PY_LIMITED_API = not sysconfig.get_config_var("Py_GIL_DISABLED") |
There was a problem hiding this comment.
I have a slight preference for making this option opt-in, namely by making it conditional on an env var (ASTROPY_LIMITED_API ?), because consumers who build from source generally don't need the portability, and should optimize for performance instead.
There was a problem hiding this comment.
and it's easy enough to set such an env var within our cibuildwheel config, that won't affect downstream consumers
There was a problem hiding this comment.
We can make this opt-in for now since we aren't actually tagging the wheel abi3, but just to check, how do you envisage this working once we add the py-limited-api option to pyproject.toml? (in the sense that either the option is there or it isn't, it's difficult to control conditionally)
There was a problem hiding this comment.
Why does it need to be declared statistically ? None of the setuptools-based projects I worked on had this constraint.
There was a problem hiding this comment.
Ah interesting, how do they declare it?
There was a problem hiding this comment.
Dynamically, by subclassing wheel.bdist_wheel.bdist_wheel
See the example I linked in #18211 (comment)
The three failing ones are all compiling for 3.12, but against system |
|
See #18236 for a much simpler alternative to this PR |
|
Closing in favor of #18236 |
7 participants
As discussed in #18163, it is possible to build C/Cython extensions against a limited subset of the Python C API. If all extensions are build against this API, it is then possible to tag a wheel as
abi3which then can be built once on the oldest supported Python version and will then work with future versions.This PR enables compilation of specific extensions against the limited API, to make sure we don't accidentally make those extensions incompatible with the limited API. Some extensions are not yet compatible and we can work over time on making those work too (in follow-up PRs).
I've included a couple of minor fixes that allow the FITS compression module and the VOTable module to build.
I don't think there is a downside to this PR - we are basically just restricting what symbols we can use in C extensions. Even if we decide to never build
abi3wheels, the present PR shouldn't do any harm.In future if all our extensions work with the limited API, we can switch to using the feature of extension-helpers which will automatically add the settings to all the Extensions and remove the changes in this PR.