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.

Sign up for GitHub

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

Jump to bottom

gh-123880: Allow recursive import of single-phase-init modules #123950

Merged
merged 13 commits into from
Sep 20, 2024
Merged

gh-123880: Allow recursive import of single-phase-init modules #123950

merged 13 commits into from
Sep 20, 2024

Conversation

encukou
Copy link
Member

@encukou encukou commented Sep 11, 2024 •
edited by bedevere-app bot
Loading

Single-phase modules can imported recursively, if they take care of returning the partially initialized module in such cases.

Concretely, mypyc emits a global static PyObject* module = NULL; set right after PyModule_Create and checked+returned right before it. This is of course utterly incompatible with multiple interpreters and with Py_Finalize/Py_Initialize “power cycles”, but, it works for their use cases.

Unless I'm missing something, the assert that mypyc is running into can be removed, and the cache value reused. Deallocation of the cache values is done in a few places, but none of them can reasonably happen witin a (recursive) import.
(Here I beg people to not try destroying pre-existing interpreters from within a recursive single-phase PyInit_* function -- please! single-phase init is a bowl of backcompat spaghetti; even after Eric's untangling, it can only take so much. See several XXX notes elsewhere in import.c.)

I'm sending this draft early, I still have to check more of the code and write a regression test.

@bedevere-app bedevere-app bot mentioned this pull request Sep 11, 2024
Closed
@encukou encukou marked this pull request as ready for review September 12, 2024 14:16
hauntsaninja
hauntsaninja reviewed Sep 12, 2024
View reviewed changes
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the fix, confirmed it on the end-to-end mypyc repro as well. I was wondering if we could get away with just removing the assert like this :-) I noticed two trivial typos in comments

Modules/_testsinglephase.c Outdated Show resolved Hide resolved
Python/import.c Outdated Show resolved Hide resolved
@hauntsaninja hauntsaninja added the needs backport to 3.13 bugs and security fixes label Sep 13, 2024
brettcannon
brettcannon reviewed Sep 13, 2024
View reviewed changes
Python/import.c Outdated Show resolved Hide resolved
Python/import.c Outdated Show resolved Hide resolved
Python/import.c Outdated Show resolved Hide resolved
Lib/test/test_import/__init__.py Outdated
loader = importlib.machinery.ExtensionFileLoader(name, filename)
spec = importlib.util.spec_from_file_location(name, filename,
loader=loader)
mod = importlib._bootstrap._load(spec)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if I love using this API for the test since it's somewhat of a hack to start, but I also understand not wanting to paste in 2 more lines of boilerplate to get the module created and set sys.modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was done elsewhere in the file too; I merged the usage into a common helper. If it breaks, there's now just one place to change.

(We never got around to adding proper public API for multiple modules in one shared library, but then, it's really only useful for testing import mechanism...)

In this case I slightly prefer not to touch sys.modules here -- the “inner” import should do that.

Lib/test/test_import/__init__.py Outdated Show resolved Hide resolved
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 16, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 5fc359b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 16, 2024
@encukou
Copy link
Member Author

encukou commented Sep 17, 2024

!buildbot iOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit c67517d 🤖

The command will test the builders whose names match following regular expression: iOS

The builders matched are:

  • iOS ARM64 Simulator PR

@encukou
Copy link
Member Author

encukou commented Sep 18, 2024

!buildbot iOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 72cd269 🤖

The command will test the builders whose names match following regular expression: iOS

The builders matched are:

  • iOS ARM64 Simulator PR

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 19, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 72cd269 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 19, 2024
@encukou encukou merged commit aee219f into python:main Sep 20, 2024
109 of 115 checks passed
@miss-islington-app
Copy link

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@encukou encukou deleted the spi-cache branch September 20, 2024 08:27
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 20, 2024
@encukou @hauntsaninja
@brettcannon @miss-islington
pythongh-123880: Allow recursive import of single-phase-init modules (p…
58ba5ba 
…ythonGH-123950)

(cherry picked from commit aee219f)

Co-authored-by: Petr Viktorin <[email protected]>
Co-authored-by: Shantanu <[email protected]>
Co-authored-by: Brett Cannon <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 20, 2024

GH-124273 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 20, 2024
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@encukou @hauntsaninja
@brettcannon @savannahostrowski
pythongh-123880: Allow recursive import of single-phase-init modules (p…
e9483e6 
…ythonGH-123950)


Co-authored-by: Shantanu <[email protected]>
Co-authored-by: Brett Cannon <[email protected]>
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@encukou @hauntsaninja
@brettcannon @savannahostrowski
pythongh-123880: Allow recursive import of single-phase-init modules (p…
25e277e 
…ythonGH-123950)


Co-authored-by: Shantanu <[email protected]>
Co-authored-by: Brett Cannon <[email protected]>
Yhg1s pushed a commit that referenced this pull request Sep 23, 2024
@miss-islington @encukou
@hauntsaninja @brettcannon
[3.13] gh-123880: Allow recursive import of single-phase-init modules (…
de8dc92 
…GH-123950) (#124273)

gh-123880: Allow recursive import of single-phase-init modules (GH-123950)

(cherry picked from commit aee219f)

Co-authored-by: Petr Viktorin <[email protected]>
Co-authored-by: Shantanu <[email protected]>
Co-authored-by: Brett Cannon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hauntsaninja hauntsaninja hauntsaninja left review comments

@brettcannon brettcannon brettcannon left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner

@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@encukou @bedevere-bot @brettcannon @hauntsaninja