[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Change by Eric Snow :


--
assignee:  -> eric.snow
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow  added the comment:

FTR, gh-9047 (for issue #34572) mentions this issue.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Change by Eric Snow :


--
pull_requests:  -11714

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Change by Eric Snow :


--
pull_requests:  -11713

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Tim Burgess]

Change by Tim Burgess :


--
pull_requests: +11713

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Tim Burgess]

Change by Tim Burgess :


--
pull_requests: +11713, 11714

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[miss-islington]

Change by miss-islington :


--
pull_requests: +7625

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Roundup Robot]

Change by Roundup Robot :


--
pull_requests: +7603

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Changes by Eric Snow :


--
pull_requests: +3596

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Changes by Eric Snow :


--
pull_requests: +3584

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:


New changeset d393c1b227f22fb9af66040b2b367c99a4d1fa9a by Eric Snow in branch 
'master':
bpo-28411: Isolate PyInterpreterState.modules (#3575)
https://github.com/python/cpython/commit/d393c1b227f22fb9af66040b2b367c99a4d1fa9a


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Changes by Eric Snow :


--
pull_requests: +3566
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:

We're reverting this (see #31404), so back to the drawing board...

--
resolution: fixed -> 
stage: resolved -> needs patch
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Changes by Eric Snow :


--
pull_requests: +3555

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:

FYI, this broke some (very) corner cases.  See issue #31404.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Changes by Eric Snow :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Changes by Eric Snow :


--
pull_requests: +1732

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Brett Cannon]

Brett Cannon added the comment:

As Nick pointed out, PyInterpreterState's fields are private so do what you 
want. :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:

Meh, there really isn't any need for _PyImport_GetModuleDict().  I'll drop it.  
Problem solved! :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

Eric Snow added the comment:

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. :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Graham Dumpleton]

Graham Dumpleton added the comment:

I always use PyImport_GetModuleDict(). So long as that isn't going away I 
should be good.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Nick Coghlan]

Nick Coghlan added the comment:

(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.

--
nosy: +grahamd

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28411] Eliminate PyInterpreterState.modules.

[Eric Snow]

New submission from Eric Snow:

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

--
components: Interpreter Core
files: drop-interp-modules.diff
keywords: patch
messages: 278445
nosy: brett.cannon, eric.snow, ncoghlan
priority: normal
severity: normal
stage: patch review
status: open
title: Eliminate PyInterpreterState.modules.
versions: Python 3.7
Added file: http://bugs.python.org/file45052/drop-interp-modules.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com