Issue42129
Created on 2020-10-23 16:03 by jaraco, last changed 2021-03-22 09:01 by vstinner. This issue is now closed.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 24670 | merged | jaraco, 2021-02-28 19:31 |
Messages (12) | |||
---|---|---|---|
msg379451 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2020-10-23 16:03 | |
In [importlib_resources#68](https://github.com/python/importlib_resources/issues/68), the project identified a deficiency with respect to pkg_resources for resources in namespace packages. The project has since merged support for these packages, slated to go out with the importlib_resources 3.2 release. This issue is to track the incorporation of those changes into importlib.resources. |
|||
msg384770 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2021-01-10 17:47 | |
In [this commit](https://github.com/python/importlib_resources/commit/bd20d893f11f387d285c666bc99fd2d4a7c33ef8), I've reconciled and merged the changes from importlib_resources 3.2-5.0, mainly the namespace package support (https://importlib-resources.readthedocs.io/en/latest/history.html#v5-0-0). Applying these changes to cpython and running the tests, there appear to be two emergent failure modes: ``` cpython master $ http --follow https://github.com/python/importlib_resources/archive/cpython.zip | bsdtar --strip-components 1 -x $ ./python.exe Tools/scripts/run_tests.py test.test_importlib 2>&1 | gist -Pc https://gist.github.com/dde7d5a951d92726380dced21503f843 ``` In my estimation, the first two failures are due to the change in the abc.ResourceReader for contents to raise FileNotFoundError. And the latter failures are seemingly due to the built-in NamespaceLoader not having a resource reader. I suspect the fix here is to add a .get_resource_reader() method to _NamespaceLoader to return a NamespaceReader. (hint: when updating _bootstrap_external.py, you'll need to run `make regen-importlib` or `make regen-all` and recompile). Filipe, would you be interested in exploring these issues and propose some fixes and prepare the PR that applies the changes to CPython? |
|||
msg384776 - (view) | Author: Filipe LaĆns (FFY00) * | Date: 2021-01-10 20:13 | |
Yes, I will look into it. Do we have a time schedule for when this should be ready, so that I can organize myself? |
|||
msg384781 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2021-01-10 21:45 | |
Thanks! No rush, but ideally soon enough to be merged before the beta release of Python 3.10 (2021-05-03). |
|||
msg386958 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2021-02-14 18:01 | |
For the first two errors, the issue seems to be that CPython includes tests for the ResourceReader ABC and asserts that .contents() returns an empty list as the default degenerate behavior (https://github.com/python/cpython/blob/1b57426e3a7842b4e6f9fc13ffb657c78e5443d4/Lib/test/test_importlib/test_abc.py#L340-L341). It appears that the ABC in importlib_resources has for a [very long time raised FileNotFound](https://github.com/python/importlib_resources/commit/e82b5675b9fef7dd971b796136687f915f7a39ca). Oh, this is interesting - these ABCs have diverged since their inception; here's where [both the empty list behavior and test were introduced in cpython](https://github.com/python/cpython/commit/4ac5150e068a3a795ef00465f6dff51747b62b91) and [later amended to return an iterable instead of iterator](https://github.com/python/cpython/commit/3ab9365dca8438f89b2060cd3eebe00606133dc4). Brett, is this a divergence worth maintaining? Do you recall if there was a reason for changing the ABC during the port to CPython? There's no mention of 'contents' in issue32248. My temptation is to go with the implementation as found in importlib_resources as it presents a more consistent interface for ResourceReaders (and explicitly defaults to raising FileNotFoundError when no implementation for contents is present) and thus to change the test to: ``` cpython master $ git diff Lib/test/test_importlib/test_abc.py diff --git a/Lib/test/test_importlib/test_abc.py b/Lib/test/test_importlib/test_abc.py index d8b9fc89f2..d1c89c183b 100644 --- a/Lib/test/test_importlib/test_abc.py +++ b/Lib/test/test_importlib/test_abc.py @@ -338,7 +338,9 @@ def test_is_resource(self): self.ins.is_resource('dummy_file') def test_contents(self): - self.assertEqual([], list(self.ins.contents())) + with self.assertRaises(FileNotFoundError): + self.ins.contents() + (Frozen_RRDefaultTests, Source_RRDefaultsTests ``` Brett, let me know if you have concern about harmonizing the two implementations around this behavior and if you have a strong preference for harmonizing toward the CPython implementation instead. Thanks. |
|||
msg387197 - (view) | Author: Brett Cannon (brett.cannon) * ![]() |
Date: 2021-02-18 00:24 | |
I would harmonize towards what the concrete implementations are doing since people who don't implement the defaults will get the appropriate results regardless. An entry in What's New will cover any potential notification of the change since you can't exactly deprecate this sort of thing. |
|||
msg387805 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2021-02-28 09:18 | |
> I would harmonize towards what the concrete implementations... For FileReader and ZipReader, both rely on TraversableResources to implement contents, which rely on `files().iterdir()`, which could raise FileNotFoundError and definitely don't return lists. Most importantly, this method is an abstract method, so subclasses can't rely on it (must override it). Searching Github, I couldn't find any other classes subclassing this ABC. |
|||
msg387808 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2021-02-28 10:11 | |
I've pushed [this branch](https://github.com/python/cpython/tree/feature/42129-resources-namespace-packages), which includes fixes for the above two identified issues. Still one issue remains: ERROR: test_package_has_no_reader_fallback (test.test_importlib.test_resource.ResourceCornerCaseTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/jaraco/code/public/cpython/Lib/test/test_importlib/test_resource.py", line 98, in test_package_has_no_reader_fallback self.assertFalse(resources.is_resource(module, 'A')) File "/Users/jaraco/code/public/cpython/Lib/importlib/resources.py", line 157, in is_resource package_contents = set(contents(package)) File "/Users/jaraco/code/public/cpython/Lib/importlib/resources.py", line 174, in contents transversable = _common.from_package(package) File "/Users/jaraco/code/public/cpython/Lib/importlib/_common.py", line 75, in from_package reader = spec.loader.get_resource_reader(spec.name) AttributeError: 'object' object has no attribute 'get_resource_reader' This same test passes on importlib_resources, and the difference seems to be rooted in how [from_package resolves the package spec using a compatibility wrapper](https://github.com/python/importlib_resources/blob/1401cc48e5077088036aa7e729c8995ffbbb9e88/importlib_resources/_common.py#L76). On the backport, this causes the package without a resource reader to have a resource reader and return a degenerate value: ``` > /Users/jaraco/code/public/importlib_resources/importlib_resources/_py3.py(139)is_resource() -> package_contents = set(contents(package)) (Pdb) _common.from_package(package) PosixPath('/path/which/shall/not/be') (Pdb) from . import _compat (Pdb) _compat.package_spec(package).loader.get_resource_reader('any').files().is_dir() False ``` This means that the compatibility shim in from_package is masking test failure in the backport, and probably the best course of action will be to unmask that failure in the backport and figure out the best behavior there. |
|||
msg388121 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
Date: 2021-03-04 18:43 | |
New changeset 67148254146948041a77d8a2989f41b88cdb2f99 by Jason R. Coombs in branch 'master': bpo-42129: Add support for resources in namespaces (GH-24670) https://github.com/python/cpython/commit/67148254146948041a77d8a2989f41b88cdb2f99 |
|||
msg389163 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2021-03-20 14:59 | |
Please see bpo-43569: "test_importlib failed on installed Python" regression introduced by commit 67148254146948041a77d8a2989f41b88cdb2f99. |
|||
msg389164 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2021-03-20 14:59 | |
I reopen the issue. |
|||
msg389288 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2021-03-22 09:01 | |
> Please see bpo-43569: "test_importlib failed on installed Python" regression introduced by commit 67148254146948041a77d8a2989f41b88cdb2f99. It's now fixed, I close again this issue. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2021-03-22 09:01:32 | vstinner | set | status: open -> closed resolution: fixed messages: + msg389288 |
2021-03-20 14:59:16 | vstinner | set | status: closed -> open resolution: fixed -> (no value) messages: + msg389164 |
2021-03-20 14:59:06 | vstinner | set | nosy:
+ vstinner messages: + msg389163 |
2021-03-04 18:43:43 | jaraco | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
2021-03-04 18:43:09 | jaraco | set | messages: + msg388121 |
2021-02-28 19:31:24 | jaraco | set | keywords:
+ patch stage: patch review pull_requests: + pull_request23456 |
2021-02-28 10:11:19 | jaraco | set | messages: + msg387808 |
2021-02-28 09:18:18 | jaraco | set | messages: + msg387805 |
2021-02-18 00:24:11 | brett.cannon | set | messages: + msg387197 |
2021-02-14 18:01:24 | jaraco | set | nosy:
+ brett.cannon messages: + msg386958 |
2021-01-10 21:45:24 | jaraco | set | messages: + msg384781 |
2021-01-10 20:13:17 | FFY00 | set | messages: + msg384776 |
2021-01-10 17:47:48 | jaraco | set | messages: + msg384770 |
2020-10-23 17:09:06 | barry | set | nosy:
+ barry |
2020-10-23 16:10:25 | FFY00 | set | nosy:
+ FFY00 |
2020-10-23 16:03:26 | jaraco | create |