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

Calling importlib_resources.files with an editable install fails #311

Closed
GarrStau opened this issue Jun 29, 2024 · 9 comments · Fixed by #315
Closed

Calling importlib_resources.files with an editable install fails #311

GarrStau opened this issue Jun 29, 2024 · 9 comments · Fixed by #315
Labels
bug Something isn't working

Comments

@GarrStau
Copy link

GarrStau commented Jun 29, 2024

When calling importlib_resources.files("sample-namespace") to retrieve a text file, an exception is thrown if an editable install has been done for the package.

I am able to trigger this with a relatively simple setup - I have a namespace package with two files, sample.txt and a __main__.py that has two lines:
from importlib_resources import files
files("sample-namespace")

In a venv with only importlib_resources 6.4.0 installed, calling "py -m sample-namespace" works without issue if sample-namespace is not installed or normally installed, but will fail if it is installed in editable mode.

The error is thrown in the MultiplexedPath constructor, because '__editable__.sample_namespace-1.0.finder.__path_hook__' is one of the paths provided, and path.is_dir() is False leading to raising NotADirectoryError.

I'm not convinced that I'm doing everything right, but I don't know of any limitations with editable installs. I'm not very familiar with this library, but if there's anything I can do to help, let me know.

Thank you!

(.venv) PS C:\git\TEST\test-importlib-resources-files-editable> py -m sample-namespace
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "C:\git\TEST\test-importlib-resources-files-editable\sample-namespace\__main__.py", line 3, in <module>
    files("sample-namespace")
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\_common.py", line 46, in wrapper
    return func(anchor)
           ^^^^^^^^^^^^
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\_common.py", line 56, in files
    return from_package(resolve(anchor))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\_common.py", line 116, in from_package
    reader = spec.loader.get_resource_reader(spec.name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\future\adapters.py", line 66, in get_resource_reader
    or super().get_resource_reader(name)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\_adapters.py", line 29, in get_resource_reader
    return CompatibilityFiles(self.spec)._native()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\_adapters.py", line 153, in _native
    reader = self._reader
             ^^^^^^^^^^^^
  File "C:\git\TEST\test-importlib-resources-files-editable\.venv\Lib\site-packages\importlib_resources\_adapters.py", line 147, in _reader
    return self.spec.loader.get_resource_reader(self.spec.name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 1425, in get_resource_reader
  File "C:\Users\garrstau\AppData\Local\Programs\Python\Python312\Lib\importlib\resources\readers.py", line 133, in __init__
    self.path = MultiplexedPath(*list(namespace_path))
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\garrstau\AppData\Local\Programs\Python\Python312\Lib\importlib\resources\readers.py", line 70, in __init__
    raise NotADirectoryError('MultiplexedPath only supports directories')
NotADirectoryError: MultiplexedPath only supports directories
@jaraco
Copy link
Member

jaraco commented Aug 19, 2024

I recently encountered this error too in jaraco.imaging. The same code previously passed on Apr 24, so my guess is there was some regression since then.

@jaraco
Copy link
Member

jaraco commented Aug 19, 2024

For what it's worth, editable installs used to work, but not since the path hook was added to the namespace path (probably a newer form of editable install).

Here's what I'm seeing for namespace path:

> /opt/homebrew/Cellar/python@3.12/3.12.5/Frameworks/Python.framework/Versions/3.12/lib/python3.12/importlib/resources/readers.py(133)__init__()
-> self.path = MultiplexedPath(*list(namespace_path))
(Pdb) namespace_path
_NamespacePath(['/Users/jaraco/code/jaraco/jaraco.imaging/.tox/py/lib/python3.12/site-packages/jaraco', '/Users/jaraco/code/jaraco/jaraco.imaging/jaraco', '__editable__.jaraco.imaging-2.1.1.dev173+gbde5bcf.finder.__path_hook__'])
(Pdb) import jaraco
(Pdb) jaraco.__path__
_NamespacePath(['/Users/jaraco/code/jaraco/jaraco.imaging/.tox/py/lib/python3.12/site-packages/jaraco', '/Users/jaraco/code/jaraco/jaraco.imaging/jaraco', '__editable__.jaraco.imaging-2.1.1.dev173+gbde5bcf.finder.__path_hook__'])

@abravalheri Do you know what it is that's adding that __editable__ entry to the _NamespacePath? Is there a spec that allows that path to be something other than an importable path? What should importlib_resources be doing (if anything) to honor that path entry?

@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

Oh, I see pypa/setuptools#3548 is already tracking the issue.

@abravalheri
Copy link
Contributor

abravalheri commented Aug 20, 2024

Do you know what it is that's adding that editable entry to the _NamespacePath? Is there a spec that allows that path to be something other than an importable path?

This is added by a .pth file added on purpose by the editable_wheel command in setuptools to the editable wheel. The objective is to trigger a PathEntryFinder, which is so far the only known way in the community to implement namespace packages via import finders. The entry added to sys.path is a path item... but not a conventional one: it is a path that does not exist but is there only to force the PathEntryFinder to be called.

I believe that the standard that supports this is PEP 302. In the text of the PEP it is suggested that adding a "non existing" path is an acceptable approach for a different class of problems (but nonetheless the approach described still adds a non existing file to sys.path):

For now, if a hook is required after sys.path has been processed, it can be simulated by adding an arbitrary “cookie” string at the end of sys.path, and having the required hook associated with this cookie, via the normal sys.path_hooks processing. In the longer term, the path handling code will become a “real” hook on sys.meta_path, and at that stage it will be possible to insert user-defined hooks either before or after it.

The Python docs also explicitly say:

...
Most path entries name locations in the file system, but they need not be limited to this.
...
sys.path contains a list of strings providing search locations for modules and packages. It is initialized from the PYTHONPATH environment variable and various other installation- and implementation-specific defaults. Entries in sys.path can name directories on the file system, zip files, and potentially other “locations” (see the site module) that should be searched for modules, such as URLs, or database queries. Only strings should be present on sys.path; all other data types are ignored.

Also here in https://docs.python.org/3/reference/import.html#finders-and-loaders:

The import path is a list of locations that may name file system paths or zip files. It can also be extended to search for any locatable resource, such as those identified by URLs.

So my interpretation is that sys.path can contain all sorts of stuff that are searched by finders/loaders. The text even mentions database queries, which is a pretty good example of not a regular file path. Nevertheless, since there is an associated PathEntryFinder, these entries work effectively as an "importable path" but not in the conventional sense.

What should importlib_resources be doing (if anything) to honor that path entry?

I am not sure here. PEP 320 mentions loader.get_data(path). I did not implement any custom loader in setuptools, just the finders that rely on standard functions of importlib, so all the loaders produced should also be fairly standard. Maybe the standard loaders of importlib implement the get_data protocol?

Another thought is that sometimes editable namespace packages do not correspond to an actual directory in disk, and it is only simulated by the importlib machinery. For example, consider that a developer uses package_dir in setuptools to remap the file paths and point package a.b.c to a project folder called x. We can map a.b.c to the existing x folder, but there is no correspondence for a or a.b.

In that case, users are better off if they avoid calling importlib_resources.files("a"). Instead, they will have to be more specific and call importlib_resources.files("a.b.c").


I believe that pypa/setuptools#3548 regards a different original problem.

To be honest, since we never got a reproducer, I believe that the original problem might be caused by another piece of software that expects all entries of sys.path to be existing regular paths to files or directories (and as explained above that is not how it is described by the Python docs). And if that is the case there is not much to be done by setuptools itself.

@abravalheri
Copy link
Contributor

but I don't know of any limitations with editable installs.

@GarrStau , there is a non-exhaustive list in https://setuptools.pypa.io/en/latest/userguide/development_mode.html#limitations.

@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

That helps a lot. Thanks for the context. I do believe it makes sense for importlib resources to support this mode, and it should continue to be possible to resolve resources from a namespace package (as long as some provider provides resources from that package).

jaraco added a commit that referenced this issue Aug 20, 2024
…amespace package with non-path elements in the path.

Ref #311
@jaraco jaraco added the bug Something isn't working label Aug 20, 2024
@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

It wasn't too hard to create a test that captures the failure (b06b5fb), but unfortunately, its traceback reveals another problem:

________________________________________________ OpenNamespaceTests.test_non_paths_in_dunder_path _________________________________________________

self = <importlib_resources.tests.test_files.OpenNamespaceTests testMethod=test_non_paths_in_dunder_path>

    @pytest.mark.xfail(reason="#311")
    def test_non_paths_in_dunder_path(self):
        """
        Non-path items in a namespace package's ``__path__`` are ignored.
    
        As reported in python/importlib_resources#311, some tools
        like Setuptools, when creating editable packages, will inject
        non-paths into a namespace package's ``__path__``, a
        sentinel like
        ``__editable__.sample_namespace-1.0.finder.__path_hook__``
        to cause the ``PathEntryFinder`` to be called when searching
        for packages. In that case, resources should still be loadable.
        """
        import namespacedata01
    
        namespacedata01.__path__.append(
            '__editable__.sample_namespace-1.0.finder.__path_hook__'
        )
    
>       resources.files(namespacedata01)

importlib_resources/tests/test_files.py:84: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
importlib_resources/_common.py:46: in wrapper
    return func(anchor)
importlib_resources/_common.py:56: in files
    return from_package(resolve(anchor))
importlib_resources/_common.py:117: in from_package
    reader = spec.loader.get_resource_reader(spec.name)
importlib_resources/future/adapters.py:66: in get_resource_reader
    or super().get_resource_reader(name)
importlib_resources/_adapters.py:29: in get_resource_reader
    return CompatibilityFiles(self.spec)._native()
importlib_resources/_adapters.py:153: in _native
    reader = self._reader
importlib_resources/_adapters.py:147: in _reader
    return self.spec.loader.get_resource_reader(self.spec.name)
<frozen importlib._bootstrap_external>:1425: in get_resource_reader
    ???
/opt/homebrew/Cellar/python@3.12/3.12.5/Frameworks/Python.framework/Versions/3.12/lib/python3.12/importlib/resources/readers.py:133: in __init__
    self.path = MultiplexedPath(*list(namespace_path))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = MultiplexedPath('/private/var/folders/f2/2plv6q2n7l932m2x004jlw340000gn/T/tmp2ubfwwvu/namespacedata01', '__editable__.sample_namespace-1.0.finder.__path_hook__')
paths = ('/private/var/folders/f2/2plv6q2n7l932m2x004jlw340000gn/T/tmp2ubfwwvu/namespacedata01', '__editable__.sample_namespace-1.0.finder.__path_hook__')

    def __init__(self, *paths):
        self._paths = list(map(pathlib.Path, remove_duplicates(paths)))
        if not self._paths:
            message = 'MultiplexedPath must contain at least one path'
            raise FileNotFoundError(message)
        if not all(path.is_dir() for path in self._paths):
>           raise NotADirectoryError('MultiplexedPath only supports directories')
E           NotADirectoryError: MultiplexedPath only supports directories

/opt/homebrew/Cellar/python@3.12/3.12.5/Frameworks/Python.framework/Versions/3.12/lib/python3.12/importlib/resources/readers.py:70: NotADirectoryError

The error is coming from importlib.resources, meaning that importlib_resources' readers aren't taking precedence, so it won't be possible to fix the issue strictly in importlib_resources without first making sure that the local readers take precedence.

@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

I do see that importlib_resources.future.adapters.TraversableResourcesLoader.get_resource_reader is getting to super().get_resource_reader(name), so ._standard_reader() would have to have been called before that, so it's probably just the case that _namespace_reader is not resolving because of this reported issue. That is, there's probably a ValueError on line 149:

@classmethod
def _resolve(cls, path_str) -> abc.Traversable:
r"""
Given an item from a namespace path, resolve it to a Traversable.
path_str might be a directory on the filesystem or a path to a
zipfile plus the path within the zipfile, e.g. ``/foo/bar`` or
``/foo/baz.zip/inner_dir`` or ``foo\baz.zip\inner_dir\sub``.
"""
(dir,) = (cand for cand in cls._candidate_paths(path_str) if cand.is_dir())
return dir

And that's causing the namespace reader to be skipped.

So, fixing the root issue might also address the secondary issue.

jaraco added a commit that referenced this issue Aug 20, 2024
When editable installs create sentinels, as they are not a valid directory, they're unsuitable for constructing a `MultiplexedPath`. Filter them out.

Fixes #311
@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

So, fixing the root issue might also address the secondary issue.

And indeed it does.

jaraco added a commit that referenced this issue Aug 20, 2024
When editable installs create sentinels, as they are not a valid directory, they're unsuitable for constructing a `MultiplexedPath`. Filter them out.

Fixes #311
jaraco added a commit that referenced this issue Sep 9, 2024
…amespace package with non-path elements in the path.

Ref #311
@jaraco jaraco closed this as completed in 2c145c5 Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants