FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-28411: Remove PyInterpreterState.modules. by ericsnowcurrently · Pull Request #3606 · python/cpython · GitHub

/ cpython Public

bpo-28411: Remove PyInterpreterState.modules. - #3606

Closed
ericsnowcurrently wants to merge 6 commits into
python:mainfrom
ericsnowcurrently:remove-modules-from-interpreter-state
ConversationCommits (6)ChecksFiles changed
Closed

bpo-28411: Remove PyInterpreterState.modules.#3606
ericsnowcurrently wants to merge 6 commits into
python:mainfrom
ericsnowcurrently:remove-modules-from-interpreter-state

Conversation

ericsnowcurrently commented Sep 15, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

The status quo for sys.modules is broken. While importlib (and a few other places) use sys.modules directly, the C-API (including PyImport_Import(), etc.) use PyInterpreterState.modules directly. Since assigning to sys.modules does not update PyInterpreterState.modules, the two can get out of sync. The workaround, which we use in the test suite, is complicated. This patch resolves the issue by getting rid of PyInterpreterState.modules and making sys.modules authoritative in the C-API.

Note that this code was landed earlier (PR #1638), but reverted due to problems when a bogus value (e.g. []) is assigned to sys.modules (see bpo-31404), which is very much a corner case. I've broken that original patch up and already landed the majority. The remaining piece here is to actually drop PyInterpreterState.modules. This PR should not be merged until the problem described in bpo-31404 is resolved.

https://bugs.python.org/issue28411

ericsnowcurrently requested a review from a team September 15, 2017 22:51
vstinner changed the title bpo-28411: Remove PyInterpreterState.modules. [WIP] bpo-28411: Remove PyInterpreterState.modules. Sep 15, 2017

vstinner commented Sep 15, 2017

Copy link
Copy Markdown
Member

I added [WIP] to your PR title for this reason: "This PR should not be merged until the problem described in bpo-31404 is resolved."

ericsnowcurrently commented Sep 15, 2017

Copy link
Copy Markdown
Member Author

Thanks.

brettcannon commented Mar 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@ericsnowcurrently It looks like bpo-31404 was resolved, so can the WIP label be removed?

ericsnowcurrently mentioned this pull request May 23, 2018
Closed
ericsnowcurrently changed the title [WIP] bpo-28411: Remove PyInterpreterState.modules. bpo-28411: Remove PyInterpreterState.modules. Mar 1, 2019

ericsnowcurrently commented Mar 1, 2019

Copy link
Copy Markdown
Member Author

FYI, I'm going to revisit this PR in the near future.

brettcannon removed the request for review from a team April 17, 2019 22:44

brettcannon commented Apr 17, 2019

Copy link
Copy Markdown
Member

Removing the import team from reviewing as this is still a WIP. We can be added back when it's ready to go.

matrixise commented Sep 17, 2019

Copy link
Copy Markdown
Member

Hi @ericsnowcurrently

Do you continue to work on this PR?

Thanks

alimcmaster1 commented Apr 11, 2020

Copy link
Copy Markdown

@matrixise -> think we should be good to close. No response from @ericsnowcurrently

ericsnowcurrently commented Apr 11, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member Author

Does the problem described in bpo-31404 still exist with this branch applied?

alimcmaster1 commented Apr 11, 2020

Copy link
Copy Markdown

Unsure - I couldn’t repro on Linux and don’t have a windows machine handy to check.

We can still keep the issue open - but might make sense to close this PR if you are no longer are interested in working on this. Unless you/others disagree?

github-actions Bot commented Feb 22, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 22, 2022
ericsnowcurrently mentioned this pull request Apr 10, 2022
Open
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 29, 2022

github-actions Bot commented Aug 31, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 31, 2022

ericsnowcurrently commented Aug 31, 2022

Copy link
Copy Markdown
Member Author

I'm fine with closing this. I'll probably circle back to it at some point.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

awaiting merge DO-NOT-MERGE stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL