search

astropy / extension-helpers

Helpers to assist with building Python packages with compiled C/Cython extensions
https://extension-helpers.readthedocs.io
BSD 3-Clause "New" or "Revised" License
16 stars 12 forks source link

Automatically set compiler flags to target PEP 384 Python limited API #26

Open lpsinger opened 3 years ago

lpsinger commented 3 years ago

PEP 384 introduced a Python limited API that allows you to build one wheel that targets all Python versions on a given platform.

Setuptools supports opting in to building wheels for the Python limited API by adding the following lines to setup.cfg:

[bdist_wheel]
py_limited_api = cp36  # replace with desired min Python version

However, there are two needed steps that Setuptools does not do automatically:

  1. Pass the py_limited_api=True keyword argument to each Extension
  2. Set the Py_LIMITED_API preprocessor macro to the version hex value for the desired minimum Python version

This patch teaches get_extension() to do those two missing steps automatically if the limited API is enabled in the setup.cfg file.

codecov[bot] commented 3 years ago

Codecov Report

Merging #26 (6547724) into main (51636f3) will decrease coverage by 1.39%. Report is 94 commits behind head on main. The diff coverage is 43.47%.

:exclamation: Current head 6547724 differs from pull request most recent head cdfeeac. Consider uploading reports for the commit cdfeeac to get more accurate results

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
- Coverage   74.65%   73.27%   -1.39%     
==========================================
  Files           9        9              
  Lines         513      535      +22     
==========================================
+ Hits          383      392       +9     
- Misses        130      143      +13
Files Changed Coverage Δ
extension_helpers/_setup_helpers.py 61.98% <43.47%> (-4.69%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Cadair commented 3 years ago

It would be good if we could work out some tests for this, the difficultly of writing tests and the benefit of having them are both high in this package :D

lpsinger commented 3 years ago

It would be good if we could work out some tests for this, the difficultly of writing tests and the benefit of having them are both high in this package :D

For sure. I just wanted to get some feedback on the idea before writing a test case.

Cadair commented 3 years ago

:+1: makes sense. The idea seems very good, if we can reduce the wheel build time by 1/3 I am very onboard.

astrofrog commented 3 years ago

Yes agree this would be great!

astrofrog commented 3 years ago

@lpsinger - I don't think there are any objections, so feel free to try and add a test for this if possible - and let us know if it's not.

lpsinger commented 3 years ago

BTW, it looks like the Cython support has come a long way, but Cython 3.0 is still in alpha and has been in alpha for a very long time.

astrofrog commented 3 years ago

@lpsinger - thanks for the update! I have a number of packages that don't use Cython that this could be very useful for, so looking forward to using it :)

lpsinger commented 3 years ago

What do people think about depending on alpha versions of Cython?

Cadair commented 3 years ago

I certainly wouldn't want to require it, but having it opt-in to use the new stuff wouldn't be too bad.

lpsinger commented 3 years ago

Sorry that progress on this stalled. I'll try and get back to it soon.

astrofrog commented 2 years ago

There is a minor conflict that needs to be resolved - could you rebase?

I'm really excited about trying this out as once thing that frustrates me is to keep having to build wheels for newer Pythons for packages that are ultra-stable and haven't had a new release for a couple of years. One practical question I have (unrelated to extension-helpers) is how to properly test the resulting wheels - I'm assuming one would build wheels with e.g. cibuildwheel for say cp36* but cibuildwheel doesn't really have a mechanism for building on Python 3.6 and testing the wheel on Python 3.9. @lpsinger do you just build and test the 3.6 wheels and then trust it will work on more recent Python versions?

lpsinger commented 2 years ago

@lpsinger do you just build and test the 3.6 wheels and then trust it will work on more recent Python versions?

Yes. And you're right that cibuildwheel doesn't do this yet... pypa/cibuildwheel#78

astrofrog commented 2 years ago

I've experimented with the limited API a little and I have an idea for a test to add, which is to add an extension that actually does not work with the limited API, and then try and build and use the package and check that it gives an error about not being able to load a symbol (this will tell us that the limited API option is indeed being passed through). Checking a working example is interesting too but not sufficient to really check things are working since it would work fine without the limited API option.

lpsinger commented 2 years ago

@lpsinger do you just build and test the 3.6 wheels and then trust it will work on more recent Python versions?

I'm sorry, I misspoke. I haven't tried this with cibuildwheel yet. For my own projects that use the limited API, I build once using Python 3.6 (or 3.7) and then install the wheels and run under all supported versions of Python.

astrofrog commented 2 years ago

So I just realized that the main code changes here depend on get_distutils_option which has since been removed since distutils is deprecated and direct use of setup.py is also deprecated. Just to check, do we actually need any changes in extension-helpers since users really just need to edit setup.cfg and then add the required option in Extension? I guess there is one case (single Cython files) where we auto-generate the Extension, and in that case we might want to add the correct flag if the limited API is set in setup.cfg?

lpsinger commented 2 years ago

Just to check, do we actually need any changes in extension-helpers since users really just need to edit setup.cfg and then add the required option in Extension?

Normally you have two make two changes to your setup.cfg:

  1. Add py_limited_api = cp36 to the [bdist_wheel] section
  2. Add the preprocessor definition to each of your extensions

The purpose of this PR is to make it so that the developer only has to do step 1, and step 2 is done automatically.

lpsinger commented 2 years ago

So I just realized that the main code changes here depend on get_distutils_option which has since been removed since distutils is deprecated and direct use of setup.py is also deprecated.

Setuptools does not yet support C extension modules in setup.cfg-only projects (pypa/setuptools#2220). (Cython .pyx extensions are supported, but that's a different matter.) Would it be a problem to bring back get_distutils_option?

astrofrog commented 2 years ago

Just a quick note that with https://github.com/astropy/extension-helpers/pull/33 we can use extension helpers with setup.cfg only projects. I would prefer simply advocating that the option to Extension for the limited API is done explicitly in setup_package.py files rather than adding back distutils-related functionality.

Cadair commented 2 years ago

Isn't the use of get_distutils_option here just parsing the config file? We already do that in other ways?

I quite like the idea of including this in a nice easy way, it seems like a great feature. Not at the expense of relying on distutils again though having just removed it.

astrofrog commented 2 years ago

Ah yes my bad, we should just parse the config file directly to check for the option

astrofrog commented 2 years ago

@lpsinger - see here:

https://github.com/astropy/extension-helpers/blob/55b7859e08d4cf711ab3037ca7df1ab64b59c922/extension_helpers/__init__.py#L17-L21

For an example of how we parse an option from setup.cfg

astrofrog commented 2 years ago

@lpsinger just to check, is this something you might have time to work on in the near future? If not, one of us could maybe try and wrap it up - just let me know!

lpsinger commented 2 years ago

@lpsinger just to check, is this something you might have time to work on in the near future? If not, one of us could maybe try and wrap it up - just let me know!

I think I can perhaps do it next week?

astrofrog commented 2 years ago

That would be great, thanks! :)

astrofrog commented 2 years ago

@lpsinger - just to check, is this something you would have time to work on?

lpsinger commented 2 years ago

Perhaps late next week.

astrofrog commented 2 years ago

Just a note that I started experimenting with Cython and the limited API and I can't get it to work even with a simple example:

https://groups.google.com/g/cython-users/c/-hasuL2VEdo/m/LFaf70PYAwAJ?utm_medium=email&utm_source=footer

This is even with Cython 3.0.0a11. Unless I'm missing something, with this PR we should probably add a note that things won't work for Cython code yet.

lpsinger commented 1 year ago

Cython 3.0.0 is out now and has initial support for Limited API](https://cython.readthedocs.io/en/latest/src/changes.html#initial-support-for-limited-api) (though nontrivial Cython extensions might fail). It looks like cibuildwheel supports it too.

Regardless of the Cython status, this is still valuable for projects like astropy-healpix that have C extensions but not through Cython.

I'm going to try to clean this PR up now.