import: route __import__ through the importlib bootstrap; frozen table; lazy loader dispatch - #746
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit acc1a7c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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
https://github.com/youknowone/pyre/blob/7eec10610d8aadfa70214da8f34ffbfd94fc9a04/pyre-interpreter/src/typedef.rs#L1824-L1827
Starting here ModuleType.__new__ now allocates the module on the managed GC heap, but the module GC shape still only traces W_MODULE_GC_PTR_OFFSETS (w_dict) while the subclass path immediately stores cls into the header w_class. For types.ModuleType subclass instances, if the heap class is otherwise unrooted by the next collection, the module no longer marks that class; and if this allocation fell back directly to old-gen, the post-allocation store also lacks a write barrier for a young class. This can leave type(m) and slot dispatch with a collected/stale class pointer, so the managed module shape needs to include W_CLASS_OFFSET and the retag store needs to be initialized/barriered accordingly.
AGENTS.md reference: AGENTS.md:L194-L195
https://github.com/youknowone/pyre/blob/7eec10610d8aadfa70214da8f34ffbfd94fc9a04/pyre-interpreter/src/baseobjspace.rs#L4675-L4676
When a ModuleType subclass overrides __getattribute__ and that override raises AttributeError, this branch sends the error to the module-dict PEP 562 __getattr__ hook. That hook is only part of the default Module.descr_getattribute__ path; after a custom slot raises, PyPy/CPython's descriptor operation falls back to __getattr__ on the receiver type (or propagates the original error), so a lazy/proxy module with a class-level __getattr__ is bypassed and a dict-level hook can mask deliberate failures.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ 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 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 address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf4d092943
ℹ️ 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 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 address that feedback".
| match get_and_call_function(slot, obj, w_type, &[name_obj]) { | ||
| Ok(v) => return Ok(v), | ||
| Err(e) if e.kind == PyErrorKind::AttributeError => { | ||
| return module_getattr_hook_or_err(obj, name, e, call_getattr); |
There was a problem hiding this comment.
When a ModuleType subclass replaces __getattribute__ and that override raises AttributeError, PyPy's generic space.getattr falls back to the type-level __getattr__ (pypy/objspace/descroperation.py:242-245); the module-dict PEP 562 hook in module.py is only the tail of the default Module.descr_getattribute. This catch routes replacement-slot misses to module_getattr_hook_or_err instead, so subclass __getattr__ is skipped and a module.__dict__['__getattr__'] can incorrectly run for custom module subclasses. Please route this case through the generic type __getattr__ path (or re-raise when none exists) rather than the module dict tail.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
module_descr_new built modules with malloc_typed, an off-heap allocation the collector never traces. A collection that moved the module dict left Module.w_dict pointing at the old location, so attribute access through a Python-created module read freed memory. Add lltype::malloc_typed_managed: allocate through the installed GC hook so the type id's registered trace offsets apply, then run the write barrier for the no-collect nursery allocator's old-generation fallback. Route types.ModuleType.__new__ through it via w_module_new_managed. Bootstrap/import module constructors keep the legacy stable allocation because native owners still hold raw pointers to them. The translator recognizes malloc_typed_managed at every site that recognizes malloc_typed (annotation, flowspace, NewWithVtable fusion, registry skip, unfused-call rejection). Assisted-by: Codex
Port interp___import__ (_frozen_importlib/interp_import.py) as
importing::dunder_import and bind it as builtins.__import__: a fast
path answers absolute imports from initialised sys.modules entries
(spec._initializing check, dotted-name top-module return, fromlist
__path__ check), the slow path calls the app-level
_bootstrap.__import__, and the native importhook stands in while the
bootstrap is not installed. The frozen bootstrap aliases
_frozen_importlib{,_external} stay on the native importer because
_install_external_importers imports them before PathFinder exists.
Install the bootstrap at startup: run_source and the REPL now run
init_importlib_bootstrap (non-fatal on failure) before import_site,
and the function body shrinks to importing importlib._bootstrap —
the native load hook fires _install(sys, _imp),
_install_external_importers(), and the frozen aliases when the
module body finishes, and a cached module skips the hook so a rerun
does not duplicate sys.meta_path entries.
Assisted-by: Claude
pyc_magic_number_token was a bare 3495; MAGIC_NUMBER derives from it as to_bytes(4, 'little') and release-gated tests pin the 3.14 value. Use 0x0A0D0E2B: magic 3627 in the low half, the \r\n marker in the high half. Cache files are already segregated by sys.implementation.cache_tag. Assisted-by: Claude
gcd_import_fast consulted check_sys_modules, which skips a None entry
and falls back to the interpreter cache — resurrecting an
already-loaded builtin whose name the sentinel blocks. Give up before
the lookup so the slow path raises
'import of {name} halted; None in sys.modules'.
Assisted-by: Claude
Add the static frozen table (bootstrap aliases, zipimport, the __hello__/__phello__ test rows) and implement find_frozen, get_frozen_object, is_frozen, is_frozen_package and _frozen_module_names against it. get_frozen_object compiles the entry's stdlib source (or the __hello_only__ literal) under a '<frozen name>' filename. _override_frozen_modules_for_tests stores a process-wide tri-state: positive serves every row, zero and negative only the bootstrap rows. Set sys._stdlib_dir from the resolved stdlib path; FrozenImporter._resolve_filename reads it to reconstruct source filenames. The importlib installer stamps __loader__/__spec__ from FrozenImporter.find_spec on the two bootstrap aliases once installed. Assisted-by: Codex
The inlined default module lookup in getattr_str_impl ran before any check of the receiver type, so a module retagged to a ModuleType subclass (importlib.util._LazyModule) never saw its __getattribute__ override. Dispatch a non-default override first, memoized per type so plain modules keep the inlined path, and route the default module slot through module_getattribute so an override calling super().__getattribute__ does not redispatch itself. The module-dict __getattr__ fallback moves into a shared tail. setattr_str/delattr_str and the surrogate-name variants now resolve the receiver type uniformly (instance type or retagged class) and call a non-default __setattr__/__delattr__ through __get__ binding. descr_set___class__ additionally accepts the exact module type as an assignment target, letting a subclass restore ModuleType after temporarily overriding the slots. Assisted-by: Codex
py_class!/py_class_typed! minted a W_TypeObject per thread through a thread_local OnceCell, as did GETSET_DESCRIPTOR_TYPE and the scoped module type cells in posix, pwd, grp, resource, time, mmap, zlib and error. Store them in process-global OnceLock<usize> cells instead, the shape #[pyre_methods] codegen already emits: the W_TypeObject is an immortal allocation and its movable children are forwarded by the builtin-type namespace root walker, so a shared pointer never goes stale. This also makes the eager set_instantiate write-once instead of last-writer-wins across threads. The structseq registry moves with them: keyed by type pointer, it must resolve classes minted on any thread, so the thread_local RefCell becomes OnceLock<Mutex<IndexMap>>. Lock scopes cover only lookup and owned-metadata cloning, never a call into Python. Assisted-by: Codex
EXC_CLASS_BY_KIND becomes a process-global [AtomicUsize; EXC_KIND_COUNT] with first-writer-wins registration returning the canonical class. EXC_CLASS_REGISTRY becomes a process-global OnceLock<Mutex<HashMap<&'static str, usize>>>. The exception-class builders (make_exc_type_with_init, make_exc_type_multi, make_exception_group_type) return the registered canonical class when one exists instead of minting a duplicate. Assisted-by: Codex
w_module_new_managed routes through malloc_typed_stable instead of malloc_typed_managed. Modules created from Python (types.ModuleType, module_from_spec) reach JIT traces as promoted constants; the movable allocation let a minor collection relocate the module under a baked pointer, crashing test_strftime in script mode with the JIT on. The GC trace shape (W_MODULE_GC_TYPE_ID offsets + write barrier) is unchanged, so w_dict forwarding still works. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/910fba61239a52eb461e147af551f7c438dadaba/pyre-interpreter/src/module/imp/interp_imp.rs#L122
When _override_frozen_modules_for_tests(0) is used to restore the normal mode (see test.support.import_helper.frozen_modules()), this predicate still serves only the always-frozen bootstrap set, so __hello__, __phello__, and the other test frozen modules disappear after every context manager exit and are also disabled in the default state. PyPy's corresponding helper treats only negative override values as disabled and lets 0 fall through to the normal enabled mode (pypy/module/imp/interp_imp.py:100-107), so make the disabled branch mode < 0 rather than mode <= 0.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ 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 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 address that feedback".
install_importlib_bootstrap can fail after importlib._bootstrap is already registered in sys.modules (on Windows _bootstrap_external imports the nt builtin, which pyre does not register). dunder_import routes through _bootstrap.__import__ whenever that entry exists, so the half-installed bootstrap answered every import with no PathFinder on sys.meta_path and file imports failed wholesale. Remove the bootstrap entries from sys.modules on install failure so the native importer stays authoritative, matching the boot sequence's documented non-fatal fallback. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc1a7c934
ℹ️ 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 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 address that feedback".
|
|
||
| fn frozen_module_served(entry: &FrozenModule) -> bool { | ||
| let mode = FROZEN_OVERRIDE.load(Ordering::Relaxed); | ||
| mode > 0 || (mode <= 0 && is_bootstrap_frozen(entry.name)) |
There was a problem hiding this comment.
With the override in its reset/default state (0), this predicate only serves the bootstrap names, so ordinary calls like _imp.is_frozen('__hello__') or FrozenImporter.find_spec('__phello__') report false unless a test temporarily sets the override to 1. In the Python 3.14 import machinery, 0 means the normal frozen table is enabled and only -1 disables non-essential frozen modules, so this makes most of the new frozen table invisible during normal imports and can break environments that rely on FrozenImporter instead of filesystem copies. Please treat mode == 0 like enabled and reserve negative values for the non-essential-module disable case.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #731. Six commits raising test_importlib coverage.
Commits
module_descr_newbuilt modules withmalloc_typed, an off-heap allocation the collector never traces; a collection that moved the module dict leftModule.w_dictstale, corrupting attribute access on Python-created modules (deterministictest_threaded_importSIGSEGV). Addslltype::malloc_typed_managed(allocation through the installed GC hook + write barrier for the old-gen fallback) and routestypes.ModuleType.__new__through it. Bootstrap module constructors keep the legacy stable allocation.interp___import__(_frozen_importlib/interp_import.py): fast path answers absolute imports from initialisedsys.modulesentries; slow path calls the app-level_bootstrap.__import__(fullsys.meta_path/sys.path_hooksprotocol); the native importhook stands in while the bootstrap is not installed.run_sourceand the REPL now install the bootstrap beforeimport site.pyc_magic_number_token=0x0A0D0E2B(magic 3627 +\r\nmarker).Noneentry insys.modulesmust block the name instead of resurrecting the cached builtin (caught bysynth/import_none_sentinel).zipimport,__hello__/__phello__rows) behindfind_frozen/get_frozen_object/is_frozen/is_frozen_package/_frozen_module_names, tri-state_override_frozen_modules_for_tests, andsys._stdlib_dirseeding forFrozenImporter._resolve_filename.ModuleTypesubclass (importlib.util._LazyModule) never saw its__getattribute__/__setattr__/__delattr__overrides. Non-default overrides now dispatch first, memoized per type so plain modules keep the fast path;__class__assignment accepts the exact module type as a restore target.Verification
check.py --backend dynasm297/297,--backend cranelift297/297 at the branch head.test_threaded_import(was SIGSEGV),test_util,test_lazy,frozen.test_finder,frozen.test_loader,test_api,test_pkg_import,import_.*. The tworegalloc.rs:1291JIT-panic targets (metadata.test_api,resources.test_files) stopped triggering the panic.resources.*,namespace_pkgs), marshal error parity (source.test_file_loader), CPython-standalone artifacts (import_.test_path,test_windows), the gh-134100 C-import-only expectations (import_.test_relative_imports), andtest_locks(blocked on real threading).🤖 Generated with Claude Code