RFC: restrict `cosmology` extension modules to CPython's Limited API by neutrinoceros · Pull Request #18261 · astropy/astropy

neutrinoceros · 2025-06-11T12:51:15Z

Description

Ref #18163
This should be the last (non-infrastructural) missing piece (!)

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-11T12:51:37Z

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-11T12:51:40Z

👋 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?

neutrinoceros · 2025-06-11T12:59:10Z

Build seems completely broken on CI but somehow works on my machine, talk about a disapointment... let me see what's gone wrong

neutrinoceros · 2025-06-11T15:48:15Z

Looks like I broke runtime when I fixed build time 🙈
Will come back to this tomorrow

da-woods · 2025-06-11T17:41:59Z

        Py_INCREF(self);
        return self;
    }
    return PyMethod_New(self, obj);


It is possible to replace this. I'd use Cython's implementation as a starting point https://github.com/cython/cython/blob/dba606bc50e90dbaf37850779d1f84f1c0a22c7a/Cython/Utility/ObjectHandling.c#L2977

Essentially we look-up the method type in types.MethodType, cache it (for speed) and then call it as a Python object.

I think you do need to provide this code, however you really don't want this code to actually be called - it's very slow.

What you want to do (in addition) is to set the Py_TPFLAGS_METHOD_DESCRIPTOR type-flag on the type. This means that Python can usually avoid going through the __get__ code. I don't think you'll have to change your call code at all - just set the flag.

The Limited API code is probably bad enough here to be worth keeping the old version and guarding it with #if !defined(Py_LIMITED_API)

Cython does some opaque stuff in its utility code that I think you're struggling to see through. I can give more detailed instructions (i.e. the exact code where it needs to be) later today if that helps.

yes, I was just about to ask actually. That would be tremendously helpful, thank you !

nstarman

I don't know enough C to meaningfully review this. But I trust y'all 😰 😆

da-woods

Suggestions aren't tested so there may be small typos, but hopefully should be very close to working.

neutrinoceros · 2025-06-12T18:17:17Z

Fantastic, thanks a lot @da-woods, I couldn't have done this without your help !
I included all your suggestions as is (no typos whatsoever) and squashed my commits back, this time putting you as a co-author. It should be stable now but I'll wait for CI to go green before I actually undraft

nstarman

👍

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

@nstarman do you want to formally approve this one, or would you like a second opinion ?

nstarman · 2025-07-02T14:24:53Z

2nd opinion plz.

neutrinoceros · 2025-07-02T14:29:33Z

Who can we ask a review from ? I think David has more expertise there than most (all ?) of us (combined ? 😅)

da-woods

One change and a couple of things that might be fixed as follow ups. But it looks good to me. Although I'm not the most impartial on this.

da-woods · 2025-07-02T17:49:36Z


    PyObject *since_unicode = PyObject_Str(self->since);
-    const char* since_utf8 = PyUnicode_AsUTF8(since_unicode);
+    const char* since_utf8 = PyUnicode_AsUTF8AndSize(since_unicode, NULL);


Propably something for a follow-up PR rather than to fix here, but I think that you could save a lot of this by using PyErr_WarnFormat later. (Unless I'm missing something which is completely possible...)

This module was my very first experience using Python's C-API and isn't really polished in any way. In any case it's also temporary code that we'll soon get rid of anyway, so it doesn't matter much if the slow path is sub-optimal.

Fair enough - that seems reasonable to leave as is then

da-woods · 2025-07-02T17:50:59Z

+        {Py_tp_members, depr_kws_wrap_members},
+        {Py_tp_call, (ternaryfunc)depr_kws_wrap_call},
+        {Py_tp_descr_get, depr_kws_wrap_get},
+        {0, NULL},


Again, something for a follow-up PR since it's not a new problem, but this type looks like it needs GC.

Do you mean I should set Py_TPFLAGS_HAVE_GC (and abide the contract that comes with it) ?

Yes I think so. It looks like the type has a dict and I think wrapped is an arbitrary Python callable, so I think it can participate in reference loops.

This doesn't change with the Limited API though, so personally I'd do it separately.

Fine with me. Thanks for the tip !

mhvk

Thought I might as well look at this one too - another possible wrong DECREF, but otherwise would seem to make sense.

neutrinoceros · 2025-07-04T14:40:01Z

@mhvk I'm not seeing the specific comment you're alluding to. Did you forget to submit it ?

Co-authored-by: da-woods <[email protected]>

mhvk

Approving since my one comment turned out to be based on a misunderstanding.

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

neutrinoceros added cosmology Refactoring no-changelog-entry-needed Limited API labels Jun 11, 2025

github-project-automation Bot added this to Cosmology, the Expansion Jun 11, 2025

github-project-automation Bot moved this to Triage in Cosmology, the Expansion Jun 11, 2025

neutrinoceros mentioned this pull request Jun 11, 2025

Using the limited Python API in astropy #18163

Closed

github-actions Bot added the testing label Jun 11, 2025

neutrinoceros added the build label Jun 11, 2025

da-woods reviewed Jun 11, 2025

View reviewed changes

nstarman reviewed Jun 11, 2025

View reviewed changes

Comment thread .github/workflows/ci_workflows.yml

neutrinoceros force-pushed the cosmology/rfc/limited-api-compat branch 2 times, most recently from 0a9d98a to f1cb13b Compare June 12, 2025 08:55

da-woods reviewed Jun 12, 2025

View reviewed changes

neutrinoceros force-pushed the cosmology/rfc/limited-api-compat branch 2 times, most recently from e87f8c8 to 9e4da2d Compare June 12, 2025 18:14

neutrinoceros marked this pull request as ready for review June 12, 2025 18:32

neutrinoceros requested a review from a team as a code owner June 12, 2025 18:32

neutrinoceros requested a review from nstarman June 12, 2025 18:32

nstarman reviewed Jun 12, 2025

View reviewed changes

da-woods reviewed Jul 2, 2025

View reviewed changes

neutrinoceros force-pushed the cosmology/rfc/limited-api-compat branch from 3ea6406 to 2476a79 Compare July 2, 2025 19:41

mhvk reviewed Jul 4, 2025

View reviewed changes

Comment thread astropy/cosmology/_src/signature_deprecations.c

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

cb04af7

Co-authored-by: da-woods <[email protected]>

neutrinoceros force-pushed the cosmology/rfc/limited-api-compat branch from 2476a79 to cb04af7 Compare July 4, 2025 15:41

mhvk approved these changes Jul 7, 2025

View reviewed changes

github-project-automation Bot moved this from Triage to Reviewer approved in Cosmology, the Expansion Jul 7, 2025

mhvk merged commit 70d7f62 into astropy:main Jul 7, 2025

github-project-automation Bot moved this from Reviewer approved to Done in Cosmology, the Expansion Jul 7, 2025

neutrinoceros deleted the cosmology/rfc/limited-api-compat branch July 7, 2025 11:53

Conversation

neutrinoceros commented Jun 11, 2025

Description

Uh oh!

github-actions Bot commented Jun 11, 2025

Uh oh!

github-actions Bot commented Jun 11, 2025

Uh oh!

neutrinoceros commented Jun 11, 2025

Uh oh!

neutrinoceros commented Jun 11, 2025

Uh oh!

da-woods Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

da-woods left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Jun 12, 2025

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Jul 2, 2025

Uh oh!

nstarman commented Jul 2, 2025

Uh oh!

neutrinoceros commented Jul 2, 2025

Uh oh!

da-woods left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Jul 4, 2025

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

da-woods Jun 11, 2025 •

edited

Loading