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

Support editable installs of packages containing Cython declaration files #442

Closed
vyasr opened this issue Jul 29, 2023 · 6 comments · Fixed by #516
Closed

Support editable installs of packages containing Cython declaration files #442

vyasr opened this issue Jul 29, 2023 · 6 comments · Fixed by #516

Comments

@vyasr
Copy link
Contributor

vyasr commented Jul 29, 2023

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:

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from foo.bar_a cimport baz
  ^
  ------------------------------------------------------------

  ...: 'foo/bar_a.pxd' not found

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:

install({'foo.bar_a': '/path/to/foo/bar_a.pyx', 'foo.bar_b': '/path/to/foo/bar_b.pxd'}, {'foo.bar_a': 'foo/bar_a/lib.cpython-310-x86_64-linux-gnu.so'}, '$build_dir', False, True, [], [])

In this case, I've introduce an additional module foo.bar_b that is only a set of (in this case cdef extern) declarations. That could be useful for the purpose of e.g. doing what Cython's C/C++ standard library support does with cimporting from libc or libcpp.

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.

@henryiii
Copy link
Collaborator

henryiii commented Jul 29, 2023

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" .pyx files in your CMakeLists, only the .so's need to be "installed". Ahh, you are probably trying to put them in-place, so they get auto-discovered. Not fond of that (mixing compiled and .py files in source expecting a different mixture in the wheel), but it should be possible with a few wheel ignores (**.pyx).

@vyasr
Copy link
Contributor Author

vyasr commented Jul 29, 2023

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. :)

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.

Ahh, you are probably trying to put them in-place, so they get auto-discovered. Not fond of that (mixing compiled and .py files in source expecting a different mixture in the wheel), but it should be possible with a few wheel ignores (**.pyx)

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 install(...); Python will ignore them anyway, so it seems like it would be mostly harmless.

@LecrisUT
Copy link
Collaborator

I think this will still be a problem. This one relates to this comment

Do you know a python way to get the list of functions, or any other way to test if a library is cython or not?

From what? A module (dir(module)), or something else? Why do we need to know if something is Cython?

This is because loadable library modules with suffix .so must contain PyInit, i.e. you can't have a shortcut of import my_pkg.bundled_lib as a shortcut for just load my C library. It must be a valid python module. So if you could do the equivalent of readelf you can then distinguish between bundled library and python module.

I'll try to get back to that PR this weekend.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Sep 28, 2023

@vyasr based on the approach to #399 with tests/packages/navigate_editable, do you think you can add/write up a mwe of the failures here? There is the one issue if you include .c files in the python source, but in your case, the issue you were describing contains another aspect if I read it correctly. For me I would need an example to understand this

@vyasr
Copy link
Contributor Author

vyasr commented Sep 28, 2023

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.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 28, 2023

@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

henryiii added a commit that referenced this issue Nov 29, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants