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

Loading file in library fails in downstream packages #298

Closed
edgarrmondragon opened this issue Mar 12, 2024 · 20 comments · Fixed by #302
Closed

Loading file in library fails in downstream packages #298

edgarrmondragon opened this issue Mar 12, 2024 · 20 comments · Fixed by #302
Assignees
Labels
bug Something isn't working

Comments

@edgarrmondragon
Copy link

edgarrmondragon commented Mar 12, 2024

Hey 👋

Starting with release 6.2.0, downstream packages dependent on a library that uses the equivalent of

import importlib_resources
import yaml

path = importlib_resources.files("my_lib").joinpath("default_logging.yml")

with path.open() as f:
    return yaml.safe_load(f)

fail with FileNotFoundError: Can't open orphan path.

This seems to only happen in Python 3.8 and 3.9.

For reference, meltano/sdk#2310 is a PR where I'm trying to fix things on my end in case I was using your APIs incorrectly, but still a breaking change might have been inadvertently introduced.

@potiuk
Copy link

potiuk commented Mar 13, 2024

Seems duplicate of #299

@potiuk
Copy link

potiuk commented Mar 13, 2024

(I've seen same error message when pytest_rewrite does it's job)

@jaraco
Copy link
Member

jaraco commented Mar 15, 2024

The only change introduced with 6.2.0 was #295.

@Jakub-CZ
Copy link
Contributor

#257 is back since importlib_resources 6.2.0

@jaraco
Copy link
Member

jaraco commented Mar 15, 2024

The issue with jsonschema seems to be masking the underlying error, so it's hard to tell if it's the same FileNotFoundError.

That FileNotFoundError comes from the CompatibilityFiles adapter, which shouldn't be reached in the case of a legitimate resource provider:

def open(self, mode='r', *args, **kwargs):
raise FileNotFoundError("Can't open orphan path")

Unfortunately, I've not yet been able to replicate the issue, even when using pytest:

 draft @ cat 298-repro.py
import sys

import importlib_resources as rs

assert sys.version_info < (3, 10)
__requires__ = ["importlib_resources>=6.2", "jaraco.text", "pytest"]


def test_open():
    path = rs.files("jaraco.text").joinpath("Lorem Ipsum.txt")
    with path.open() as f:
        f.read()
 draft @ py -3.9 -m pip-run -- -m pytest 298-repro.py
============================================================== test session starts ===============================================================
platform darwin -- Python 3.9.18, pytest-8.1.1, pluggy-1.4.0
rootdir: /Users/jaraco/draft
collected 1 item                                                                                                                                 

298-repro.py .                                                                                                                             [100%]

=============================================================== 1 passed in 0.01s ================================================================

Can anyone help devise a reproducer that I can run, preferably something minimal and including importlib_resources in the traceback? I need help reproducing what it is about the failing environments that leads to the CompatibiltyFiles / OrphanPath objects. Maybe something that minimally uses jsonschema might be helpful.

@jaraco
Copy link
Member

jaraco commented Mar 15, 2024

I thought I'd try to ascertain where jsonschema is going wrong in the handling of resources, but the only mention of importlib_resources in the project is in the pyproject.toml.

@jaraco
Copy link
Member

jaraco commented Mar 15, 2024

I could revert the change introduced in 6.2.0, but at this stage, it looks like a lot of the issues have already been worked around by pinning importlib_resources, so I'd like to focus on fixing forward, but do let me know if reverting the change would provide benefit to your project (and how) and I'll consider reverting to limit the damage while investigating a long-term solution.

@edgarrmondragon
Copy link
Author

I've given it a try but I couldn't produce a MRE for this. I've updated our project to restrict importlib-resources to <6.2, but other dependents might not catch up for a while so having a new release with the root cause fixed or reverted would be nice to have.

Also fwiw, at least for the way we use importlib-resources, this bug only seems to be impacting Python 3.8, which represent 10-15% of PyPI downloads according to pypistats.org.

rsokl added a commit to mit-ll-responsible-ai/hydra-zen that referenced this issue Mar 17, 2024
ndevenish added a commit to ndevenish/data that referenced this issue Mar 18, 2024
ndevenish added a commit to ndevenish/data that referenced this issue Mar 18, 2024
@ndevenish
Copy link

We also ran into this with CI error. However, pinning is fine for us, and we can probably migrate things over to the standard library equivalent now anyhow.

ndevenish added a commit to dials/data that referenced this issue Mar 18, 2024
@jaraco
Copy link
Member

jaraco commented Mar 19, 2024

Building on the repro in #299, I'm able to replicate the reported KeyError with this Dockerfile:

FROM jaraco/multipy-tox

RUN git clone https://github.com/apache/airflow
WORKDIR airflow
RUN git checkout 72d19565d84744b59f3d22b32c09ab5fc553b779
RUN py -3.8 -m venv .venv
RUN py -m pip install -e ./dev/breeze
CMD py -m pytest dev/breeze/tests -x

