Skip to content

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

Merged
astrofrog merged 1 commit into
astropy:mainfrom
neutrinoceros:utils/rfc/limited-api-compat
Jul 4, 2025
Merged

astrofrog merged 1 commit into
astropy:mainfrom
neutrinoceros:utils/rfc/limited-api-compat

Conversation

@neutrinoceros

Copy link
Copy Markdown
Contributor

Description

Ref #18163

  • 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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

👋 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
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from 3a6e4a6 to 34f4b8d Compare June 10, 2025 10:08
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Oh I just realized that PyTuple_SET_ITEM is monkeypatched on clang, which is hiding some remaining errors on my setup.

@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from da78804 to 33fe44f Compare June 10, 2025 10:30
@neutrinoceros
neutrinoceros marked this pull request as ready for review June 10, 2025 10:34
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

The compilation error on windows seems real. Will investigate.

@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch 2 times, most recently from b8acdfd to bd66828 Compare June 10, 2025 13:16
@neutrinoceros
neutrinoceros marked this pull request as draft June 10, 2025 13:16
@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from bd66828 to 1ef1fd5 Compare June 10, 2025 13:18
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Hum, I'll take a step back and try to get building on Windows + Limited API stable first in #18252

@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from 1ef1fd5 to 8497527 Compare June 10, 2025 14:43
@neutrinoceros neutrinoceros changed the title RFC: restrict utils.xml's extension modules to CPython's Limited API RFC: restrict utils.xml's extension modules to CPython's Limited API (⏰ wait for #18252 and #18253) Jun 10, 2025
Comment thread astropy/utils/xml/src/iterparse.c Outdated
return NULL;

if (PyType_Ready(&IterParserType) < 0)
IterParserType = PyType_FromModuleAndSpec((PyObject*)&moduledef, &IterParserType_spec, NULL);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fairly sure the first argument should be the module (m) not moduledef

@neutrinoceros neutrinoceros Jun 11, 2025 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

of course ! I don't know what I was thinking (and I'm a bit worried that it seemed to work anyway ?). Thanks !

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it does much with the module except store the pointer somewhere. So it's unlikely to be an issue unless you try to use it.

@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from 8497527 to cfec34a Compare June 11, 2025 07:02
@neutrinoceros neutrinoceros changed the title RFC: restrict utils.xml's extension modules to CPython's Limited API (⏰ wait for #18252 and #18253) RFC: restrict utils.xml's extension modules to CPython's Limited API (⏰ wait for #18252) Jun 12, 2025
@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch 2 times, most recently from d7127a5 to 0103a7f Compare June 12, 2025 18:13
@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch 2 times, most recently from 2fb1ee3 to f742109 Compare June 13, 2025 08:42
@neutrinoceros neutrinoceros changed the title RFC: restrict utils.xml's extension modules to CPython's Limited API (⏰ wait for #18252) RFC: restrict utils.xml's extension modules to CPython's Limited API Jun 13, 2025
@neutrinoceros
neutrinoceros marked this pull request as ready for review June 13, 2025 08:53
@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from f742109 to 6746078 Compare July 2, 2025 18:44

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know all the details of things like Py_VISIT, but overall this looks good. Approving, though it would probably good if someone like @astrofrog could have a look too.

@astrofrog

Copy link
Copy Markdown
Member

It might be good to add a comment about Py_VISIT but not critical so approving

@neutrinoceros

neutrinoceros commented Jul 4, 2025 •

Copy link
Copy Markdown
Contributor Author

I added a call to Py_VISIT following @da-woods' comment on a sibling PR: #18231 (comment)

Specifically, the source reference is https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_dealloc.

Heap types (Py_TPFLAGS_HEAPTYPE) must visit their type with:

Py_VISIT(Py_TYPE(self));

where IterParserType is a heap-type by virtue of being (with this refactor) defined dynamically with PyType_FromModuleAndSpec

I'll summarize this as an inline comment.

@neutrinoceros
neutrinoceros force-pushed the utils/rfc/limited-api-compat branch from 6746078 to 83c8ece Compare July 4, 2025 14:38
@astrofrog
astrofrog merged commit cc87b22 into astropy:main Jul 4, 2025
@astrofrog

Copy link
Copy Markdown
Member

Thanks!

@neutrinoceros
neutrinoceros deleted the utils/rfc/limited-api-compat branch July 4, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants