feat: Support `importlib.resources` in edittable installs #399

LecrisUT · 2023-06-22T14:32:23Z

Here's an initial attempt at getting importlib.resources to work with editable install.

Closes #388

LecrisUT · 2023-06-22T14:40:26Z

I need some advice on getting the pre-commit working and on how I can run pip install -e . and have it link to the version of scikit-build-core in my virtual environment.

LecrisUT · 2023-06-22T17:44:45Z

Ok, this one is somehow working. 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?

About test issues:

module.__path__ seems to not be supported, probably need to alter the Loader to set_data properly. But then would that work as an editable install? or is set_data not affecting the source?
3.7 is not supported because there is no Traversable class in importlib.abc. Any suggestions on what to do with that? Same for mypy issues (btw if you upgrade mypy's python version, a bunch of errors arise)

henryiii · 2023-06-23T20:12:45Z

I need some advice on getting the pre-commit working

If it's the mypy part, I can touch that up in _editable.py; given that that's imported whenever you start up Python, keeping it minimal is important - I almost don't even want to import typing to get typing.TYPE_CHECKING. But we can use that to protect anything else that's only imported for typing (including importlib.abc.Traversable). Also that's moving to importlib.resources.abc.Traversable.

pip install -e .

Easiest way is to add --no-build-isolation. That's better for editable installs usually anyway. I'd love a single-letter version of this to make it easier to use (ideally one that could also be used with build for --no-isolation).

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?

module.__path__

Pretty sure we can get that working, though manipulating it won't always be correct.

henryiii · 2023-06-23T20:13:54Z

We should also clearly support (or not support) namespace packages.

LecrisUT · 2023-06-23T20:29:51Z

--no-build-isolation

Thanks a lot. Now I understand what that option does.

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.

module.__path__

Pretty sure we can get that working, though manipulating it won't always be correct.

That one is weird, because I imagined that using the native set_data would fix that, but it seems to be a different section that manipulates that.

We should also clearly support (or not support) namespace packages.

I haven't played around with namespaces, but isn't that natively supported due to the tree mapping? Got any folder structure or mwe to see how namespaces are supposed to work?

src/scikit_build_core/resources/_editable_redirect.py

LecrisUT · 2023-06-23T23:24:31Z

Idea for the future: navigate the cmake-file-api to extract the install paths without doing the actual install. Then use the build/source files directly. That way library linkage is preserved without patching rpaths.

The file-api can also be used to find the python module target by checking their dependencies assuming the python binding is set to private.

I was trying to find a way to check if the configuration is dirty, but couldn't find a way. But on the other hand the build system could handle the unnecessary re-builds. The configure-log from 3.26 can be used to check if a build was triggered, or some simple stdout parsing.

henryiii · 2023-06-24T03:32:50Z

FYI, one of the first things added to scikit-build-core was the file API module, but we've not used it yet anywhere except tests. It reads the FileAPI into fully typed dataclasses.

LecrisUT · 2023-07-31T08:54:55Z

TODO:

Update the test to try various path locations and import (i.e. test that setting __path__ is sufficient to resolve sources in src-dir or build/install dir)
Update the editable install to inject all __path__

I think that is all the todos for this PR?

vyasr · 2023-09-22T18:06:57Z

Hi @LecrisUT @henryiii any updates on this PR? Totally understand things get busy, so just checking in. I'm happy to try and help get this over the finish line if that would be useful/appreciated (to be clear I don't know much about this PR so would have to get myself up to speed, but I'm willing to do that), but I'm also OK waiting if there's a plan to get back to this. I don't want to step on any toes, was just hoping to tackle #442 at some point soonish and I know @henryiii wanted this PR to get done first.

LecrisUT · 2023-09-23T05:49:26Z

I will work on this more today and try to get the tests for the different cases with a simple python binding

LecrisUT · 2023-09-27T15:29:09Z

@henryiii I am starting over with this one, I will start with a2e7452, do the type-checking and other style fixes, add importlib calls to the tests and then bring back the implementation that was there so far.

henryiii · 2023-09-27T17:20:23Z

Okay, couple of things to keep in mind:

Every single usage of Python imports this file, just to start up the import system. You really want to avoid importing anything unnecessary (like pathlib) (unless it’s already imported by something else).

It’s fine if we have to disable linter checks for this file. It’s not normal Python, don’t worry if a check is telling you to use pathlib, etc.

henryiii · 2023-09-27T17:29:01Z

Also, I prefer import x and x.y over from x import y. It’s easier for a reader to see where things come from, reduces name collisions, and most importantly protects from circular import issues. (Types and common imports are okay)

Also A | None is better than Optional[A] as long as you can use the future import (which is also faster).

tests/test_pyproject_pep660.py

src/scikit_build_core/resources/_editable_redirect.py

henryiii · 2023-10-02T21:33:30Z

AFAICT the only remaining issue is the weird 3.9 UNIX behavior. I've added a macOS run so we can see if it's all UNIX or just Linux.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Remove leftover comment Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

LecrisUT · 2023-10-04T08:29:12Z

Ok, seems like I've overlooked something. If the shared_pkg.data is a package files("shared_pkg.data") will generate a Path object which does not have multiple path identities. But if it isn't a package it will generate a MultiplexedPath. Not sure what causes this distinction 😟. It seems we still might have to implement the approach that was in meson. Should we for now just support editably importing non-package files for now or keep working in this PR? Currently this Pr does not affect previous workflows and we could mark it as experimental with known failures for navigating packages and 3.9 specifically.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii · 2023-10-05T13:00:21Z

I've added xfails. We could put this in then continue to work on getting rid of the xfails in followups.

LecrisUT · 2023-11-13T16:19:08Z

@henryiii Quick update about the Python 3.9 issue. Apparently using importlib_resources for that python version does fix the issue. Since it is needed for 3.8 anyway, I think it would be reasonable to document that it should be a dependency for 3.9 as well?

henryiii · 2023-11-13T17:23:35Z

I thought I checked that. Yes, if it works, then we can document that & test it.

LecrisUT requested a review from henryiii June 22, 2023 14:32

LecrisUT force-pushed the editable/importlib branch 5 times, most recently from 5291b67 to 4198d3a Compare June 22, 2023 17:17

LecrisUT mentioned this pull request Jun 22, 2023

feat: Switch to importlib.resources spglib/spglib#308

Merged

1 task

LecrisUT changed the title ~~Rework editable install to work with importlib.resources~~ feat: Support importlib.resources in edittable installs Jun 22, 2023

LecrisUT mentioned this pull request Jun 23, 2023

Allow mapping to sub-packages pfmoore/editables#20

Closed

LecrisUT commented Jun 23, 2023

View reviewed changes

src/scikit_build_core/resources/_editable_redirect.py Outdated Show resolved Hide resolved

henryiii mentioned this pull request Jun 30, 2023

Proper setup for editable installs wjakob/nanobind_example#30

Closed

henryiii mentioned this pull request Jul 29, 2023

Support editable installs of packages containing Cython declaration files #442

Closed

LecrisUT force-pushed the editable/importlib branch 2 times, most recently from 575e801 to 6efa883 Compare September 25, 2023 12:01

henryiii force-pushed the editable/importlib branch from 02d3357 to e211960 Compare September 27, 2023 15:14

LecrisUT force-pushed the editable/importlib branch 2 times, most recently from b0f084d to 0a415e2 Compare September 27, 2023 17:19

henryiii force-pushed the editable/importlib branch 2 times, most recently from 5af5bd1 to b01fcbe Compare September 30, 2023 17:22

LecrisUT commented Sep 30, 2023

View reviewed changes

tests/test_pyproject_pep660.py Outdated Show resolved Hide resolved

vyasr mentioned this pull request Oct 1, 2023

Support editable installs of Cython pxd files #516

Merged

LecrisUT commented Oct 2, 2023

View reviewed changes

tests/test_pyproject_pep660.py Outdated Show resolved Hide resolved

LecrisUT force-pushed the editable/importlib branch from e3cb861 to 33d0489 Compare October 2, 2023 19:53

henryiii reviewed Oct 2, 2023

View reviewed changes

src/scikit_build_core/resources/_editable_redirect.py Outdated Show resolved Hide resolved

LecrisUT and others added 10 commits October 4, 2023 09:34

Add basic editable install navigation tests

679675c

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add importlib.resources test

8a1f645

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Implement importlib.resources support

37d03cc

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Fix test_pep660

142227e

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Fix support for testing python < 3.9

4c6765b

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

fix(types): Use TypedDict to avoid type ignores

f4cddf0

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Remove leftover comment Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>

chore: minor touchup

8ba0774

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

tests: use samefile

6260886

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

tests: use samefile + EXT_SUFFIX

bee2bf0

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

ci: check 3.9 on macOS

a28a8e6

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

LecrisUT force-pushed the editable/importlib branch 2 times, most recently from 907c1b6 to 3dd0548 Compare October 4, 2023 07:38

tests: parametrize on data package

2972c04

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

LecrisUT force-pushed the editable/importlib branch from cf43dfe to 2972c04 Compare October 5, 2023 07:38

henryiii approved these changes Oct 5, 2023

View reviewed changes

henryiii merged commit fbbf3eb into scikit-build:main Oct 6, 2023

LecrisUT deleted the editable/importlib branch October 9, 2023 10:23

ajoubertza mentioned this pull request Oct 23, 2023

Import issues with relative paths and editable install #532

Closed

feat: Support importlib.resources in edittable installs #399

feat: Support importlib.resources in edittable installs #399

Uh oh!

Conversation

LecrisUT commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LecrisUT commented Jun 22, 2023

Uh oh!

LecrisUT commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Jun 23, 2023

Uh oh!

henryiii commented Jun 23, 2023

Uh oh!

LecrisUT commented Jun 23, 2023

Uh oh!

Uh oh!

LecrisUT commented Jun 23, 2023

Uh oh!

henryiii commented Jun 24, 2023

Uh oh!

LecrisUT commented Jul 31, 2023

Uh oh!

vyasr commented Sep 22, 2023

Uh oh!

LecrisUT commented Sep 23, 2023

Uh oh!

LecrisUT commented Sep 27, 2023

Uh oh!

henryiii commented Sep 27, 2023

Uh oh!

henryiii commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henryiii commented Oct 2, 2023

Uh oh!

LecrisUT commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Oct 5, 2023

Uh oh!

LecrisUT commented Nov 13, 2023

Uh oh!

henryiii commented Nov 13, 2023

Uh oh!

Uh oh!

feat: Support `importlib.resources` in edittable installs #399

feat: Support `importlib.resources` in edittable installs #399

LecrisUT commented Jun 22, 2023 •

edited

Loading

LecrisUT commented Jun 22, 2023 •

edited

Loading

henryiii commented Sep 27, 2023 •

edited

Loading

LecrisUT commented Oct 4, 2023 •

edited

Loading