@jaraco
Copy link
Member

jaraco commented Mar 19, 2024

After some troubleshooting, I think I understand the problem better now.

In importlib_resources <6.2, the _compat module would return a zip or namespace or file reader if it appeared to be compatible with the spec:

https://github.com/python/importlib_resources/blob/v6.1.3/importlib_resources/future/adapters.py#L17-L21

When running under pytest with the assert rewrite hook enabled (and presumably some other settings, because I couldn't replicate the issue by simply importing jsonschema in a pytest), the loader for jsonschema_specifications is a _pytest.assertion.rewrite.AssertionRewritingHook, which has no get_resource_reader, so it gets wrapped in a degenerate CompatibilityFiles object that always returns no resources.

While it was intentional (in #297) to expose third-party traversable readers when present, it was not the intention to give the degenerate CompatibilityFiles wrapper precedence for loaders without any implementation.

@jaraco
Copy link
Member

jaraco commented Mar 19, 2024

What I don't yet understand is why doesn't this issue manifest on newer Pythons. Presumably the same AssertionRewritingHook loader exists and would similarly get wrapped in a degenerate CompatibilityFiles.

I created this tox recipe to run in airflow to more easily run tests across different Pythons:

[tox]
package_root = dev/breeze

[testenv]
commands =
	pytest dev/breeze/tests {posargs}
package = editable

@jaraco
Copy link
Member

jaraco commented Mar 19, 2024

What I don't yet understand is why doesn't this issue manifest on newer Pythons.

Aha - It's because pytest's hook implements a resource provider on Python 3.10+, so even though the hook is present, it's deferring to the stdlib readers.

@potiuk
Copy link

potiuk commented Mar 19, 2024

Aha - It's because pytest's hook implements a resource provider on Python 3.10+, so even though the hook is present, it's deferring to the stdlib readers.

Yep. importlib_resources is not even installed in Airflow for 3.10 +

jaraco added a commit that referenced this issue Mar 19, 2024
…aders and instead prefer the standard readers before once again falling back to any CompatibilityFiles reader. Closes #298.
@jaraco
Copy link
Member

jaraco commented Mar 19, 2024

After pushing a proposed fix, I've confirmed the fix has the intended effect with this tox.ini file:

[tox]
package_root = dev/breeze

[testenv]
deps =
	importlib_resources @ git+https://github.com/python/importlib_resources@bugfix/298-fallback-precedence
commands =
	pytest dev/breeze/tests {posargs}
package = editable

Running it in apache/airflow@72d19565d8 with tox -e py38 shows all tests pass.

@jaraco
Copy link
Member

jaraco commented Mar 19, 2024

Releasing as v6.3.2; please report back if it has any unintended consequences or doesn't solve the issue for another use case.

@edgarrmondragon
Copy link
Author

Reporting back that 6.3.2 fixed the issue for us. Thanks!

potiuk added a commit to potiuk/airflow that referenced this issue Mar 19, 2024
The problem with importlib_resources interacting badly with
pytest_rewrite has been solved in 6.3.2 version today and we can
simply skip over the bad versions.

See python/importlib_resources#298
@potiuk
Copy link

potiuk commented Mar 19, 2024

Running the PR on Airflow apache/airflow#38294

potiuk added a commit to apache/airflow that referenced this issue Mar 19, 2024
The problem with importlib_resources interacting badly with
pytest_rewrite has been solved in 6.3.2 version today and we can
simply skip over the bad versions.

See python/importlib_resources#298
@potiuk
Copy link

potiuk commented Mar 19, 2024

Confirmed! Works!

jedcunningham pushed a commit to apache/airflow that referenced this issue Mar 19, 2024
The problem with importlib_resources interacting badly with
pytest_rewrite has been solved in 6.3.2 version today and we can
simply skip over the bad versions.

See python/importlib_resources#298

(cherry picked from commit 2cb4209)
@potiuk
Copy link

potiuk commented Mar 20, 2024

Thanks for quick handling It @jaraco !

rouault added a commit to rouault/gdal that referenced this issue Apr 6, 2024
…sue with pytest with jsonschema"

This reverts commit 6703d30.

According to python/importlib_resources#298,
the issue has been fixed in importlib-resources 6.3.2
rouault added a commit to rouault/gdal that referenced this issue Apr 6, 2024
…sue with pytest with jsonschema"

This reverts commit 6703d30.

According to python/importlib_resources#298,
the issue has been fixed in importlib-resources 6.3.2
utkarsharma2 pushed a commit to astronomer/airflow that referenced this issue Apr 22, 2024
The problem with importlib_resources interacting badly with
pytest_rewrite has been solved in 6.3.2 version today and we can
simply skip over the bad versions.

See python/importlib_resources#298
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.

5 participants