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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Feb 9, 2021

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
Copy link

codecov bot commented Feb 9, 2021

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%.

❗ 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%) ⬇️

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

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @lpsinger this looks really interesting.

ext.py_limited_api = True
ext.define_macros.append(('Py_LIMITED_API', version_hex))
else:
log.warn(_PEP_384_WARNING)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the limitations of the limited API really, but does numpy compile against it? I am just wondering if this warning is too keen, i.e. if most of our users (scientific python packages with numpy C dep) are likely to be able to make use of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downstream packages that compile against and link with Numpy can use the limited API. I maintain one such package. Here are the wheels: https://pypi.org/project/ligo.skymap/#files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major limitation is that Cython support for the limited API is very, very new: cython/cython#3223

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you could make astropy core compile with this set 🤔 that would certainly be a good indicator that most things could use it.

Thanks for the extra info.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to make sure I understand, people will see the warning if they don't opt-in to this? If so I wonder if we should perhaps not do that, or at least make it clear to people how to get rid of the warning if they don't want to opt in to the limited API?

extension_helpers/_setup_helpers.py Show resolved Hide resolved

Python limited API
------------------

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a similar vein to my other comment, it would be nice to explain when using this is appropriate and when it isn't as I expect most people don't even know this exists (I didn't) and why they would want to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right! It's not a well-known feature, although it seems to be well supported by pip. Part of the aim of this PR is to raise awareness by making it easy for Astropy affiliated packages to opt in.

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.
@Cadair
Copy link
Member

Cadair commented Feb 10, 2021

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
Copy link
Contributor Author

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
Copy link
Member

Cadair commented Feb 10, 2021

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

@astrofrog
Copy link
Member

Yes agree this would be great!

@astrofrog
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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

@Cadair
Copy link
Member

Cadair commented Apr 29, 2021

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

@lpsinger
Copy link
Contributor Author

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

@astrofrog
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

astrofrog commented Feb 23, 2022

Just a quick note that with #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
Copy link
Member

Cadair commented Feb 23, 2022

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
Copy link
Member

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

@astrofrog
Copy link
Member

@lpsinger - see here:

cfg = ConfigParser()
cfg.read(config_files[0])
if (cfg.has_option("extension-helpers", "use_extension_helpers") and
cfg.get("extension-helpers", "use_extension_helpers").lower() == 'true'):
distribution.ext_modules = get_extensions()

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

Comment on lines +45 to +47
def _version_info_to_version_hex(major=0, minor=0, micro=0,
releaselevel=0, serial=0):
return f'0x{major:0>2}{minor:0>2}{micro:0>2}{releaselevel:0>2}{serial:0>2}'
Copy link
Member

@ConorMacBride ConorMacBride May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the Python API and ABI Versioning docs I think this would need to be:

Suggested change
def _version_info_to_version_hex(major=0, minor=0, micro=0,
releaselevel=0, serial=0):
return f'0x{major:0>2}{minor:0>2}{micro:0>2}{releaselevel:0>2}{serial:0>2}'
def _version_info_to_version_hex(major=0, minor=0):
"""Returns a PY_VERSION_HEX for {major}.{minor).0"""
return f'0x{major:02x}{minor:02x}0000'

Currently for cp310, {10:0>2} would give 10 instead of 0a, and the release level should be f for a final release. (Edit: according to this python doc setting the release level to 0 should be fine.)

extension_helpers/_setup_helpers.py Outdated Show resolved Hide resolved
extension_helpers/_setup_helpers.py Outdated Show resolved Hide resolved
abi = get_distutils_option('py_limited_api', ['bdist_wheel'])
if abi:
version_info = _abi_to_version_info(abi)
if version_info:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't raising an error/warning if py_limited_api is specified but does not conform to the pattern, however, pypa/wheel will raise an error in this case so this should be fine.

extension_helpers/_setup_helpers.py Show resolved Hide resolved
@astrofrog
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

That would be great, thanks! :)

@astrofrog
Copy link
Member

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

@lpsinger
Copy link
Contributor Author

Perhaps late next week.

@astrofrog
Copy link
Member

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
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

4 participants