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

Use Python Limited API #6171

Merged
merged 2 commits into from
May 26, 2022
Merged

Use Python Limited API #6171

merged 2 commits into from
May 26, 2022

Conversation

ConorMacBride
Copy link
Member

Not sure if we want to do this. Basically, it compiles the extensions in such a way that they are compatible with all the Python versions, so we only need one wheel per OS. It will still test the wheel across all supported Python version so there won't be much of a speed up when publishing.

The ANA tests passed for me locally on macOS. Will see if all the tests pass in the CI. This is assuming the tests have good coverage of the C extension.

This is based on astrofrog/fast-histogram#56. See also PEP 384 – Defining a Stable ABI.

@ConorMacBride ConorMacBride added Discussion An issue opened for, or undergoing discussion. Run publish Run publish CI on this PR labels May 21, 2022
@nabobalis
Copy link
Contributor

Is there a downside?

@Cadair
Copy link
Member

Cadair commented May 23, 2022

I have no issues with this, I know of no downsides. It would be interesting to get astropy/extension-helpers#26 in before we do this though.

@ConorMacBride
Copy link
Member Author

Okay. This PR adds to sunpy what the extension-helpers PR would add automatically. (I accidentally set the Py_LIMITED_API macro for cp36 instead of cp38, which I've now fixed here.)

@@ -110,6 +110,9 @@ asdf.resource_mappings =
asdf.extensions =
sunpy = sunpy.io.special.asdf.entry_points:get_extensions

[bdist_wheel]
py_limited_api = cp38
Copy link
Member

Choose a reason for hiding this comment

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

Put a comment here saying if you change this you need to change the line in _pyana.c or else we will forget.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the comment. Here's the working CI from before the comment was added: https://github.com/sunpy/sunpy/actions/runs/2371727589

Copy link
Contributor

Choose a reason for hiding this comment

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

Silly question, how does one get the version hex? Is it just a conversion of the number to hexadecimal?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ConorMacBride ConorMacBride marked this pull request as ready for review May 25, 2022 18:17
@ConorMacBride ConorMacBride requested a review from a team as a code owner May 25, 2022 18:17
@Cadair
Copy link
Member

Cadair commented May 25, 2022

If we rebase this we can see the CI run?

@ConorMacBride
Copy link
Member Author

Does this need a changelog?

@nabobalis
Copy link
Contributor

Up to you, we could add a trivial one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion An issue opened for, or undergoing discussion. Run publish Run publish CI on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants