RFC: implement PEP 793/820 multi-phase module init (wcs) - #20494

I'd probably do

since _Py_OPAQUE_PYOBJECT is no longer the recommended way of detecting opaque objects (that's Py_TARGET_ABI3T). But also there's no reason not to do this on any 3.15+ build.

I don't intend to support targeting a version of the limited API other than the one we're already building against, so, it seems to me that I could even simplify this condition to
#if PY_VERSION_HEX >= 0x030F0000
is that right ?

That should be fine, but if it were my code I'd probably still go for the longer explicit version.

It's definitely a good rule when preparing your official wheels, but I've found it convenient to be able to build stuff locally on whatever version of Python you're developing on (in general rather than for Astropy specifically).

fair enough. I encapsulated this condition into a _ASTROPY_MULTI_PHASE_MODULE_INIT pre-proc variable, and added an inline comment based on your explanation

All this is getting missed in the PEP-793 version. Which likely is important.

It probably wants to be in a Py_mod_exec slot.

Thanks !
I just pushed a tentative implementation for it.

An alternative to consider (that I think may be clearer) is to define module_exec unconditionally then define the old init function as:

PyMODINIT_FUNC
PyInit__wcs(void)
{
  PyObject* m;
  m = PyModule_Create(&moduledef);
  if (module_exec(m) != 0) {
    // Error
    Py_CLEAR(m);
  }
  return m;
}

oh, indeed that's a lot better. I just pushed it. Thanks !

The return value is guesswork: I'm assuming that 0 means "no exception" in this context, though the documentation is unclear
https://docs.python.org/3/c-api/module.html#c.Py_mod_exec

Yeah it isn't clearly documented but I'm almost certain that 0 means "good"

The newer way to import numpy is

    if (PyArray_ImportNumPyAPI() < 0) {
        return NULL;
    }

though really that should still be in init, not exec.

pushed. Though, to be clear, are you saying that PyArray_ImportNumPyAPI doesn't need to be called in the multi-phase init mode ?

It does need to be called, but more easily in PyInit see comment further down: #20494 (comment)

You don't need to create the module in the new version, rather something like,

PyMODINIT_FUNC PyInit__wcs(void)
{
    if (PyArray_ImportNumPyAPI() < 0) {
        return NULL;
    }
    return PyModuleDef_Init(&moduledef);
}

I don't think that's correct: module_exec would not run.

It does - you need something like what I have in #20183,

static PyModuleDef_Slot scaler_module_slots[] = {
    {Py_mod_exec, scaler_module_exec},
    {0, NULL},
};

and those slots get added to the module def struct.

Maybe I'm misunderstanding something but it looks to me like the docs explicitly mention we cannot do that:

https://docs.python.org/3/c-api/module.html#c.PyModuleDef.m_slots

When using legacy single-phase initialization, m_slots must be NULL

But we're not using legacy single-phase initialization any more! Or, at least, isn't that the point?

Why not just choose multi-phase? Might as well get it done and it works fine on older python (otherwise #20183 would not pass, nor would all those numpy pieces that have already been converted).

It is a fairly easy conversion. You add scaler_module_slots as @mhvk suggests, and then you change

PyMODINIT_FUNC
PyInit__wcs(void)
{
    // maybe initialize Numpy here too.
    return (PyObject*)&moduledef;
}

and you're done.

Why not just choose multi-phase?

Oh I see what happened here: I read #20494 (comment)

also there's no reason not to do this on any 3.15+ build.

and concluded that there were (implicitly) reasons to keep single-phase init for Python 3.14 and older. Is it the case, really, and do we care ? It sounds like we don't, in which case I could just simplify this by a lot.

No there wasn't any intended implicit meaning in that comment about Python 3.14 and older. I don't know any reason why you should deliberately keep single-phase init.

For clarity, I think the init needs return PyModuleDef_Init(&moduledef);

PyArray_ImportNumPyAPI definitely needs calling in both cases. I think @mhvk was proposing to put it there. I don't think it really matters where you put it provided it's called before you try to use Numpy. The documentation says it's fine to call multiple times.