Make iterparse.c work with opaque Python objects by da-woods · Pull Request #20504 · astropy/astropy

da-woods · 2026-09-29T18:09:01Z

Description

This is mainly a case of:

introducing a cast macro to either do the call to PyObject_GetTypeData or do the C cast depending on the mode.
changing the non-internal functions to accept PyObjects* rather than the data struct.
-ve size on the type spec.

I've added the ASTROPY_OPAQUE_OBJECTS macros which can be defined to force it to compile opaque objects mode. This is optional (i.e. you could take it out of the PR)
but convenient for development. Note that you could just switch to doing the opaque object unconditionally since it'd be supported from Python 3.12 upwards. That trades a small performance benefit for a single code-path.

Part of #19479

AI Disclosure

No AI. Just a note that it's all pretty mechanical so AI would probably do pretty well at the follow up PRsns with reviewers.

Merge method

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.

This is mainly a case of: * introducing a cast macro to either do the call to PyObject_GetTypeData or do the C cast depending on the mode. * changing the non-internal functions to accept `PyObjects*` rather than the data struct. * -ve size on the type spec. I've added the `ASTROPY_OPAQUE_OBJECTS` macros which can be defined to force it to compile opaque objects mode. This is optional (i.e. you could take it out of the PR) but convenient for development. Note that you could just switch to doing the opaque object unconditionally since it'd be supported from Python 3.12 upwards. That trades a small performance benefit for a single code-path.

github-actions · 2026-09-29T18:09:13Z

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.

da-woods · 2026-09-29T18:13:15Z

When doing the rest of it signature_deprecations.c has a small extra thing you need to do:

offsetof(DeprKwsObject, dict)

That (and the other offsetofs in the PyMemberDef) need Py_RELATIVE_OFFSET. Unfortunately that needs Python 3.14 to work at runtime for __dictoffset__ specifically. I don't know of a workaround, but it would mean that you couldn't change that file unconditionally.

da-woods · 2026-09-29T18:14:37Z

 * requested (as occurs with transparent decompression of the input
 * stream), and for the initial allocation to combine the logic in one place.
 */
 static int queue_realloc(IterParser *self, Py_ssize_t req_size)


These are internal functions that never treat self as a PyObject * so it's easier to pass the data struct and not a PyObject*. Hence they're unchanged.

da-woods · 2026-09-29T18:15:04Z

-        {Py_tp_dealloc, (destructor)IterParser_dealloc},
+        {Py_tp_dealloc, IterParser_dealloc},
        {Py_tp_doc, "IterParser objects"},
-        {Py_tp_traverse, (traverseproc)IterParser_traverse},


Casts are no longer needed because the signature with PyObject * matches (and because they were only really needed on the static type version anyway)

pllim · 2026-09-29T18:24:26Z

No AI.

But are you a human? 😆 j/k j/k Thanks!

xref #20492

pllim · 2026-09-29T18:26:47Z

pre-commit.ci autofix

da-woods · 2026-09-29T22:40:59Z

Generic comment on how to approach this:

Be very suspicious of any cast to/from PyObject *. So if you search in the source for * ) that'll find most of the places you need to change.
Be quite suspicious of any function arguments that are YourTypeStruct * and not PyObject * especially where the function is called by the Python interpreter. Again something to explicitly search for.

neutrinoceros

Since there are no plans to use ASTROPY_OPAQUE_OBJECTS yet, I think it's best to omit it for now.
Otherwise, and for the most part, this seems pretty clear to me. I only have one question I want to elucidate before merge.

it's all pretty mechanical so AI would probably do pretty well at the follow up PRs

I object to AI use in any shape or form. In this particular instance I think using AI would deprive us from the opportunity to learn about these details.
While my position isn't that of the project as a whole, I'll make a point not to use any LLM in continuing this work.

neutrinoceros · 2026-09-30T08:06:52Z

-    .basicsize = sizeof(IterParser),
+    .basicsize =
+#if defined(Py_TARGET_ABI3T) || defined(ASTROPY_OPAQUE_OBJECTS)
+        -(Py_ssize_t) // negative size indicates opaque PyObject


did you mean PY_SSIZE_T_MAX here ?
I don't understand how Py_ssize_t, which is a type, would be interpreted as a value.

No - it's casting the line afterwards (to avoid a c compile warning). Maybe that isn't written super-clearly but it's it evaluates as

.basic size = -(Py_ssize_t)sizeof(IterParser),

It might be clearer to have a bit more duplication and not put the ifdef guards in the middle of a line...

oh, I see. This is the "review before coffee" effect at work :)
#ifdef are never particularly easy to read in my opinion so I'm actually happy to keep it as is. Do feel free to push a different version if you find one that feels easier to understand though.

da-woods · 2026-09-30T08:34:45Z

Since there are no plans to use ASTROPY_OPAQUE_OBJECTS yet, I think it's best to omit it for now.

Just to clarify the intent of this: it's so you can force this module to be compiled with opaque objects to test it when developing it. So the intention is that it never gets used in a release (which will ultimately be governed by Py_TARGET_ABI3T).

Completely happy to remove it though.

neutrinoceros · 2026-09-30T08:50:56Z

it's so you can force this module to be compiled with opaque objects to test it when developing it.

I understand, and I actually like the idea, though we'd need to document it in some way and I'm not sure it's worth the effort yet as it might get no use at all if I'm the only one working on this and I forget about it (which is pretty likely). I might re-introduce it later if I change my mind though.

github-actions Bot added the utils label Sep 29, 2026

da-woods commented Sep 29, 2026

View reviewed changes

pllim added this to the v8.1.0 milestone Sep 29, 2026

pllim added build Build all wheels Run all the wheel builds rather than just a selection Limited API Python 3.15 no-changelog-entry-needed labels Sep 29, 2026

pllim requested a review from neutrinoceros September 29, 2026 18:26

chore: pre-commit fixes

fc1eba2

neutrinoceros reviewed Sep 30, 2026

View reviewed changes

neutrinoceros mentioned this pull request Sep 30, 2026

RFC: (optionally) treat PyObject as an opaque struct using heap types for PEP 803 compliance #19479

Open

Remove ASTROPY_OPAQUE_OBJECTS and express basicsize more cleanly

9a9903d

neutrinoceros approved these changes Sep 30, 2026

View reviewed changes

neutrinoceros merged commit c330471 into astropy:main Sep 30, 2026
50 of 51 checks passed

da-woods deleted the opaque-iterparse branch September 30, 2026 19:57

Conversation

da-woods commented Sep 29, 2026

Description

AI Disclosure

Merge method

Uh oh!

github-actions Bot commented Sep 29, 2026

Uh oh!

da-woods commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

da-woods Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

da-woods Sep 29, 2026 • 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!

pllim commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Sep 29, 2026

Uh oh!

da-woods commented Sep 29, 2026

Uh oh!

neutrinoceros left a comment • 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!

neutrinoceros Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

da-woods Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

da-woods commented Sep 30, 2026

Uh oh!

neutrinoceros commented Sep 30, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

da-woods commented Sep 29, 2026 •

edited

Loading

da-woods Sep 29, 2026 •

edited

Loading

pllim commented Sep 29, 2026 •

edited

Loading

neutrinoceros left a comment •

edited

Loading