Support getting files in the build tree from editable installs

[vyasr]

trafficstars

Resolves #807

[vyasr]

@LecrisUT here's the example for #807

[LecrisUT]

What is the type you get with importlib.resources.files(example)? It should be MultiplexedPath

Trying to refresh my reading of the workflow

[vyasr]

No, it is currently producing a PosixPath. But that is a good pointer, that does seem like the way that this should work. This seems similar to what I reported in #647.

[LecrisUT]

I remember submodule_search_locations was an important aspect that we converged to. Check around the execution of: https://github.com/scikit-build/scikit-build-core/blob/9d49270b3cb424dfb009bcaeaf79f26f2891fb80/src/scikit_build_core/resources/_editable_redirect.py#L83-L84

If self.submodule_search_locations['example'] has indeed a list of 2 then I think we have it wrong in the assumption of: https://github.com/scikit-build/scikit-build-core/blob/9d49270b3cb424dfb009bcaeaf79f26f2891fb80/src/scikit_build_core/resources/_editable_redirect.py#L91-L95 (fullname == "example" should be called there)

[vyasr]

Indeed, that seems to be it. I added

        if fullname in self.submodule_search_locations:
            print(
                f"The submodule_search_locations for {fullname} "
                f"are {self.submodule_search_locations[fullname]}"
             )
             submodule_search_locations: list[str]  = list(self.submodule_search_locations[fullname])

and I see

(skbuild_dev) vyasr@vyasr-mlt test % python -c "import example"
The submodule_search_locations for example are {'/Users/vyasr/miniconda3/envs/skbuild_dev/lib/python3.11/site-packages/example', '/Users/vyasr/local/testing/skbc_importlib/example'}

What would you recommend that we do?

[LecrisUT]

Is this with the default loader or the new one. If it's the same on the default, then we need to rethink it. One reference would be what do setuptools and hatchling do for namespace packages. I remember one of them didn't support it at that time. Meson and cargo could be additional references, but those would be harder to navigate and might not be as minimal.

Worst case is we go for the method of meson defining full Loaders, but it's not ideal. The difficulty iirc was with getting everything else other than files interface to work correctly, e.g. reading the source code.

[vyasr]

The output is the same both with and without the loader (commenting out # loader=ScikitBuildRedirectingLoader(fullname, redir),).

[vyasr]

@LecrisUT any other thoughts on how we ought to proceed here? I've committed the test to the repo so hopefully it's easy enough to reproduce the issue if you need to.

[LecrisUT]

@LecrisUT any other thoughts on how we ought to proceed here? I've committed the test to the repo so hopefully it's easy enough to reproduce the issue if you need to.

It is a difficult one and I think we need to separate what we are testing here:

• import for python modules .py and .so with editable and without. This is the minimal that should be tested and work. This is already in the test suite, but I think it will help if it is easier to find
• Navigate packages with import (same as previous point but focus on covering project structures). I was thinking of covering this with https://github.com/scikit-build/scikit-build-core/pull/535 but I don't like how hard it is to follow that one and track what navigation we are covering. It would help a lot if at least this part can be revisited.
• Navigate packages using relative path, i.e. within the source code
• Navigating using importlib.resources. If we cover the previous point, this part would be easier to follow. This part has too many moving pieces. What about covering the following way:
  • Go from testcase and work backwards to implementation
  • Test for the leaf (pkgA.pkgB.moduleC) and make sure it is always a pathlib.Path pointing to the resource in all cases
  • Test for the branches (pkgA, pkgA.pkgB) and make sure it is a pathlib.Path when not editable and a MultiplexedPath when editable
  • Naked modules (moduleD) always points to pathlib.Path Normally should be discouraged, but for prototyping I think this should be covered as well
  • Find how to fix the implementation
    • We should try to use as much vanilla functionality as possible, otherwise, make sure to cover execution, read source and relative navigation
    • Construct a virtual tree of pathlib.Path or MultiplexedPath at ScikitBuildRedirectingFinder constructor
    • Inject the virtual tree nodes in find_spec (get ready for all tests to breakdown from here)
    • Worst case scenario is we create our own Loaders as you made here, but if that's the case it is important to cover separately pkg vs python-module vs c-module (are there any c-package to worry about as well?), Extension/Source/Sourceless file loaders (there might be more) [^1] . We don't need to go as far as creating our own Traversable in this case

[^1] : https://github.com/mesonbuild/meson-python/blob/main/mesonpy/_editable.py

[vyasr]

That sounds like a solid plan. I don't follow the difference between your second and third top-level bullets, but in general the approach of adding tests for each of the possible cases and working backwards from there sounds right. This PR adds a test for one specific case from that list. I can work on slowly building this up. JFYI it will take me a few weeks to get back to this PR since I'll be traveling for the next two weeks.

@henryiii would you be open to me creating a PR that is just a bunch of xfailed tests indicating functionality that we would like to work and getting that merged then working backwards to fixing tests? I fear that the number of different cases alone will make this kind of work hard to keep track of, so getting test cases merged and consistently tested would already be a good starting point to indicate what we aspirationally want working.

[LecrisUT]

About 2nd and 3rd point, it is subtle but basically it involves how the Loader, submodule_search_locations, etc. interact. You remember the last PR on this, it basically addressed that.

[vyasr]

@henryiii friendly ping. Does the above approach of adding xfailed tests first sound OK?

[LecrisUT]

I think you should go ahead with the xfail approach. If needed I will open a branch and you can move the target to there.

[henryiii]

Ah, sorry, yes the xfail approach is preferred.

[vyasr]

Cool. I head out on vacation on Thursday and have a number of things to wrap up, so I may not get to this by then. If not, then I'll revisit in about 3 weeks (I'm off for 2, will realistically need a week to get caught up on everything else again).

[vyasr]

Working on the tests in https://github.com/scikit-build/scikit-build-core/pull/906.

[vyasr]

#906 looks like it's finally nearing readiness. Once that is done we can revisit this PR and see about fixing some of the cases that are now xfailed there.

[vyasr]

@LecrisUT I think we can return to the discussion of what to do in this PR now. I've rebased it onto the latest main with the new tests, so we can start removing some of the xfails to see how we might fix things.

[LecrisUT]

Yes, I need to look again at the importlib code over the weekend, but as far as I remember:

• The Loader/Reader approach would require full implementation of these classes. In order to use that approach we should construct and save a virtual file tree like meson is doing.
• The other approach would be to see in spec_from_file_location if there is any code that creates a MultiplexedPath. I would have guessed that submodule_search_locations is involved.

In the meantime, can you edit the test_importlib_resources and print out some tracing debug info? Maybe trace.Trace would be useful to navigate through.

[vyasr]

In the meantime, can you edit the test_importlib_resources and print out some tracing debug info? Maybe trace.Trace would be useful to navigate through.

Sure I can gather some info on this.

[vyasr]

Sorry I've dropped the ball here. I can catch back up here and see about getting more debug information.