-
Notifications
You must be signed in to change notification settings - Fork 54
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
Support editable installs of packages containing Cython declaration files #442
Comments
Let's wait to see if this is still a problem after #399. I don't think that helps here, but one editable rewrite at a time. :) I'm pretty sure you don't want to "install" the header files, you just want to be able to find them in-place. Similarly, I'd expect you'd not want to "install" |
Sounds good :) I saw that PR but didn't inspect its contents enough to judge whether it might be relevant. But also please let me know if there's any experimentation you'd like to do to resolve this issue, happy to try to lighten the load by taking items off your plate that I'm capable of addressing.
I'm not sure I follow this. My project's CMake code has no install rules for pyx or pxd files, in case that's what you're suggesting: The only install rule I have is for the compiled extension module. If what you're saying is that you aren't fond of the fact that my prior message suggesting pyx and py files differently, I could see the argument for that. I don't think it would cause any problems to leave the pyx (or pxd) files in the dict passed to |
I think this will still be a problem. This one relates to this comment
I'll try to get back to that PR this weekend. |
@vyasr based on the approach to #399 with |
Sure. Let me first put together a stand-alone MWE and post that so that you can see the issue. Once I have that I'll try to rework it into something more like that you have in #399 so that it can be incorporated as a test case. |
@LecrisUT here's a gist that shows the problem. I know it's not the most convenient format, and I'll work on creating a proper test case like you requested, but wanted to demo the problem for you quickly in case it's helpful |
Editable installs of packages containing Cython currently do not properly support downstream Cython packages finding the pxd files from the editable install. The issue is that editable installs are managed using a meta path finder, and [Cython does not appear to support that for pxd files](cython/cython#5855). However, Cython does support pth files adding include paths directly, so this PR solves the pxd search issue by adding the necessary paths directly to the pth file that currently adds the meta path finder. Resolves #442 --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]>
Problem statement
I don't understand enough about how scikit-build-core is working here to be confident in any recommendation, so to avoid the XY problem I'll state exactly what I'm observing before I make any suggestions.
I have two projects that use Cython where one contains pxd files with declarations that I want to be able to
cimport
into the other package. If I install the first package, everything works fine when I build the second package. If I have an editable installation of the first package, however, Cython fails to find the headers:I would like for this to work.
Some observations
IIUC the way editable installs work is that build artifacts are copied into site-packages from the build directory (ideally not a temp one, so something specified like
build-dir=build/{wheel_tag}
in pyproject.toml) after the build completes, but Python modules are instead remapped via a custom import loader registered via*_editable.py
in site-packages.py.Inspecting this list reveals that Cython modules are being included. I see something like this in the installed
_editable.py
file:In this case, I've introduce an additional module
foo.bar_b
that is only a set of (in this casecdef extern
) declarations. That could be useful for the purpose of e.g. doing what Cython's C/C++ standard library support does withcimport
ing fromlibc
orlibcpp
.Note that, interestingly, the pyx file is included in the set of what I assume are source files, but the corresponding compiled extension module is also included in the second list of what I assume are meant to be importable extension modules (and the extension module is not in the source tree, it is placed into site-packages). On the other hand, the pxd file shows up in the first list and not in the second.
My guess is that the pyx file shouldn't be in the first list at all; Cython source files require compilation, so there's no reason to include them in the first list if it is indeed a list of source files that should be found in the search tree upon import. However, I don't think that's relevant for my problem, and I suspect that in practice it has no effect and importlib just ignores the pyx file. What I'm observing is because Cython's cimports don't have anything to do with Python's import machinery, so registering this import loader for a pxd file has no effect on Cython compilation.
Proposed solution
If my ramblings above seem correct, then my guess is that the best solution here would be for editable installs to support some way to specify an additional set of files that need to be installed into the site directory for an editable install. It is insufficient to only take extension modules when Cython headers are in use, so I think the pxd files need to be installed as well just like the DSOs. My suggestion of configuring an additional set of files (ideally globs or so) is to avoid indexing too heavily towards Cython and to instead make the support more general.
The text was updated successfully, but these errors were encountered: