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

[subinterpreters] Eliminate PyInterpreterState.modules. #72597

Open
ericsnowcurrently opened this issue Oct 10, 2016 · 22 comments
Open

[subinterpreters] Eliminate PyInterpreterState.modules. #72597

ericsnowcurrently opened this issue Oct 10, 2016 · 22 comments
Labels
3.13 bugs and security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@ericsnowcurrently
Copy link
Member

BPO 28411
Nosy @brettcannon, @ncoghlan, @ericsnowcurrently
PRs
  • bpo-28411: Remove "modules" field from Py_InterpreterState. #1638
  • bpo-31404: Revert "remove modules from Py_InterpreterState (#1638)" #3565
  • bpo-28411: Isolate PyInterpreterState.modules #3575
  • bpo-28411: Support other mappings in PyInterpreterState.modules. #3593
  • bpo-28411: Remove PyInterpreterState.modules. #3606
  • bpo-28411: Fix redundant declaration of _PyImport_AddModuleObject #7992
  • [3.7] bpo-28411: Fix redundant declaration of _PyImport_AddModuleObject (GH-7992) #8017
  • Files
  • drop-interp-modules.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ericsnowcurrently'
    closed_at = None
    created_at = <Date 2016-10-10.22:33:03.757>
    labels = ['expert-subinterpreters', 'type-bug', '3.7']
    title = '[subinterpreters] Eliminate PyInterpreterState.modules.'
    updated_at = <Date 2020-05-15.01:02:08.283>
    user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2020-05-15.01:02:08.283>
    actor = 'vstinner'
    assignee = 'eric.snow'
    closed = False
    closed_date = None
    closer = None
    components = ['Subinterpreters']
    creation = <Date 2016-10-10.22:33:03.757>
    creator = 'eric.snow'
    dependencies = []
    files = ['45052']
    hgrepos = []
    issue_num = 28411
    keywords = ['patch']
    message_count = 15.0
    messages = ['278445', '278456', '278457', '278507', '278508', '278509', '278514', '278664', '301286', '302127', '302141', '302148', '302193', '302303', '335026']
    nosy_count = 4.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'grahamd', 'eric.snow']
    pr_nums = ['1638', '3565', '3575', '3593', '3606', '7992', '8017']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28411'
    versions = ['Python 3.7']

    @ericsnowcurrently
    Copy link
    Member Author

    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 bpo-12633.

    [1] see _Py_InitializeEx_Private() and Py_NewInterpreter() in Python/pylifecycle.c
    [2] see PyModule_Create2() and _PyImport_FindBuiltin() in Python/import.c

    @ericsnowcurrently ericsnowcurrently added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.7 (EOL) end of life labels Oct 10, 2016
    @ncoghlan
    Copy link
    Contributor

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

    @grahamd
    Copy link
    Mannequin

    grahamd mannequin commented Oct 11, 2016

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

    @ericsnowcurrently
    Copy link
    Member Author

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

    @ericsnowcurrently
    Copy link
    Member Author

    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?

    @ericsnowcurrently
    Copy link
    Member Author

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

    @ncoghlan
    Copy link
    Contributor

    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.

    @brettcannon
    Copy link
    Member

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

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 86b7afd by Eric Snow in branch 'master':
    bpo-28411: Remove "modules" field from Py_InterpreterState. (bpo-1638)
    86b7afd

    @ericsnowcurrently
    Copy link
    Member Author

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

    @ericsnowcurrently
    Copy link
    Member Author

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

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 93c92f7 by Eric Snow in branch 'master':
    bpo-31404: Revert "remove modules from Py_InterpreterState (bpo-1638)" (bpo-3565)
    93c92f7

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset d393c1b by Eric Snow in branch 'master':
    bpo-28411: Isolate PyInterpreterState.modules (bpo-3575)
    d393c1b

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 3f9eee6 by Eric Snow in branch 'master':
    bpo-28411: Support other mappings in PyInterpreterState.modules. (bpo-3593)
    3f9eee6

    @ericsnowcurrently
    Copy link
    Member Author

    FTR, #53293 (for issue bpo-34572) mentions this issue.

    @ericsnowcurrently ericsnowcurrently self-assigned this Feb 7, 2019
    @ericsnowcurrently ericsnowcurrently added the type-bug An unexpected behavior, bug, or error label Feb 7, 2019
    @vstinner vstinner added topic-subinterpreters and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) labels May 15, 2020
    @vstinner vstinner changed the title Eliminate PyInterpreterState.modules. [subinterpreters] Eliminate PyInterpreterState.modules. May 15, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland
    Copy link
    Contributor

    @ericsnowcurrently: is this issue and the linked PR still something you are working on, or has it been superseded by other more recent issues?

    @ericsnowcurrently
    Copy link
    Member Author

    It is still alive, just way back on my TODO list. 🙂

    @kumaraditya303
    Copy link
    Contributor

    @ericsnowcurrently I'll pick this one up.

    @kumaraditya303 kumaraditya303 added 3.12 bugs and security fixes and removed 3.7 (EOL) end of life labels Jul 26, 2022
    @kumaraditya303
    Copy link
    Contributor

    I have a WIP branch 1, it passes all the tests but has refleak. I'll investigate more once I get some time till then you can try it.

    Footnotes

    1. https://github.com/kumaraditya303/cpython/pull/6

    @ericsnowcurrently
    Copy link
    Member Author

    Be cautious. There are some potentially tricky bits to this (as evidenced by #72597 (comment)).

    FWIW, there is no urgency with this at the moment.

    @kumaraditya303
    Copy link
    Contributor

    kumaraditya303 commented Jul 26, 2022

    Be cautious. There are some potentially tricky bits to this (as evidenced by #72597 (comment)).

    FWIW, there is no urgency with this at the moment.

    Okay, it was on low priority anyways but we now have a updated branch against main to work with (if we want to).

    My top priority is Per Interpreter GIL for 3.12~3.13 and it's precursors which is more urgent and interesting than this ;)

    @ericsnowcurrently
    Copy link
    Member Author

    I've closed my PR (gh-3606) but will probably get back to it some time down the road.

    @kumaraditya303 kumaraditya303 removed their assignment Oct 22, 2022
    @arhadthedev arhadthedev added 3.13 bugs and security fixes and removed 3.12 bugs and security fixes labels May 15, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.13 bugs and security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error
    Projects
    Status: Todo
    Development

    No branches or pull requests

    7 participants