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.

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

gh-117953: Track Extra Details in Global Extensions Cache #118532

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented May 3, 2024

We have only been tracking each module's PyModuleDef. However, there are some problems with that. For example, in some cases we load single-phase init extension modules from def->m_base.m_init or def->m_base.m_copy, but if multiple modules share a def then we can end up with unexpected behavior.

With this change, we track the following:

  • PyModuleDef (same as before)
  • for some modules, its init function or a copy of its __dict__, but specific to that module
  • whether it is a builtin/core module or a "dynamic" extension
  • the interpreter (ID) that owns the cached __dict__ (only if cached)

This also makes it easier to remember the module's kind (e.g. single-phase init) and if loading it previously failed, which I'm doing in another PR.

@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) May 4, 2024 21:04
@ericsnowcurrently ericsnowcurrently merged commit 291cfa4 into python:main May 4, 2024
34 checks passed
@ericsnowcurrently ericsnowcurrently deleted the extensions-cache-better-tracking branch May 5, 2024 02:43
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
…ongh-118532)

We have only been tracking each module's PyModuleDef.  However, there are some problems with that.  For example, in some cases we load single-phase init extension modules from def->m_base.m_init or def->m_base.m_copy, but if multiple modules share a def then we can end up with unexpected behavior.

With this change, we track the following:

* PyModuleDef (same as before)
* for some modules, its init function or a copy of its __dict__, but specific to that module
* whether it is a builtin/core module or a "dynamic" extension
* the interpreter (ID) that owns the cached __dict__ (only if cached)

This also makes it easier to remember the module's kind (e.g. single-phase init) and if loading it previously failed, which I'm doing separately.
@hugovk
Copy link
Member

hugovk commented May 14, 2024

We're getting python: Python/import.c:460: _get_module_index_from_def: Assertion `index > 0' failed. when trying to import ujson on 3.13, and git bisect points to this PR.

See ultrajson/ultrajson#629.

Does it sound like a CPython or ujson problem?

@ericsnowcurrently
Copy link
Member Author

Sorry for the delay. PyCon was a bit distracting. :) I'll take a look.

ericsnowcurrently added a commit that referenced this pull request May 25, 2024
The assertion was added in gh-118532 but was based on the invalid assumption that PyState_FindModule() would only be called with an already-initialized module def.  I've added a test to make sure we don't make that assumption again.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 27, 2024
…ongh-119561)

The assertion was added in pythongh-118532 but was based on the invalid assumption that PyState_FindModule() would only be called with an already-initialized module def.  I've added a test to make sure we don't make that assumption again.
(cherry picked from commit 0c5ebe1)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit that referenced this pull request May 27, 2024
…119561) (gh-119632)

The assertion was added in gh-118532 but was based on the invalid assumption that PyState_FindModule() would only be called with an already-initialized module def.  I've added a test to make sure we don't make that assumption again.

(cherry picked from commit 0c5ebe1)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…ongh-119561)

The assertion was added in pythongh-118532 but was based on the invalid assumption that PyState_FindModule() would only be called with an already-initialized module def.  I've added a test to make sure we don't make that assumption again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants