bpo-38159: Clarify documentation of PyState_AddModule by encukou · Pull Request #16101 · python/cpython · GitHub

This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.

https://bugs.python.org/issue38159

Automerge-Triggered-By: @encukou

This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value.

Just to clarify "implementing an alternative import mechanism" meant someone changing __import__ right? Otherwise, could you expand a bit more on what you meant by that? :)

Btw for context, these were added since the documentation currently specifies that PyState_FindModule needs to be paired up with PyState_AddModule to work. Since the import documentation does not offer a guarantee that your module will be attached to the interpreter state (through PyState_FindModule), it seemed like the right call to have that in the init functions.

As a matter of fact, in our system we do rely on this signal to correctly work with PyState_FindModule. Our import mechanism does not do it by default.

Maybe an alternative can be to update the import documentation that it should offer this guarantee? On the other hand, it seems like that should be an implementation detail and it's best to let the module themselves call the right APIs. Thoughts?

Changing __import__, creating module objects dynamically, things like that. I'm not 100% sure on this expected behavior myself, unfortunately. But it's currently redundant in the common case.

I'm proposing a PyState_AddModule doc update to make things clearer: python#16101

I asked over at at capi-sig.

@encukou maybe you could add "DO-NOT-MERGE"?

…imported.

Note to reviewer: See the capi-sig discussion

cc @ncoghlan

LGTM with a small comment

An alternate implementation might not necessarily call PyState_AddModule but still achieve the same end result. Perhaps just say that Python attaches the module to the interpreter state and should be accessible through PyState_FindModule. How they achieve that should be up to the implementation

All ways I can find of wording this more precisely make it significantly harder to understand.

I think it's understood that alternate implementations are free to do an equivalent of calling PyState_AddModule, instead of actually calling it. They should still implement the PyState_AddModule semantics (unless they decide to differ, of course).

Another thing that's implied here is that Python only calls this on single-phase-init modules. And another is that it's idempotent.

Should we document all that? How much hand-holding do authors of alternative import mechanisms need? (This is a real question, which you are in a good position to answer.)

But my main issue is:

All ways I can find of wording this more precisely make it significantly harder to understand.

If you can think of a better way, let me know! Otherwise I can merge this.

Added a couple of specific suggestions that I think improve precision without making it too much harder to read.

Co-Authored-By: Nick Coghlan <ncoghlan@gmail.com>

Azure Pipelines didn't start, I'll try closing & reopening.

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

GH-17026 is a backport of this pull request to the 3.8 branch.

GH-17027 is a backport of this pull request to the 3.7 branch.

This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value. (cherry picked from commit 9bc94ec) Co-authored-by: Petr Viktorin <encukou@gmail.com>

GH-17026) This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value. (cherry picked from commit 9bc94ec) Co-authored-by: Petr Viktorin <encukou@gmail.com> https://bugs.python.org/issue38159 Automerge-Triggered-By: @encukou

GH-17027) This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value. (cherry picked from commit 9bc94ec) Co-authored-by: Petr Viktorin <encukou@gmail.com> https://bugs.python.org/issue38159 Automerge-Triggered-By: @encukou

This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value.

Clarify documentation of PyState_AddModule

46cc296

This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value.

the-knights-who-say-ni added the CLA signed label Sep 13, 2019

bedevere-bot added docs Documentation in the Doc dir awaiting core review labels Sep 13, 2019

encukou added the skip news label Sep 13, 2019

encukou added a commit to eduardo-elizondo/cpython that referenced this pull request Sep 13, 2019

Don't call PyState_AddModule

b9a49aa

I'm proposing a PyState_AddModule doc update to make things clearer: python#16101

encukou added the DO-NOT-MERGE label Sep 13, 2019

Point out that Python only calls PyState_AddModule after a module is …

96a53b2

…imported.

encukou removed the DO-NOT-MERGE label Sep 24, 2019

eduardo-elizondo approved these changes Sep 24, 2019

View reviewed changes

eduardo-elizondo reviewed Sep 24, 2019

View reviewed changes

eduardo-elizondo mentioned this pull request Sep 30, 2019

bpo-38303: Make audioop extension module PEP-384 compatible #16497

Merged

ncoghlan reviewed Oct 8, 2019

View reviewed changes

Comment thread Doc/c-api/module.rst Outdated

Comment thread Doc/c-api/module.rst Outdated

encukou and others added 2 commits October 22, 2019 13:04

Apply Nick's suggestions

8dddb47

Co-Authored-By: Nick Coghlan <ncoghlan@gmail.com>

Wrap lines

904535b

encukou added needs backport to 3.7 and removed 🤖 automerge labels Oct 22, 2019

encukou closed this Nov 1, 2019

encukou reopened this Nov 1, 2019

encukou added the 🤖 automerge label Nov 1, 2019

encukou merged commit 9bc94ec into python:master Nov 1, 2019

encukou deleted the PyState_AddModule-doc branch November 1, 2019 15:47

bedevere-bot removed the awaiting core review label Nov 1, 2019

bedevere-bot removed needs backport to 3.8 labels Nov 1, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38159: Clarify documentation of PyState_AddModule (pythonGH-16101 )

09a8f08

This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value.

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38159: Clarify documentation of PyState_AddModule (pythonGH-16101 )

7243712

This was never intented to be called manually from PyInit_*. Also, clarify PyState_RemoveModule return value.


		Only effective on modules created using single-phase initialization.

		Python calls ``PyState_AddModule`` automatically after importing a module,

bpo-38159: Clarify documentation of PyState_AddModule by encukou · Pull Request #16101 · python/cpython · GitHub

Repository navigation

bpo-38159: Clarify documentation of PyState_AddModule - #16101

bpo-38159: Clarify documentation of PyState_AddModule#16101encukou merged 4 commits intopython:masterpython/cpython:masterfrom encukou:PyState_AddModule-docencukou/cpython:PyState_AddModule-docCopy head branch name to clipboard

Conversation

encukou commented Sep 13, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduardo-elizondo commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou commented Sep 13, 2019

Uh oh!

encukou commented Sep 13, 2019

Uh oh!

matrixise commented Sep 13, 2019

Uh oh!

encukou commented Sep 24, 2019

Uh oh!

encukou commented Sep 24, 2019

Uh oh!

eduardo-elizondo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

encukou commented Nov 1, 2019

Uh oh!

miss-islington commented Nov 1, 2019

Uh oh!

bedevere-bot commented Nov 1, 2019

Uh oh!

bedevere-bot commented Nov 1, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bpo-38159: Clarify documentation of PyState_AddModule#16101
encukou merged 4 commits into
python:masterfrom
encukou:PyState_AddModule-doc

encukou commented Sep 13, 2019 •
edited by miss-islington
Loading

eduardo-elizondo commented Sep 13, 2019 •
edited
Loading

eduardo-elizondo left a comment •
edited
Loading