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

encukou · 2019-09-13T13:26:14Z

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.

eduardo-elizondo · 2019-09-13T15:36:13Z

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?

encukou · 2019-09-13T15:46:58Z

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

encukou · 2019-09-13T15:59:20Z

I asked over at at capi-sig.

matrixise · 2019-09-13T16:46:51Z

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

…imported.

encukou · 2019-09-24T14:36:49Z

Note to reviewer: See the capi-sig discussion

encukou · 2019-09-24T14:37:07Z

cc @ncoghlan

eduardo-elizondo

LGTM with a small comment

eduardo-elizondo · 2019-09-24T16:50:09Z


 Only effective on modules created using single-phase initialization.

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


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.

ncoghlan

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>

encukou · 2019-11-01T14:44:41Z

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

miss-islington · 2019-11-01T15:47:08Z

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

bedevere-bot · 2019-11-01T15:47:43Z

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

bedevere-bot · 2019-11-01T15:47:50Z

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>

@encukou

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

@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 threadDoc/c-api/module.rst Outdated
Comment threadDoc/c-api/module.rst Outdated

encukouand 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:masterNov 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,

Uh oh!

Uh oh!

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-elizondoSep 24, 2019

Choose a reason for hiding this comment

Uh oh!

encukouSep 25, 2019

Choose a reason for hiding this comment

Uh oh!

encukouOct 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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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