importlib.readers not valid until python 3.10

[kdelee]

This exists https://github.com/python/cpython/blob/3.10/Lib/importlib/readers.py and FileReader is in there
This is a 404 https://github.com/python/cpython/blob/3.9/Lib/importlib/readers.py

This change needs to get backported to the 7.0.z branch(s) too
closes #9608

I'm going to go ahead and open this PR, if you think we need a changelog for this LMK

[bluetech]

Thanks, this LGTM. Since I have no idea about the underlying importlib stuff, I'll leave it to someone else to actually approve, maybe @gaborbernat who added it (#9173), or @asottile who knows about this stuff.

I do notice that latest python says this about importlib.readers:

https://github.com/python/cpython/blob/bf95ff91f2c1fc5a57190491f9ccdc63458b089e/Lib/importlib/readers.py#L2-L5

Maybe we can be forward-compatible with Python 3.11 already by importing from importlib.resources.readers instead on python>=3.11.

[bluetech]

And if someone knows how to test this that would be great as well (though not necessary for this PR).

[kdelee]

Not surgical, but this test has my reproducer in it. If desired I can push this, or feel free to make edits to the PR

diff --git a/testing/test_compat.py b/testing/test_compat.py
index 88f0f33ef..e5bc5fa64 100644
--- a/testing/test_compat.py
+++ b/testing/test_compat.py
@@ -261,3 +261,16 @@ def test_assert_never_literal() -> None:
         pass
     else:
         assert_never(x)
+
+def test_pytest_get_resource_reader_works_with_pip(pytester: Pytester) -> None:
+    """Assert we dont regress on https://github.com/pytest-dev/pytest/issues/9608"""
+    pytester.makepyfile(
+        """
+        from pip._internal.req import parse_requirements
+
+        def test_import_works():
+            assert True
+    """
+    )
+    result = pytester.runpytest()
+    result.stdout.fnmatch_lines(["*1 passed*"])

[nicoddemus]

How about we move these two imports outside the function? This would at least caught the error in the correct Python versions.

    if sys.version_info >= (3, 10):
        from types import SimpleNamespace
        from importlib.readers import FileReader

        def get_resource_reader(self, name: str) -> importlib.abc.TraversableResources:  # type: ignore
            return FileReader(SimpleNamespace(path=self._rewritten_names[name]))

[nicoddemus]

Hmm nevermind that, that conditional import does not work.

[nicoddemus]

Took the liberty of moving the import to the global level, and adding a CHANGELOG so this is "ready to go", but still would like to hear from someone who knows "importlib" stuff better.

[The-Compiler]

I'm a bit confused by two things here:

• Why didn't the test added in Support the importlib.resources files API in rewritten files #9173 by @gaborbernat catch this?
• Why does the same test not fail now on Python 3.9, given that the code added there is now disabled there, but the test is unaltered?

[asottile]

the test in the original review passes in 3.9 because the implementation of importlib.resources doesn't attempt get_resource_reader and just utilizes the .submodule_search_locations of the module's spec

so the (3, 9) vs. (3, 10) difference is ~mostly harmless -- is there a place where you're seeing this break?

[asottile]

I agree that this should be changed to (3, 10) since FileReader isn't needed there

[kdelee]

Not sure about the others reporting #9608, but this specifically occurs for us in a test file that imports from pip._internal.req import parse_requirements.

from pip._internal.req import parse_requirements

def test_broken():
    assert True

In the full traceback shown in #9608 we see it hitting this in https://github.com/pypa/pip/blob/44018de50cafba25445a225c1a1986d6312e1ef3/src/pip/_vendor/certifi/core.py#L50 which calls this path function in importlib.resources which ends up calling https://github.com/python/cpython/blob/459e26f0987a12a19238baba422e13a8f7fcfca3/Lib/importlib/resources.py#L161 which then checks if there is a attribute get_resource_reader on the "spec.loader" https://github.com/python/cpython/blob/459e26f0987a12a19238baba422e13a8f7fcfca3/Lib/importlib/resources.py#L74 -- which it looks like AssertionRewritingHook is a subclass of importlib.abc.Loader. This class are defines get_resource_loader on on the importlib.abc.Loader subclass and way down in importlib goes ahead and tries to call that function: https://github.com/python/cpython/blob/459e26f0987a12a19238baba422e13a8f7fcfca3/Lib/importlib/resources.py#L76.
This get_resource_loader defined on AssertionRewritingHook then tries to import code that doesn't exist in python 3.9 and everything breaks.

