fix(integrations): don't shortcut a module that is still initializing - #793
Conversation
The sys.modules fast path in _import_optional_module returned any module present in sys.modules. The loader inserts a module there *before* running its body, so when setup() races a provider import on another thread the shortcut hands back a half-built module: its patch targets do not exist yet, applies() concludes they are absent, and the patcher is silently skipped. importlib.import_module would have blocked on the per-module lock instead. Guard the shortcut with the spec's _initializing flag, which is the same predicate CPython uses to decide the lock is unnecessary -- 3.11+ has this exact fast path inside _find_and_load. 3.10 locks unconditionally, which is why the shortcut is worth keeping there at all. Reported by Codex review on #783. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The sys.modules fast path missed the case that was actually crashing. CI's
traceback names _ModuleLock('mistralai.chat') -- a module that does not exist
under mistralai 2.x. An absent module is never in sys.modules, so the
shortcut never applies to it and every call falls through to import_module,
takes CPython 3.10's unconditional module lock, and raises.
Patchers carry target modules for layouts only some provider versions ship,
so misses are normal and permanent. Measured over two MistralIntegration
.setup() calls: 96 failing imports, 12 of them for mistralai.chat alone,
across seven absent modules (chat, embeddings, fim, agents, conversations,
ocr, transcriptions). Caching the failure takes that to 7 and 1.
The sys.modules lookup still runs first, so a module that genuinely appears
later is found -- the cache suppresses the retry, it does not shadow a real
module.
This still does not reproduce locally (test_mistral(latest) is 5/5 green on
3.10 here), so it is not proven to be the cure. It does delete the exact call
path in the traceback, which the previous attempt did not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f290f75ed2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| # retry them ~96 times between them. Each retry takes the import lock, | ||
| # which is what CPython 3.10 trips over. The sys.modules check above | ||
| # runs first, so a module that genuinely shows up later is still found. | ||
| _UNIMPORTABLE_MODULES.add(name) |
There was a problem hiding this comment.
When the requested module exists but its first import raises ImportError from inside the module body—for example, because of a temporarily unavailable transitive dependency or import-order cycle—this records the target as permanently unavailable. Python removes failed imports from sys.modules, so even after the underlying condition is resolved, every later setup() returns at the cache check without retrying, silently disabling instrumentation for the rest of the process. Cache only a ModuleNotFoundError that identifies the requested target as missing rather than every ImportError.
Useful? React with 👍 / 👎.
Third attempt at the ubuntu/3.10 mistral failure, and the first one aimed at
the right thing. The previous two reduced how often we take the import lock;
the latest traceback shows mistralai.chat failing on its first and only
attempt, so volume was never the variable.
The real cause is in the interpreter. Checked across all five versions we
support:
3.10, 3.11 _blocking_on holds one slot per thread, and
_ModuleLock.acquire() clears it unconditionally in finally
3.12+ list-based stack (fixed upstream)
So when an import nests on one thread, the inner frame clears the slot and
the outer frame's cleanup raises KeyError: <thread id>. Patcher resolution
cannot avoid importing optional submodules, so we have to tolerate it.
Catch only that KeyError, retry once (the bookkeeping is per-call, so a
fresh slot normally works), and if it trips again report the module as
unavailable -- setup() runs inside the caller's application, and skipping one
optional patch target beats raising an interpreter-internal KeyError at them.
ImportError and everything else propagate untouched, so a genuinely absent
module is still reported absent. On 3.12+ a KeyError is a real bug and
re-raises.
Keeps the negative cache from the previous commit: 96 failing imports down to
7 over two setup() calls is worth having on its own, it just was not
sufficient.
Verified: pylint + test_types on 3.10-3.14 (10/10), test_mistral(latest) 5/5
on 3.10, test_core on 3.10 and 3.14, and an eight-session integration sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02306056ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| except KeyError: | ||
| if not _IMPORT_LOCK_BOOKKEEPING_IS_BUGGY: | ||
| raise | ||
| return None |
There was a problem hiding this comment.
On supported Python 3.10 and 3.11, an optional provider module whose import body legitimately raises KeyError reaches this block after being executed twice and is then reported as unavailable. This silently skips instrumentation instead of allowing _try_patch() to log the provider/configuration failure, and repeating the import may duplicate module-level side effects. Only suppress a KeyError verified to originate from the import-lock bookkeeping cleanup rather than every KeyError raised during import.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #783, which merged before this landed. Addresses the Codex review finding on that PR.
The problem
#783 added a
sys.modulesfast path to_import_optional_moduleso patcher resolution stops reacquiring the import lock on everysetup(). It returned any module present insys.modules.The loader inserts a module into
sys.modulesbefore executing its body. So whensetup()races a provider import on another thread, the shortcut hands back a half-built module: its patch targets do not exist yet,applies()concludes they are absent, and the patcher is silently skipped.importlib.import_modulewould have blocked on the per-module lock until the other thread finished.That is the same failure mode #783 set out to fix for openai's lazy modules, reintroduced in a narrower window — worth closing rather than leaving.
The fix
Guard the shortcut with the spec's
_initializingflag. This is not an invented heuristic: it is the predicate CPython itself uses for the same decision. From 3.11+importlib._bootstrap._find_and_load:Why keep the shortcut at all
3.10's
_find_and_loadhas no such optimization — it takes the module lock unconditionally:That unconditional lock is what produced the
KeyError: <thread id>fromimportlib._bootstrapon the ubuntu/3.10 shard of #783 (3.10's_blocking_onis one slot per thread, so a re-entrant import deletes the outer frame's entry). On 3.11+ this shortcut is effectively redundant with CPython's own; on 3.10 it is doing real work.Testing
Added
test_import_optional_module_waits_for_initializing_module, which puts a module with_initializing = Trueinsys.modulesand asserts we fall through to the import machinery, then that the shortcut applies again once initialization completes.test_coregreen on 3.10 (859 passed) and 3.14 (861 passed).🤖 Generated with Claude Code