Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-1635741: port _ctypes extension module to multiphase initialization #30525

Closed
wants to merge 2 commits into from

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jan 11, 2022

@kumaraditya303 kumaraditya303 marked this pull request as ready for review January 11, 2022 04:22
@@ -5847,20 +5840,25 @@ _ctypes_mod_exec(PyObject *mod)
return 0;
}

static struct PyModuleDef_Slot _ctypes_slots[] = {
{Py_mod_exec, _ctypes_mod_exec},
Copy link
Member

Choose a reason for hiding this comment

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

I am just curious about those codes in _ctypes_mod_exec, do we need judge the attr manually?

_unpickle = PyObject_GetAttrString(mod, "_unpickle");
    if (_unpickle == NULL) {
        return -1;
    }

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

You should not convert an extension to the multi-phase init API until all static types defined by the extension are defined as heap types.

See _ctypes_add_types(): there are many types defined as static types. Just one example: PyCArg_Type.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@erlend-aasland
Copy link
Contributor

You should not convert an extension to the multi-phase init API until all static types defined by the extension are defined as heap types.

True; but sadly, converting static types to heap types has been put on hold by the SC. The good news is that work is in progress to resolve the "deadlock", so we can continue applying PEP 489 and PEP 630 to the standard library 😃

See SC communications as of February 2021:

"The Steering Council discussed the ongoing work on porting types in the standard library to heap-types and the subinterpreter-related changes. It was decided that through Pablo, the Steering Council will ask the core developers driving those changes to create an informational PEP and not to make any more changes in this area after beta 1, as per our general policy."

@vstinner
Copy link
Member

ask the core developers driving those changes to create an informational PEP

Eric is working on a PEP: python/peps#2212

Not sure if it fulfill SC requirements to convert more static types to heap types.

@kumaraditya303
Copy link
Contributor Author

IIUC converting static types to heap types is a different issue https://bugs.python.org/issue40077 which is not I am tackling in this PR, also itertools module also creates many static types but still uses multiphase initialization

return PyModuleDef_Init(&itertoolsmodule);
so how is _ctypes different ?

@erlend-aasland
Copy link
Contributor

It is subinterpreter related. Also, we first convert global state to module state, then apply multi-phase init, as Victor already pointed out.

Some modules have been converted to multi-phase init prematurely. That does not alter the status quo, or set a precedent.

@corona10
Copy link
Member

FYI, Please read https://github.com/vstinner/talks/blob/main/2021-PyconUS/subinterpreters.pdf how we've converted modules :)

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Same as Victor

@erlend-aasland
Copy link
Contributor

FYI: python/steering-council#99

@kumaraditya303 kumaraditya303 deleted the ctypes branch January 28, 2022 11:36
@erlend-aasland
Copy link
Contributor

FYI, PEP-687 was just accepted.

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.

7 participants