[asottile]

Not sure about the others reporting #9608, but this specifically occurs for us in a test file that imports from pip._internal.req import parse_requirements.

from pip._internal.req import parse_requirements

def test_broken():
    assert True

In the full traceback shown in #9608 we see it hitting this in https://github.com/pypa/pip/blob/44018de50cafba25445a225c1a1986d6312e1ef3/src/pip/_vendor/certifi/core.py#L50 which calls this path function in importlib.resources which ends up calling https://github.com/python/cpython/blob/459e26f0987a12a19238baba422e13a8f7fcfca3/Lib/importlib/resources.py#L161 which then checks if there is a attribute get_resource_reader on the "spec.loader" https://github.com/python/cpython/blob/459e26f0987a12a19238baba422e13a8f7fcfca3/Lib/importlib/resources.py#L74 -- which it looks like AssertionRewritingHook is a subclass of importlib.abc.Loader. This class are defines get_resource_loader on on the importlib.abc.Loader subclass and way down in importlib goes ahead and tries to call that function: https://github.com/python/cpython/blob/459e26f0987a12a19238baba422e13a8f7fcfca3/Lib/importlib/resources.py#L76. This get_resource_loader defined on AssertionRewritingHook then tries to import code that doesn't exist in python 3.9 and everything breaks.

ah yeah this was the whole 3.9.3 fiasco where the resources api was partially rewritten and reverted in a patch version but some of the apis use the readers already

I think if you take the test that was in the previous PR and use open_binary or any of the other resource apis that are not files() then it would demonstrate the failure -- that said, they wouldn't be supported pre-3.9 anyway due the missing reader helpers

so I guess to move forward, I would inline the conditional import and personally I wouldn't make it version-specific but that's up to you

there isn't really a way to test this "regression" because the feature itself doesn't exist in previous versions

[kdelee]

@nicoddemus @bluetech so what would ya'll like to do. It sounds like @asottile would like us to revert the move of the import outside the function.

[bluetech]

@kdelee Can you move the imports back into the function? Then I think we can merge.

[kdelee]

@bluetech done, lmk if anything else is needed

[asottile]

[kdelee]

😬 well not sure what those tests are blowing up about
Tried running them locally and I'm not seeing similar results

[bluetech]

The failures are not due to this PR, it suddenly started to happen, we're not sure why yet. We will rerun the tests once this is resolved.

[bluetech]

(Should have squashed instead of merged - oops)

[pombredanne]

Thanks... 🙇 Do you mind to push a dot release and may be yank 7.0.0 on PyPI as this is making CI fails all over? That would be awesome!

[nicoddemus]

#9654 is up, if no other maintainer would like to include another fix, we can release it. (But doesn't make sense to yank 7.0.0 if we are releasing 7.0.1 right away).

[pombredanne]

You rock 🎸

[matthiasmoke]

Hey, I'm currently writing a code analysis tool for Python projects. It pointed me to this code part, which I find quite interesting. Hence, I would have some questions about it:

1. Where exactly is the get_resource_reader() function called? I can't find any references to it.
2. Why is the function defined by a condition? Would it make sense to move the version check inside the function with an early return? (As there is already a version check within it) As the function is public, it would keep the interface of the AssertionRewritingHook independent to the Python version.

I think these questions may be adressed to @kdelee or @nicoddemus? I would be really happy to hear from you as this would give me some qualitative insights on the reports of my tool.

Kind regards,
Matthias

[asottile]

it's called by the python import machinery itself. this function isn't valid until 3.10 (it breaks 3.9)

[matthiasmoke]

I saw the Github issue on the FileReader import causing the break for 3.9

Before, the import statement in the function was without any version check. But having the check sys.version_info >= (3, 10) within the function with an early return shouldn't break anything as the import statement would never be reached for 3.9.
Or am I overlooking anything why the function would be invalid otherwise?

[asottile]

there's backports of parts of importlib which would reach into this and if the function is defined try and call it (and then error) -- by not defining the function, those backports won't even attempt to call it and therefore won't crash

[matthiasmoke]

Ahh ok I understand. Thanky you very much for your explanation, this helps me a lot!