RFC: restrict `wcs` extension modules to CPython's limited API by neutrinoceros · Pull Request #18231 · astropy/astropy

neutrinoceros · 2025-06-07T17:55:39Z

Description

Following @da-woods' advice, I think I got most of this right, but I'm still struggling to resolve the last compilation-error I'm getting, namely:

astropy/wcs/src/astropy_wcs.c:774:28: error: initializer element is not a compile-time constant
  774 | static PyObject* WcsType = PyType_FromSpec(&WCSBaseClass_spec);
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

At this point, I'd like to ask @astrofrog and @da-woods for a hand solving this last bit.
Partially address #18163

update: notes to reviewers

in a couple places I added FIXME comments indicating what I believe are bug-for-bug refactors, where I found some methods defined but not included in their natural parent structs
a couple of functions were simply replaced with a slower equivalent (e.g. Py_List_GET_ITEM -> Py_List_GetItem). It should be possible to instead select the version we want at compile time using #ifdef ... pre-proc directives (if we keep LIMITED_API as a opt-in build option). Let me know if this (small) additional complexity is desired.
because my primary source of information when working on this were compilation errors and warnings, I ended up fixing many unrelated but noisy warning about mismatched pointer types (i.e. PY_XDECREF expects a PyObject* pointer)

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2025-06-07T17:56:09Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2025-06-07T17:56:12Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

astrofrog · 2025-06-07T17:59:37Z

@neutrinoceros just to check, did you forget to paste the error?

neutrinoceros · 2025-06-07T18:02:18Z

Whoops, yes ! thanks for your quick response !

astrofrog · 2025-06-09T17:20:44Z

What an epic effort - nice work! 👏

astrofrog · 2025-06-18T21:52:07Z

@neutrinoceros - could you rebase this so that it picks up the multiple limited API CI jobs?

astrofrog · 2025-06-18T21:54:36Z

@mcara @nden - just to check, would either of you have time in the near future to review this? Or would you be happy for others to review/approve if ready?

neutrinoceros · 2025-06-19T09:18:51Z

rebased !

mcara · 2025-06-23T16:51:23Z

I would prefer to review myself in order to understand (for my own education) the proposed changes but I will not be able to look at this PR in detail for the next 2 weeks. At first glance (without me following all the links to CPython and initial issues), changes look non-controversial. So I am fine with someone else reviewing this PR, especially if time is critical.

neutrinoceros · 2025-07-02T14:17:46Z

rebased with no modifications to refresh CI

mhvk

I tried to have a look, and made some comments, in particular about decref'ing what I think are borrowed references (from Py_TYPE). Not sure I'm correct, or that I caught all. Am a bit worried that, if my reasoning is correct, this is not caught by testing. In numpy, there is a test run that somehow checks things more carefully -- cannot easily find out right now what that is (am travelling), but can try next week.

neutrinoceros · 2025-07-07T12:32:32Z

Time is still not critical, but I'd like to emphasize that this the last remaining PR for making astropy (as a whole) limited-api compliant.

mhvk

With my earlier worries proven wrong, I think this is all OK, so approving. I'll leave merging to the wcs team, though.

Really nice work!

astrofrog · 2025-07-07T14:32:53Z

@mcara @nden - this should be merged before my preserve_units PR, happy to rebase mine

mcara

LGTM. Thank you!

I run regression tests on drizzlepac and they all pass. All stwcs and tweakwcs unit tests pass as well.

neutrinoceros added this to the v7.2.0 milestone Jun 7, 2025

neutrinoceros added build Refactoring labels Jun 7, 2025

github-actions Bot added the wcs label Jun 7, 2025

neutrinoceros changed the title ~~BLD: refactor astropy_wcs Cython module for compatibility with CPython's limited API~~ BLD: refactor astropy_wcs Cython module for compatibility with CPython's limited API Jun 7, 2025

da-woods reviewed Jun 7, 2025

View reviewed changes

Comment thread astropy/wcs/src/astropy_wcs.c Outdated

Comment thread astropy/wcs/src/astropy_wcs.c

da-woods reviewed Jun 7, 2025

View reviewed changes

Comment thread astropy/wcs/src/astropy_wcs.c Outdated

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 4b64975 to fac10b9 Compare June 7, 2025 18:07

da-woods reviewed Jun 7, 2025

View reviewed changes

Comment thread astropy/wcs/src/astropy_wcs.c Outdated

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch 2 times, most recently from 0713776 to 4e74776 Compare June 7, 2025 18:24

neutrinoceros added the no-changelog-entry-needed label Jun 7, 2025

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 4e74776 to 26acbf4 Compare June 7, 2025 18:26

neutrinoceros marked this pull request as ready for review June 7, 2025 18:38

neutrinoceros requested a review from mcara as a code owner June 7, 2025 18:38

neutrinoceros changed the title ~~BLD: refactor astropy_wcs Cython module for compatibility with CPython's limited API~~ BLD: refactor astropy_wcs extension module for compatibility with CPython's limited API Jun 8, 2025

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 26acbf4 to a5249ed Compare June 8, 2025 08:12

neutrinoceros changed the title ~~BLD: refactor astropy_wcs extension module for compatibility with CPython's limited API~~ BLD: restrict C-API usage in wcs extensions to Limited API Jun 8, 2025

neutrinoceros marked this pull request as draft June 8, 2025 08:13

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from a5249ed to 8edb949 Compare June 8, 2025 08:19

da-woods reviewed Jun 8, 2025

View reviewed changes

Comment thread astropy/wcs/src/str_list_proxy.c

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch 3 times, most recently from d6b457b to 161d76b Compare June 8, 2025 12:13

neutrinoceros marked this pull request as ready for review June 9, 2025 16:40

da-woods reviewed Jun 9, 2025

View reviewed changes

Comment thread astropy/wcs/src/pyutil.c Outdated

neutrinoceros requested a review from taldcroft as a code owner June 10, 2025 07:13

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch 2 times, most recently from 9ac87c6 to fd79957 Compare June 10, 2025 07:29

da-woods reviewed Jun 10, 2025

View reviewed changes

Comment thread astropy/wcs/src/pyutil.c Outdated

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch 2 times, most recently from 0ab79bb to 65b7559 Compare June 10, 2025 08:21

neutrinoceros mentioned this pull request Jun 10, 2025

Run limited API build on Mac in addition to Linux #18250

Merged

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 65b7559 to 46f5179 Compare June 19, 2025 09:18

astrofrog mentioned this pull request Jun 24, 2025

Implement option in WCS class to preserve the original units #18338

Merged

14 of 18 tasks

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 46f5179 to 0982f1c Compare July 2, 2025 14:17

mhvk reviewed Jul 4, 2025

View reviewed changes

Comment thread astropy/wcs/src/pyutil.c

Comment thread astropy/wcs/src/wcslib_celprm_wrap.c

Comment thread astropy/wcs/src/wcslib_prjprm_wrap.c

Comment thread astropy/wcs/src/wcslib_tabprm_wrap.c

Comment thread astropy/wcs/src/wcslib_wrap.c

Comment thread astropy/wcs/src/wcslib_wtbarr_wrap.c

neutrinoceros mentioned this pull request Jul 4, 2025

RFC: restrict utils.xml's extension modules to CPython's Limited API #18249

Merged

1 task

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 0982f1c to 5ee3035 Compare July 4, 2025 15:38

neutrinoceros mentioned this pull request Jul 7, 2025

RFC: restrict cosmology extension modules to CPython's Limited API #18261

Merged

1 task

RFC: restrict wcs extension modules to CPython's limited API

93c37b1

neutrinoceros force-pushed the bld/wcs/limited_api_compat branch from 5ee3035 to 93c37b1 Compare July 7, 2025 12:30

mhvk approved these changes Jul 7, 2025

View reviewed changes

mcara approved these changes Jul 11, 2025

View reviewed changes

mcara merged commit 91fc8bc into astropy:main Jul 11, 2025

neutrinoceros deleted the bld/wcs/limited_api_compat branch July 11, 2025 13:57

Conversation

neutrinoceros commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Jun 7, 2025

Uh oh!

github-actions Bot commented Jun 7, 2025

Uh oh!

astrofrog commented Jun 7, 2025

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Jun 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astrofrog commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

astrofrog commented Jun 18, 2025

Uh oh!

astrofrog commented Jun 18, 2025

Uh oh!

neutrinoceros commented Jun 19, 2025

Uh oh!

mcara commented Jun 23, 2025

Uh oh!

neutrinoceros commented Jul 2, 2025

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Jul 7, 2025

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Jul 7, 2025

Uh oh!

mcara left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neutrinoceros commented Jun 7, 2025 •

edited

Loading