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

Add support for Py_LIMITED_API ? #78

Closed
jlaine opened this issue Jun 30, 2018 · 21 comments · Fixed by #1091
Closed

Add support for Py_LIMITED_API ? #78

jlaine opened this issue Jun 30, 2018 · 21 comments · Fixed by #1091

Comments

@jlaine
Copy link
Contributor

jlaine commented Jun 30, 2018

This is a more a "should we do this" question rather than an actual issue.

Background : Python 3.2 and up offer a subset of the C API which is guaranteed to be stable at the ABI level. For packages that only make use of this limited API, it is possible to build wheels which can be used across multiple Python versions.

https://docs.python.org/3/c-api/stable.html

For instance cryptography switched to Py_LIMITED_API a few months ago:

pyca/cryptography#3307

Adding support for this would for instance have avoid the need to rebuild wheels for Python 3.7.

@YannickJadoul
Copy link
Member

Hmmm, I hadn't come across this before. So how do you reckon would this then be supported by cibuildwheel? What's the difference to the build process?

And, trying to assess the idea further: how relevant is cibuildwheel to those users, if they don't have to run the build for all different Python versions anyway?

@jlaine
Copy link
Contributor Author

jlaine commented Jun 30, 2018

I think the only change to the build process is passing something like --py-limited-api=cp34 to setup.py

As for for "does it make sense to use this with cibuildwheel" I can think of a couple of benefits:

  • some APIs may only have been declared "ABI stable" from a certain version of Python onwards. This is the case for cryptography: pythons 3.4 and up can use a common wheel, previous ones need their own wheels

  • Python 2.x has no equivalent for this feature -> you still need to build a separate wheel

  • cibuildwheel's ability to run a test suite on the resulting wheels remains a great bonus

  • cibuildwheel remains a big help for the "cross platform" aspect

As stated initially, I mainly wanted to get a discussion started about whether this makes sense or not, I don't really have the answer at this stage.

@joerick
Copy link
Contributor

joerick commented Jul 1, 2018

Interesting. I didn't know about this! It would be a great improvement if we could support this.

A few notes off the top of my head:

  • It would be nice to be able to build limited-api wheels just by specifying the lowest python version's limited API e.g. CIBW_LIMITED_API=cp34. Builds below that python version (that haven't been skipped) would still build individual wheels. Builds at or above would build a limited-api wheel if there isn't one already.
  • We'd still like to test the limited-api wheels on many versions of Python. So those builds would run as normal, but if there's already a suitable limited-api wheel in the output directory (from a previous limited-api build), skip the build and just run the tests using that.
  • Can support for the limited-api be detected from setup.py? It would be lovely to just do the right thing without adding a new option!
  • Currently we use pip wheel to make the wheels. We'd have to switch to python setup.py bdist_wheel instead. I don't think it's a huge change though.

@jlaine
Copy link
Contributor Author

jlaine commented Jul 1, 2018

If we don't want to introduce an extra option, and want to continue using pip wheel we could check setup.cfg for the py_limited_api option, I believe this should be picked up correctly:

http://wheel.readthedocs.io/en/stable/#defining-the-python-version
https://bitbucket.org/pypa/wheel/pull-requests/69/add-py-limited-api-flag-to-control-abi3/diff

I'm not sure what happens if you're building with an older Python though.

For the testing phase: I definitely agree, we want to test all Python versions.

@jlaine
Copy link
Contributor Author

jlaine commented Jul 1, 2018

Here's a little experiment, I set py_limited_api=cp35 on a branch of pylibsrtp, see:

jlaine/pylibsrtp@fec3b43

Here are the results:

https://travis-ci.org/jlaine/pylibsrtp/builds/398909104
https://ci.appveyor.com/project/jlaine/pylibsrtp/build/1.0.98

The build actually succeeds on Linux! On OS X I get a "file already exists" error and Windows just bombs.

@jlaine
Copy link
Contributor Author

jlaine commented Jul 1, 2018

It seems tooling for Py_LIMITED_API on Windows is not quite there yet:

pypa/pip#4445

@YannickJadoul
Copy link
Member

@jlaine Yeah, ok, those are good arguments indeed :-) But I think it's important to maybe explicitly list them and keep them in the back of our minds.

@joerick pip wheel calls python setup.py bdist_wheel, no? As far as I know, the main difference is that pip wheel creates a temporary directory to build, instead of creating one next to setup.py. Maybe there are a few other differences, but I can't directly find them (https://pip.pypa.io/en/stable/reference/pip_wheel/). But that sometimes also results in minor problems, apparently: https://stackoverflow.com/questions/41643744/pip-wheel-much-slower-than-setup-py-bdist-wheel

@joerick
Copy link
Contributor

joerick commented Jul 2, 2018

@YannickJadoul looks like the --build-option argument to pip wheel will allow us to supply other options to bdist_wheel.

