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

breaking api change in 1.1.0 for importlib_resources.path(package, folder_name) #85

Closed
jaraco opened this issue Oct 21, 2020 · 16 comments
Closed

Comments

@jaraco
Copy link
Member

jaraco commented Oct 21, 2020

In GitLab by @RonnyPfannschmidt on Mar 2, 2020, 03:38

in a project we used
with importlib_resources.path(package, "data") as data_path:

to get a folder path (as back then traversal had not been supported)

with the update to 1.1.0 we see a
IsADirectoryError: [Errno 21] Is a directory: '/builds/*******/*****/.venv/lib64/python3.7/site-packages/***/data'

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 2, 2020, 18:08

Ugh. That's really unfortunate. Underlying this issue here is that prior to the introduction of files(), directories were not resources, as specified in the documentation. The fact that it worked in importlib 1.0 was probably an accident. And importantly, I would not expect that usage to work at all if your package is imported using zipimporter.

So now we need to decide what's the best path from here. I'm not sure restoring compatibility with the old lenient behavior is necessarily the best approach.

I'll need to do more investigation.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 2, 2020, 18:36

I'm tempted to recommend that users who relied on this behavior should do:

try:
  handle = importlib_resources.files(package).joinpath("data")
except AttributeError:
  handle = importlib_resources.path(package, "data")

# use Traversable operations on handle.

It's ugly, but

  1. Avoids creating a new release.
  2. Prefers the new files() API.
  3. Avoids the need for future adaptation when .path is removed.
  4. Works on zipimporter when importlib_resources 1.1 or later is present.

It does possibly require rewrite and doesn't fix issues with previous versions of the code against newer versions of importlib_resources. Is that a case that needs to be supported?

edit: the recommendation won't work because .path() is a context manager

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @RonnyPfannschmidt on Mar 3, 2020, 02:47

thanks for providing context, i can totally go with "worked due to a unfortunate bug, was never intended to work in 1.x" as the code itself was added using the 0.x series

for the time being we pinned ~=1.0.2 and based on the context we will get a better handle on the traversable api and port entirely to it

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 3, 2020, 05:10

mentioned in commit e3f4cb4

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 3, 2020, 05:14

mentioned in merge request !92

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 3, 2020, 05:26

mentioned in commit c2ea947

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 3, 2020, 05:31

In !92, I've drafted a change that would restore the prior expectation (suitable for a bugfix release) and in !93 drafted a subsequent change that would officially exclude that support in a backward-incompatible change. If you or other find that it's more than a minor inconvenience to adapt to the new API, I'll consider cutting a release to temporarily restore the expectation.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @RonnyPfannschmidt on Mar 3, 2020, 06:08

thanks for dealing that out on such a short notice

i already ported to the new api's,
but while doing so i stumbled upon a possible case where i couldn't consistently switch as it would mean 2 packages in my dependency tree depending on incompatible importlib

in my own case, it was possible to sort out by porting one of the dependencies to use the other and then following up with the api port on that one

but people in the wild may face something similar without the ability to do such a workaround

i suggest waiting for more input on this issue before cutting a release

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @maxking on Mar 9, 2020, 02:44

We are having the same issue with Mailman project :) I did manage to create a fix for the new API pretty easily, but since the new API does not exist in Standard Library, compatibility across Python versions becomes really hard.

Looks like many linux distros (Debian/Ubuntu/Gentoo that I know of) do not want to package importlib_resources for Python 3.7+ since it is a backport package.

I am not sure what the release policy for this library is, but we won't be able to use the new API until it is a part of the standard library too and since this is a new API and not bugfix, I am assuming that would be only in 3.9?

So, I guess my request is that could you keep the backwards compatibility in path API for longer (which I assume is restored by !92 and then removed again in !93)?

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 9, 2020, 03:42

I would advise Debian, etc to supply the backport for Python 3.8 and earlier now that it provides new functionality that won’t be backported to Python. In fact, they should probably not try to associate the two products and just supply them independently.

As for supplying the compatibility longer, my intention was to restore compatibility long enough for the 1.x releases to be compatible, but to remove the functionality immediately in a 2.x release. That will only be valuable if your project can pin to <2 in which case why not just pin to <1.1? I’m disinclined to restore compatibility for an extended period as that’s a maintenance burden to carry and limits other refactoring efforts.

Perhaps projects that require this compatibility should simply use:

pathlib.Path(package.__file__).parent.joinpath(‘data’)

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 18, 2020, 19:43

Interestingly, in bpo-39980, a user reported the inverse of this issue, indicating that it was unexpected that the implementation provided the interface on which the OP was relying.

Given that users have been able to find workarounds and that other workarounds exist, I'd rather move forward with the head as it's implemented and I'll withdraw the associated MRs. Feel free to comment or open a new issue if there's a more demanding use-case to restore this functionality.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Mar 18, 2020, 19:43

closed

@gitpushdashf
Copy link

.files().joinpath("dir") is working great, thank you!

Related: https://bugs.python.org/issue44162

Natrinicle added a commit to Natrinicle/pyloot that referenced this issue Jun 4, 2021
importlib.resources.path specifies that directories are not resources
and throws an error. Incorporated fix from Jaraco in
python/importlib_resources#85

sys.getcheckinterval is deprecated as of 3.2, use sys.getswitchinterval
instead per
https://docs.python.org/3.8/library/sys.html#sys.getcheckinterval

Added 3.9 to Tox and Github Workflows to ensure testing was
accomplished.
jjmurre added a commit to Amsterdam/amsterdam-schema that referenced this issue Aug 16, 2022
The function importlib.resoures.path worked for directories only by
accident. Resources should never be directories.

See: https://bugs.python.org/issue44162

This has now been fixed using `importlib.resources.files`
as suggested in: python/importlib_resources#85
jjmurre added a commit to Amsterdam/amsterdam-schema that referenced this issue Aug 16, 2022
The function importlib.resoures.path worked for directories only by
accident. Resources should never be directories.

See: https://bugs.python.org/issue44162

This has now been fixed using `importlib.resources.files`
as suggested in: python/importlib_resources#85
@a-gn
Copy link

a-gn commented Oct 5, 2022

In GitLab by @jaraco on Mar 2, 2020, 18:36

I'm tempted to recommend that users who relied on this behavior should do:

try:
  handle = importlib_resources.files(package).joinpath("data")
except AttributeError:
  handle = importlib_resources.path(package, "data")

# use Traversable operations on handle.

Hi, @jaraco, how can this be used to pass the entire folder to a separate program that requires a filesystem path to the data directory? This Traversable object does not seem to actually provide a path.

I see how this can be used to iterate over files inside the directory within Python, but I can't do that here.

@jaraco
Copy link
Member Author

jaraco commented Oct 5, 2022

@a-gn See #255, which added support for materializing a traversable directory of resources to the filesystem. It was released with importlib_resources 5.9.0.

@jaraco
Copy link
Member Author

jaraco commented Jul 7, 2023

FTR, I've archived the abandoned branches:

  • feature/85-drop-path-dirsrefs/archive/feature-85-drop-path-dirs
  • bugfix/85-path-dirsrefs/archive/bugfix-85-path-dirs

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

No branches or pull requests

3 participants