Repository navigation
ResourceReader API can't be implemented for zipimporter #49
Description
In GitLab by @warsaw on Jan 2, 2018, 13:25
The ResourceReader APIs only take a single resource argument, but this is incompatible with the current implementation of zipimporter. The problem is that, unlike with file-based modules, all modules imported from a zip file share the same loader:
>>> import test.test_importlib.zipdata02
>>> import sys, os
>>> sys.path.insert(0, os.path.join(os.path.dirname(test.test_importlib.zipdata02.__file__), 'ziptestdata.zip'))
>>> import ziptestdata.two
>>> import ziptestdata.one
>>> ziptestdata.one.__spec__.loader == ziptestdata.two.__spec__.loader
TrueSo let's say you have a resource called myresource.txt in ziptestdata.two and ziptestdata.one. The implementation of ResourceReader.open_resource() on the zipimporter's loader can't possibly know which myresource.txt file is being requested.
@brettcannon mentioned PEP 302's design decision not to require a unique loader instance per module, and wanted to move away from that, but I think we're stuck with requiring a package argument to the ResourceReader. The only other option would be to change zipimporter to provide a unique loader instance per module, but that seems like a backward compatibility break. Besides, if other 3rd party loaders follow the same implementation of a shared loader, they won't be able to implement ResourceReader either.
Activity
jaraco commentedon Oct 21, 2020
In GitLab by @brettcannon on Jan 2, 2018, 14:27
Basically I don't like the API of specifying the package you want to work with because it's (1) redundant API in all cases where the loader is unique to the module, (2) the memory savings of sharing a loader is small, and (3) it's implementation complexity/overhead for an optimization case.
Changing it, though, buys us (1) not having to re-implement
zipimporterto have a unique loader per module (although they can obviously share almost everything underneath the hood through the delegate pattern). And I don't see how much of compatibility break it is to go from a sentinel loader to a loader per module.As you can see, not weighed equally. 😉 The other issue is if we change this API now then I consider us locked into this style of API for loaders for life as this is our last chance to change the tide in any way before it's just too obvious that's the style of loading support we are going for.
Or put another way, I'm okay with changing it if you're doing the
zipimporterchanges and I can blame you for eternity for us keeping that style of API, both if people ever ask for it and I can curse your name any time I have to implement something involving it. 😁jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Jan 2, 2018, 14:56
Oh sure, blame the FLUFL. It's okay, I can take it (in fact, I think that's my only official duty).
I'll post something to python-dev to get some feedback.
jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Jan 2, 2018, 15:17
BTW, I think the only way we can remove the package from the API is if we require a unique loader instance per module in Python. re: third party loaders.
jaraco commentedon Oct 21, 2020
In GitLab by @Yhg1s on Jan 2, 2018, 18:20
Does the API require a loader per module, or a loader per package? The latter makes perfect sense to me, the former seems workable but a little superfluous.
In the (Google-internal, at the moment) rewrite of zipimport, I've stopped equating loaders and finders, so there's a single finder per ZIP file, and a loader per path entry (essentially, per package) within the ZIP file. This has other benefits too, like it being sane and workable and efficient: each loader knows what package it's for and is identifiable directly, so the zipimporter doens't have to go through the path argument and figure out which part is the ZIP file each time.
jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Jan 2, 2018, 20:51
Only a unique loader per package, since resources are named relative to packages and may not contain path separators.
Interesting to hear about your rewrite (see also this PR which I didn't do anything with really except PR-ify Serhiy's patch). Do you have an ETA for open sourcing it? Possibly for 3.7?
It sounds like imposing a restriction on a unique loader-per-package wouldn't be a burden on you... but is it a good idea?
jaraco commentedon Oct 21, 2020
In GitLab by @gpshead on Jan 3, 2018, 03:48
I think the concept of a unique loader per package makes sense and would be happy with the concept.
But from a what to do for 3.7 point of view, it may be worth looking around at other loaders outside of our direct control to see how feasible this is.
Off the top of my head the only Python project I know of with a loader of its own is Mercurial, https://www.mercurial-scm.org/repo/hg/file/tip/hgdemandimport/demandimportpy3.py, but there are probably others. Run a BigQuery search over github for r
.py$file uses ofsys.path_hooksandsys.meta_pathperhaps?Here's a query:
https://bigquery.cloud.google.com/savedquery/255219552056:d864dde3546649db8c6378720bb424cd
... and results running that on the full data set rather than the
sample_tables:https://docs.google.com/spreadsheets/d/1EhpqYsC1CRizGvmDUTizsx0SdCXZT5ws8_AZNWpdBUM/edit?usp=sharing
That's quite a messy data set with a lot of copies of things to ignore in there (I filtered a few before I stopped bothering to figure out patterns to exclude in the query) but spot checking some larger projects to see how they use loaders makes sense.
(I wish PyPI data were available in BigQuery so I could've filtered the above on relevance instead of also seeing everybody's own copies of other projects code in the results... I don't know who created https://bigquery.cloud.google.com/dataset/the-psf:pypi but it only appears to have some form of download stats data, not package metadata that could be cross-referenced to a github user/project repo_name)
jaraco commentedon Oct 21, 2020
In GitLab by @ewdurbin on Jan 3, 2018, 10:28
@gpshead the linked dataset is the official dataset for downloads. and at this point it is only that.
currently it is fed by https://github.com/pypa/linehaul which receives and parses our CDN's access logs.
package metadata would be a very good addition to our BigQuery dataset, but the work just has not been done.
jaraco commentedon Oct 21, 2020
In GitLab by @Yhg1s on Jan 3, 2018, 10:37
With regards to opensourcing elfzipimport (our zipimport rewrite), it's on my TODO list but I can't say much more than that... I have a lot of stuff on my TODO list, and so far the benefit to others hasn't been that great: it avoids various issues (like the ZIP file changing during the Python process's lifetime), has ZIP64 support and can load extension modules without unpacking if you have a modified libc (https://sourceware.org/bugzilla/show_bug.cgi?id=11767). The downside is that the API it offers isn't 100% compatible; it tries to be, but some parts of the zipimport API as tested by test_zipimport are just pretty insane. Serious discussion would have to be had on whether that's warranted breakage in 3.7/3.8.
jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Jan 3, 2018, 10:45
There's also the lazyweb search API
jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Jan 3, 2018, 11:38
@Yhg1s it sounds like elfzipimport won't be ready for 3.7 in any case. It does sound useful for 3.8 ; it's written in C though right, so it's not compatible with a possible desired rewrite of zipimport in Python. I'm cool with that if it's fast :). And it sounds like you could implement
ResourceReadersince you have a unique loader-per-package.Not every loader has to implement
ResourceReader, butimportlib.resourceswon't have fallbacks for such loaders, so you simply wouldn't be able to load resources from them, unless we change the API or those 3rd party import hooks were to change too.Maybe that's okay, and maybe it's okay for us to change built-in zipimporter to use the shim loaders as @brettcannon suggests. We'd have to document this limitation of course.
The other option perhaps is to provide hooks in
importlib.resourcesthat third party loaders could use, but that seems like more complexity for little value (i.e. why not just do whatever you have to do to implementResourceReaderafter all?).So it comes down to two questions I think:
ResourceReaderABC require the package as an argument?jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Feb 15, 2018, 09:23
We solved this through Nick's
get_resource_reader()idea. It's already implemented this way in Python 3.7.jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Feb 15, 2018, 09:23
closed
jaraco commentedon Oct 21, 2020
In GitLab by @warsaw on Feb 17, 2018, 22:04
removed milestone