GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-28411: Remove "modules" field from Py_InterpreterState. by ericsnowcurrently · Pull Request #1638 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented May 17, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

(see http://bugs.python.org/issue28411)

https://bugs.python.org/issue28411

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 17, 2017
@mention-bot

mention-bot commented May 17, 2017

Copy link
Copy Markdown

@ericsnowcurrently, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @serhiy-storchaka and @tim-one to be potential reviewers.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently

ericsnowcurrently commented May 17, 2017

Copy link
Copy Markdown
Member Author

@ncoghlan
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner requested changes May 17, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I like the overall change. I don't expect any major performance slowdown.

I just have a few suggestions on the implementation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/modsupport.h Outdated

@vstinner vstinner May 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I suggest to name _PyModule_CreateInitialized().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/import.c Outdated

@vstinner vstinner May 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm not a big fan of functions calling Py_FatalError(). I suggest to return an integer and call Py_FatalError() in the caller. Rename the function to _PyImport_IsInitialized().

Or... maybe use PyDict_GetItemWithError()? :-) I'm lost in all these "Get" functions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Consolidating direct usage of sys.modules ended up being more work than expected. However, I think it's worth it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/import.c Outdated

@vstinner vstinner May 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe add a private helper function to PyDict_GetItem(PyImport_GetModuleDict)? I ask because in another function PyMapping_GetItemString() is used. I don't know which one is correct :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ericsnowcurrently
ericsnowcurrently force-pushed the remove-modules-from-interpreter-state branch from b073e48 to 1c472f7 Compare May 20, 2017 06:42
pitrou
pitrou reviewed May 22, 2017
View reviewed changes
Comment thread Doc/c-api/import.rst Outdated

@pitrou pitrou May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What happens if the module is not found?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Seems this function returns a borrowed reference. This should be specially emphasized.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 22, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It shouldn't be returning a borrowed reference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The only code that uses it (_pickle_Unpickler_find_class_impl) is written as PyImport_GetModule returning a borrowed reference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That was an omission on my part. I should have moved the Py_DECREF(module) line down.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 4, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You should document that the function returns NULL if the module is not already imported, but raise an exception and return NULL on exception. I don't think that it's obvious from the current doc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pitrou
pitrou reviewed May 22, 2017
View reviewed changes
Comment thread Include/pystate.h Outdated

@pitrou pitrou May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think we can remove a field in the middle of the struct, as that would break the stable ABI (perhaps @zooba can confirm). Instead probably replace it with void *_unused or something.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This structure is not a part of the stable ABI.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
serhiy-storchaka self-requested a review May 22, 2017 11:19
serhiy-storchaka
serhiy-storchaka requested changes May 22, 2017
View reviewed changes
Comment thread Python/import.c Outdated

@serhiy-storchaka serhiy-storchaka May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This returns a borrowed reference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 22, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unlike PyDict_GetItem() and PyDict_GetItemString(), PyDict_GetItemWithError() does not return a borrowed reference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think so.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah, the documentation does not indicate it returns a borrowed reference. However, looking at the implementation indicates it is a borrowed reference. Thanks for catching that. I'll make a separate PR to fix the docs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/import.c Outdated

@serhiy-storchaka serhiy-storchaka May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

But this returns an owned reference.

Is there a need to support non-dicts?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 22, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, part of the point here is to support arbitrary mappings (anything you might assign to sys.modules).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/import.c Outdated

@serhiy-storchaka serhiy-storchaka May 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why not use just PyImport_GetModuleDict()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently May 23, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I was following some precedent. However, there isn't a strong argument either way. I'm fine with changing it to PyImport_GetModuleDict().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ericsnowcurrently
ericsnowcurrently force-pushed the remove-modules-from-interpreter-state branch 3 times, most recently from b7d3895 to 19388d4 Compare May 25, 2017 18:31
ericsnowcurrently added 18 commits September 4, 2017 09:40
@ericsnowcurrently
Add _PyImport_GetModuleDict().
70f6e19
@ericsnowcurrently
Import the sys module before any other imports.
10f7443
@ericsnowcurrently
Pass the modules dict to _PyImport_FindBuiltin().
5ea0e3b
@ericsnowcurrently
Replace usage of interp->modules with PyImport_GetModuleDict().
29780ec
@ericsnowcurrently
Add _PyImport_EnsureInitialized().
e6de215
@ericsnowcurrently
Add _PyModule_Create2().
29f6b7c
@ericsnowcurrently
Use _PyModule_Create2() for key builtin modules.
b77d73f
@ericsnowcurrently
Check sys.modules in _PyImport_EnsureInitialized().
8275633
@ericsnowcurrently
Drop PyInterpreterState.modules.
7c62114
@ericsnowcurrently
Pass the modules dict to _PyImport_FixupBuiltin().
428d223
@ericsnowcurrently
Pass PyInterpreterState to _PyImport_GetModuleDict().
833b00b
@ericsnowcurrently
Drop _PyImport_GetModuleDict().
e0e51d2
@ericsnowcurrently
_PyModule_Create2 -> _PyModule_CreateInitialized
88aadeb
@ericsnowcurrently
_PyImport_EnsureInitialized() -> _PyImport_IsInitialized()
79320bd
@ericsnowcurrently
Add _PyImport_GetModule*.
6a0215c
@ericsnowcurrently
Allow sys.modules to be any mapping.
dabd187
@ericsnowcurrently
Add _PyImport_SetModule*.
58d9be7
@ericsnowcurrently
Add PyImport_GetModule().
e3b0f0c
@ericsnowcurrently

ericsnowcurrently commented Sep 4, 2017

Copy link
Copy Markdown
Member Author

I didn't expect the Spanish Inquisition!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Sep 4, 2017
@bedevere-bot

bedevere-bot commented Sep 4, 2017

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

@Haypo, @serhiy-storchaka: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pitrou

pitrou commented Sep 4, 2017

Copy link
Copy Markdown
Member

Actually, I did.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
Switch the code order back.
6544fa2
vstinner
vstinner approved these changes Sep 4, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. Maybe just mention the change in https://docs.python.org/dev/whatsnew/3.7.html#porting-to-python-3-7

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Sep 4, 2017
@ericsnowcurrently
Add a whatsnew entry for porting.
8e86c53
@ericsnowcurrently
ericsnowcurrently merged commit 86b7afd into python:master Sep 4, 2017
@ericsnowcurrently
ericsnowcurrently deleted the remove-modules-from-interpreter-state branch September 4, 2017 23:54
jimmylai pushed a commit to jimmylai/cpython that referenced this pull request Sep 5, 2017
Jimmy Lai
Merge branch 'master' of https://github.com/python/cpython into asyncio
51a4872 
* 'master' of https://github.com/python/cpython: (32 commits)
  Conceptually, roots is a set.  Also searching it as a set is a tiny bit faster (python#3338)
  bpo-31343: Include sys/sysmacros.h (python#3318)
  bpo-30102: Call OPENSSL_add_all_algorithms_noconf (python#3112)
  Prevent a few make suspicious warnings. (python#3341)
  Include additional changes to support blurbified NEWS (python#3340)
  Simplify NEWS entry to prevent suspicious warnings. (python#3339)
  bpo-31347: _PyObject_FastCall_Prepend: do not call memcpy if args might not be null (python#3329)
  Revert "bpo-17852: Maintain a list of BufferedWriter objects.  Flush them on exit. (python#1908)" (python#3337)
  bpo-17852: Maintain a list of BufferedWriter objects.  Flush them on exit. (python#1908)
  Fix terminology in comment and add more design rationale. (python#3335)
  Add comment to explain the implications of not sorting keywords (python#3331)
  bpo-31170: Update libexpat from 2.2.3 to 2.2.4 (python#3315)
  bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638)
  random_triangular:  sqrt() is more accurate than **0.5 (python#3317)
  Travis: use ccache (python#3307)
  remove IRIX support (closes bpo-31341) (python#3310)
  Code clean-up.  Remove unnecessary pre-increment before the loop starts. (python#3312)
  Regen Moduls/clinic/_ssl.c.h (pythonGH-3320)
  bpo-30502: Fix handling of long oids in ssl. (python#2909)
  Cache externals, depending on changes to PCbuild (python#3308)
  ...
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
@ericsnowcurrently @GadgetSteve
bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638
fa0ff58 
)

sys.modules is the one true source.
@ericsnowcurrently
ericsnowcurrently restored the remove-modules-from-interpreter-state branch September 14, 2017 06:04
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Sep 14, 2017
@ericsnowcurrently
Revert "bpo-28411: Remove "modules" field from Py_InterpreterState. (p…
9e05a48 
…ython#1638)"

This reverts commit 86b7afd.
@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 14, 2017
bpo-31404: Revert "remove modules from Py_InterpreterState (#1638)" #3565
Merged
ericsnowcurrently added a commit that referenced this pull request Sep 14, 2017
@ericsnowcurrently
bpo-31404: Revert "remove modules from Py_InterpreterState (#1638)" (#…
93c92f7 
…3565)

PR #1638, for bpo-28411, causes problems in some (very) edge cases. Until that gets sorted out, we're reverting the merge. PR #3506, a fix on top of #1638, is also getting reverted.
@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 14, 2017
bpo-28411: Isolate PyInterpreterState.modules #3575
Merged
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Sep 14, 2017
@ericsnowcurrently
bpo-28411: Isolate PyInterpreterState.modules (python#3575)
d393c1b 
A bunch of code currently uses PyInterpreterState.modules directly instead of PyImport_GetModuleDict(). This complicates efforts to make changes relative to sys.modules. This patch switches to using PyImport_GetModuleDict() uniformly. Also, a number of related uses of sys.modules are updated for uniformity for the same reason.

Note that this code was already reviewed and merged as part of python#1638. I reverted that and am now splitting it up into more focused parts.
This was referenced Sep 14, 2017
bpo-28411: Support other mappings in PyInterpreterState.modules. #3593
Merged
bpo-28411: Remove PyInterpreterState.modules. #3606
Closed
@Mariatta Mariatta removed the awaiting merge label Oct 8, 2017
@hroncok

hroncok commented May 11, 2018

Copy link
Copy Markdown
Contributor

Maybe just mention the change in https://docs.python.org/dev/whatsnew/3.7.html#porting-to-python-3-7

What this ever put there? I've juts been bit by the changed API of _PyImport_FixupBuiltin (in gdb).

https://bugzilla.redhat.com/show_bug.cgi?id=1577396
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented May 12, 2018

Copy link
Copy Markdown
Member

@hroncok: Would you mind to open a new issue at bugs.python.org? This PR has been closed since last September, so people will unlikely notice your comment :-(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hroncok

hroncok commented May 12, 2018

Copy link
Copy Markdown
Contributor

Sure, i was just checking. Here it is: https://bugs.python.org/issue33470
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 10, 2022
[subinterpreters] Eliminate PyInterpreterState.modules. #72597
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka requested changes
@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@ericsnowcurrently @mention-bot @bedevere-bot @pitrou @hroncok @vstinner @serhiy-storchaka @Mariatta @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.