classification
Title: Eliminate PyInterpreterState.modules.
Type: Stage: patch review
Components: Interpreter Core Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, eric.snow, grahamd, ncoghlan
Priority: normal Keywords: patch

Created on 2016-10-10 22:33 by eric.snow, last changed 2017-09-15 22:51 by eric.snow.

Files
File name Uploaded Description Edit
drop-interp-modules.diff eric.snow, 2016-10-10 22:33 review
Pull Requests
URL Status Linked Edit
PR 1638 merged eric.snow, 2017-05-17 21:16
PR 3565 merged eric.snow, 2017-09-14 06:23
PR 3575 merged eric.snow, 2017-09-14 15:25
PR 3593 merged eric.snow, 2017-09-14 23:32
PR 3606 open eric.snow, 2017-09-15 22:51
Messages (14)
msg278445 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-10-10 22:33
tl;dr PyInterpreterState does not need a "modules" field.  Attached is a patch that removes it.

During interpreter startup [1] the sys module is imported using the same C API [2] as any other builtin module.  That API only requires one bit of import state, sys.modules.  Obviously, while the sys module is being imported, sys.modules does not exist yet.  To accommodate this situation, PyInterpreterState has a "modules" field.  The problem is that PyInterpreterState.modules remains significant in the C-API long after it is actually needed during startup.  This creates the potential for sys.modules and PyInterpreterState.modules to be out of sync.

Currently I'm working on an encapsulation of the import state.  PyInterpreterState.modules complicates the scene enough that I'd like to see it go away.  The attached patch does so by adding private C-API functions that take a modules arg, rather than getting it from the interpreter state.  These are used during interpreter startup, rendering PyInterpreterState.modules unnecessary and allowing sys.modules to become the single source of truth.

If this patch lands, we can close issue12633.


[1] see _Py_InitializeEx_Private() and Py_NewInterpreter() in Python/pylifecycle.c
[2] see PyModule_Create2() and _PyImport_FindBuiltin() in Python/import.c
msg278456 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-10-11 04:06
(added Graham Dumpleton to the nosy list to ask if this change may impact mod_wsgi for 3.7)

+1 on the general idea, but given that the current field is a public part of the interpreter state, the replacement access API should really be public as well - we can't be sure folks will always be going through the "PyImport_GetModuleDict()" API.

If you replace the addition of the `_PyImport_GetModuleDict` API with a public `PyInterpreterState_GetModuleDict` API, I think that will cover it - the new calls would just be "PyInterpreterState_GetModuleCache(tstate->interp)" rather than `_PyImport_GetModuleDict(tstate)`

Folks accessing this field directly can then define their own shim function if PyInterpreterState_GetModuleCache isn't defined.

(The rationale for the GetModuleDict -> GetModuleCache change is that "ModuleDict" is ambiguous - every module has a dict. For PyImport_* we're stuck with it, but the "PyImport" prefix at least gives a hint that the reference might be to the sys.modules cache. That affordance doesn't exist for the "PyInterpeterState" prefix.
msg278457 - (view) Author: Graham Dumpleton (grahamd) Date: 2016-10-11 04:34
I always use PyImport_GetModuleDict(). So long as that isn't going away I should be good.
msg278507 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-10-11 22:44
What's the benefit to adding PyInterpreterState_GetModuleCache()?  TBH, it should only be needed in this short period during startup when the import system hasn't been bootstrapped yet.  After that code can import sys and access sys.modules from there.  (For that matter, PyImport_GetModuleDict() doesn't buy all that much either...)  I think all this would be clearer in a world with PEP 432.  :)

FWIW, I'm inclined to encourage new APIs where it makes sense that take an explicit interp arg.  I just don't think a new public API is warranted here.  If we didn't already have PyImport_GetModuleDict(), I'd probably just move the code to Python/pylifecycle.c, inlined or in a small static function.

And +1 to ModuleCache vs. ModuleDict. :)
msg278508 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-10-11 22:57
Hmm, actually _PyImport_GetModuleDict() isn't needed to solve the startup issue.  It's still rather internally focused but the same could be said for PyImport_GetModuleDict().  I guess I'm still not sold on adding a new public API function for what amounts to a rename of PyImport_GetModuleDict().

Furthermore, wouldn't it make more sense to keep it in the PyImport_* namespace?  Perhaps we could set the precedent that explicitly-arg'ed functions should be in the PyInterpreterState_* (or PyInterpreter_*) namespace?
msg278509 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-10-11 23:05
Meh, there really isn't any need for _PyImport_GetModuleDict().  I'll drop it.  Problem solved! :)
msg278514 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-10-12 03:39
I just checked the docs, and it turns out I'm wrong about this being a previously public API: "There are no public members in this structure."

From https://docs.python.org/3/c-api/init.html#c.PyInterpreterState

That means the only externally supported API that needs to be preserved is PyImport_GetModuleDict() to get the current thread's module cache, and your original patch already did that.
msg278664 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-10-14 20:04
As Nick pointed out, PyInterpreterState's fields are private so do what you want. :)
msg301286 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-04 23:54
New changeset 86b7afdfeee77993fe896a2aa13b3f4f95973f16 by Eric Snow in branch 'master':
bpo-28411: Remove "modules" field from Py_InterpreterState. (#1638)
https://github.com/python/cpython/commit/86b7afdfeee77993fe896a2aa13b3f4f95973f16
msg302127 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-13 22:47
FYI, this broke some (very) corner cases.  See issue #31404.
msg302141 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-14 06:25
We're reverting this (see #31404), so back to the drawing board...
msg302148 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-14 06:46
New changeset 93c92f7d1dbb6e7e472f1d0444c6968858113de2 by Eric Snow in branch 'master':
bpo-31404: Revert "remove modules from Py_InterpreterState (#1638)" (#3565)
https://github.com/python/cpython/commit/93c92f7d1dbb6e7e472f1d0444c6968858113de2
msg302193 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-14 18:18
New changeset d393c1b227f22fb9af66040b2b367c99a4d1fa9a by Eric Snow in branch 'master':
bpo-28411: Isolate PyInterpreterState.modules (#3575)
https://github.com/python/cpython/commit/d393c1b227f22fb9af66040b2b367c99a4d1fa9a
msg302303 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-15 22:35
New changeset 3f9eee6eb4b25fe1926eaa5f00e02344b126f54d by Eric Snow in branch 'master':
bpo-28411: Support other mappings in PyInterpreterState.modules. (#3593)
https://github.com/python/cpython/commit/3f9eee6eb4b25fe1926eaa5f00e02344b126f54d
History
Date User Action Args
2017-09-15 22:51:14eric.snowsetpull_requests: + pull_request3596
2017-09-15 22:35:22eric.snowsetmessages: + msg302303
2017-09-14 23:32:45eric.snowsetpull_requests: + pull_request3584
2017-09-14 18:18:14eric.snowsetmessages: + msg302193
2017-09-14 15:25:03eric.snowsetstage: needs patch -> patch review
pull_requests: + pull_request3566
2017-09-14 06:46:06eric.snowsetmessages: + msg302148
2017-09-14 06:25:14eric.snowsetstatus: closed -> open
resolution: fixed ->
messages: + msg302141

stage: resolved -> needs patch
2017-09-14 06:23:47eric.snowsetpull_requests: + pull_request3555
2017-09-13 22:47:12eric.snowsetmessages: + msg302127
2017-09-04 23:55:13eric.snowsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-09-04 23:54:12eric.snowsetmessages: + msg301286
2017-09-04 18:27:53eric.snowlinkissue12633 superseder
2017-05-17 21:16:44eric.snowsetpull_requests: + pull_request1732
2016-10-14 20:04:25brett.cannonsetmessages: + msg278664
2016-10-12 03:39:44ncoghlansetmessages: + msg278514
2016-10-11 23:05:25eric.snowsetmessages: + msg278509
2016-10-11 22:57:23eric.snowsetmessages: + msg278508
2016-10-11 22:44:47eric.snowsetmessages: + msg278507
2016-10-11 04:34:18grahamdsetmessages: + msg278457
2016-10-11 04:06:06ncoghlansetnosy: + grahamd
messages: + msg278456
2016-10-10 22:33:03eric.snowcreate