@jlaine great catch on the setup.cfg option! But it's going to be a problem while Windows refuses to build when including that option. I think therefore have a few options

  • wait for a stable ABI on windows (could be a long time?)
  • request to the wheel folks that windows ignores the setup.cfg option for now
  • build this using a CIBW_LIMITED_API option such that we can control when to supply the --build-option --py-limited-api=cpXX

I'm leaning toward the latter, but open to other opinions too.

@mattip
Copy link
Contributor

mattip commented Jan 30, 2021

The packaging issue was solve a while ago. pnumpy is using it via the setup.cfg directive. But there is still a problem when running the matrix since, I think, an identical wheel is built for each version of python. This is fine for linux and mac, but windows complains with

shutil.Error: Destination path 'wheelhouse\pnumpy-2.0.3.post0+g4566478.d20210120-cp36-abi3-win_amd64.whl' already exists

There should be some kind of a check that an abi3 wheel is only built once, preferably with the lowest supported version of CPython.

@henryiii
Copy link
Contributor

We could have CIBW_LIMITED_API but take the default from setup.cfg if it exists?

@henryiii
Copy link
Contributor

Also, shouldn't you just build for one Python version with CIBW_BUILD?

@joerick
Copy link
Contributor

joerick commented Jan 30, 2021

I keep meaning to come back to this issue! cibuildwheel should definitely support this. Design-wise, we should read it from pyproject.toml, setup.cfg, or setup.py, and if we really need it, a fallback CIBW_LIMITED_API option too.

If there's a limited-api set, we'd build the limited-api wheel once (probably on the oldest version selected by BUILD/SKIP and the limited abi). Subsequent runs with newer Pythons should skip the build step, because we already built a compatible wheel, but should still use it to run tests. On Pythons not compatible with the limited-api, we'd just do a normal platform wheel build.

Ideally, the user should be able to just add py-limited-api and our minimal config should do the right thing.

I haven't full thought it through, but I think this might be simple-ish to add into the codebase, by making the build parts of the main build loop conditional on whether a abi-compatible wheel is already in the output_dir.

@YannickJadoul
Copy link
Member

YannickJadoul commented Jan 30, 2021

One currently available workaround is to just select one Python version, ofc. CIBW_BUILD="{cp,pp}37-*"?

In general, how would any cibuildwheel support for Py_LIMITED_API play together with our current set of (Python-version-specific) build identifiers?

@henryiii
Copy link
Contributor

select one Python version

Yes, this even lets you select the version you want to build on. Maybe just detecting that a wheel already exists and has a limited ABI name should cause a warning or an error, on all platforms? It's wasteful to rebuild the same wheels, even if it allowed on Unix, and means that the final wheel depends on what order we build in (which could change).

general, how would any cibuildwheel support for Py_LIMITED_API

I assume it would change the set to ones that started cp3- or something like that. But then you'd have to choose the version to build on. Honestly, it seems simpler to just ask users to pick the version they want with CIBW_BUILD, possibly with a nicer error/warning message if they try to build the same wheel on multiple Pythons.

@YannickJadoul
Copy link
Member

Maybe just detecting that a wheel already exists and has a limited ABI name should cause a warning or an error, on all platforms?

Judging from @mattip's comment (#78 (comment)) I thought it already was an error? Not the clearest, though

@YannickJadoul
Copy link
Member

YannickJadoul commented Jan 30, 2021

Just a line or 2 in the docs' FAQ might also just do the trick, though?

@henryiii
Copy link
Contributor

It's not an error on Linux or macOS; I rather think it should be, you are waisting resources by just rebuilding the same wheel and replacing it over and over. And on Windows, the error makes it seem like it's something cibuildwheel should support, while if the error said "Wheel would be overwritten; it looks like you are making limited ABI wheels; please limit your Python selection to one version when building limited ABI wheels with CIBW_BUILD", then users would do that instead of coming here. :)

PS: Not very familiar with Limited ABI; pybind11 doesn't support it, and Cython/SWIG either don't or just partially do, so don't know of anything using it); basing the above mostly on the comments above.

@henryiii
Copy link
Contributor

And agree about adding a line or two in the FAQ, too.

@YannickJadoul
Copy link
Member

It's not an error on Linux or macOS; I rather think it should be, you are waisting resources by just rebuilding the same wheel and replacing it over and over.

Ah, that's the part I overlooked! Thanks!

@mayeut
Copy link
Member

mayeut commented Jan 31, 2021

While building for abi3 is already be possible when taking appropriate actions, what I like about @joerick proposition is the ability to build once but test on every available version.

@henryiii
Copy link
Contributor

Yes, longer term, it would be nice to have multiple Pythons selected cause later ones to be tested with the wheel built on the earlier one (though a user can set this up afterwards with nox or something, as well). It's distinctly different than the current situation, where on Unix, the wheel is rebuilt on each Python version and replaced, which doesn't test its ability to be loaded on multiple Pythons at all! (which is why #569 is a strict improvement).

This would be a very good reason to keep the builds going from earliest to latest Python version, though. I do think #536 will be enough to really help out on #468 so that reordering the builds is not needed to improve on that.

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