fix(editable): recognize Cython __init__.pxd/.pyx as package indicators#1292
fix(editable): recognize Cython __init__.pxd/.pyx as package indicators#1292vyasr wants to merge 5 commits intoscikit-build:mainfrom
__init__.pxd/.pyx as package indicators#1292Conversation
- In ScikitBuildRedirectingFinder.__init__: use os.path.basename(file).partition('.')[0]
== '__init__' instead of '__init__.py' in file, so any __init__.* extension
(including .pxd and .pyx) is recognized as a package indicator.
- In find_spec: use 'fullname in self.pkgs' (already computed) instead of
redir.endswith(('__init__.py', '__init__.pyc')) for deciding whether to pass
submodule_search_locations, making it extension-agnostic.
- In mapping_to_modules: replace dict comprehension with explicit loop that
prefers .py/.pyc entries over other extensions (.pxd, .pyx) when multiple
source files map to the same module key.
vyasr
force-pushed
the
fix/cython_package_dirs
branch
from
0ae31b2 to
3f3f8d4
Compare
There was a problem hiding this comment.
Pull request overview
Fixes editable-install package recognition for Cython projects where packages may have __init__.pxd/__init__.pyx, preventing incorrect __path__ construction and import resolution issues (per #1291).
Changes:
- Treat any
__init__.*file as a package indicator when building package metadata for the redirecting finder. - Use the precomputed package list (
pkgs) to decide when to attachsubmodule_search_locationsinfind_spec. - Make
mapping_to_modules()deterministic for collisions by preferring.py/.pycover other extensions like.pxd/.pyx.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/scikit_build_core/resources/_editable_redirect.py |
Improves package detection and spec creation for packages whose init file isn’t .py/.pyc. |
src/scikit_build_core/build/_editable.py |
Makes module mapping stable by preferring importable Python sources when multiple files map to the same module name. |
tests/test_editable_redirect.py |
Adds regression tests for .pxd/.pyx init handling and mapping collision preference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Prefer .py/.pyc over other extensions (e.g. .pxd, .pyx) for the same module | ||
| if module not in result or rel.suffix in (".py", ".pyc"): | ||
| result[module] = str(Path(k).resolve()) |
There was a problem hiding this comment.
The logic here is a bit hard to parse, although the whole logic in these is hard to parse and could use a refactor. Iiuc mapping_to_modules is used on the src files, and libdir_to_installed is used for the CMake installed files. Why not drop all files that do not end with .py? It will interfere with importlib.resources.files, but I do not think the status-quo is helping us there either.
My proposal is to split (and rename) the inputs to _make_editable into 3 variables:
source_modules: mapping of python modules to source.pyfiles. All other file types are ignored and presumed that they are compiled. Not sure how to handle.pycthoughinstalled_modules: mapping of python modules to compiled python modules or installed.pyfilesfile_tree: a complete file tree of python navigable sources viaimportlib.resources.files. It could be a mapping of modules and list of file paths
@henryiii @vyasr what do you think? The second variable at the very least would unblock me for #1170
There was a problem hiding this comment.
Iiuc mapping_to_modules is used on the src files, and libdir_to_installed is used for the CMake installed files.
Yes this matches my recollection as well.
The split sounds reasonable. I assume file_tree will have to be modified substantially as we rework importlib.resources.files over time.
There was a problem hiding this comment.
Not sure how to handle .pyc though
How many realistic use cases are there for shipping pyc files in wheels? They shouldn't be there most of the time, but I'm sure there are edge cases I'm not thinking of.
There was a problem hiding this comment.
How many realistic use cases are there for shipping pyc files in wheels?
For wheels yes it does not make sense, but this part is about usage of editable install paths which is a bit more questionable. I don't know whose responsibility these are or if there is some magic to be careful about in the importlib
There was a problem hiding this comment.
Should we merge this as a fix, then work on a refactor? I could probably just feed that comment into Kimi or GPT and have it do the refactor, actually.
There was a problem hiding this comment.
👍 for me. LGTM in the current state as an intermediary.
Not sure how to handle
.pycthough
will be tricky but we can move that discussion
Fixes #1291.
Three targeted fixes for editable installs of Cython projects where packages have
__init__.pxdor__init__.pyxfiles alongside (or instead of)__init__.py:_editable_redirect.py— package detection (line 113):"__init__.py" in file→os.path.basename(file).partition(".")[0] == "__init__"— matches any__init__.*extension, consistent withpath_to_module()which already handles this correctly._editable_redirect.py—find_spec(lines 154/163):redir.endswith(("__init__.py", "__init__.pyc"))→fullname in self.pkgs— uses the already-computed package list rather than re-checking file extensions._editable.py—mapping_to_modules:Dict comprehension → explicit loop that prefers
.py/.pycwhen multiple source files (e.g.__init__.py+__init__.pxd) map to the same module key.