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

Plugin tests break on python 3.10 because of importlib and pytest assertion rewriting #1854

Closed
martenlienen opened this issue Oct 14, 2021 · 10 comments
Labels
bug Something isn't working

Comments

@martenlienen
Copy link
Contributor

🐛 Bug

Description

pytest's assertion rewriting replaces the loader in a module's __spec__ for modules with rewriting enabled. This loader does not implement get_resource_reader which importlib.resources.is_resource uses internally. For some reason, hydra.test_utils gets registered for assertion rewriting which means that hydra.test_utils.configs is not recognized as a resource and cannot be loaded as a config source by hydra.

The tests can be made to pass by disabling assertion rewriting with pytest --assert=plain or putting it in pyproject.toml.

[tool.pytest.ini_options]
addopts = "--assert=plain"

To reproduce

This can reproduced in the example sweeper plugin.

$ cd examples/plugins/example_sweeper_plugin
$ pip install -e .
$ pytest

** Stack trace/error message **

# ...
>       raise MissingConfigException(
            missing_cfg_file=config_name, message=add_search_path()
        )
E       hydra.errors.MissingConfigException: Primary config module 'hydra.test_utils.configs' not found.
E       Check that it's correct and contains an __init__.py file

~/.pyenv/versions/3.10.0/envs/hydra/lib/python3.10/site-packages/hydra/_internal/config_loader_impl.py:103: MissingConfigException
# ....

Expected Behavior

The tests should succeed.

System information

  • Hydra Version : master
  • Python version : 3.10
  • Virtual environment type and version : none
  • Operating system : Ubuntu 21.04
@martenlienen martenlienen added the bug Something isn't working label Oct 14, 2021
@jieru-hu
Copy link
Contributor

Thanks @martenlienen. Hydra does not officially support python 3.10 yet. I've created #1856 to track this.

@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 14, 2021

Maybe hydra.test_utils is getting targeted for assertion re-writing because of it's name.
If so, we could consider renaming -> hydra.testing_utils so as to avoid monkeying with the pytest options.

@omry
Copy link
Collaborator

omry commented Oct 14, 2021

Hydra is registering a pytest plugin in setup.py (entry_points={"pytest11": ["hydra_pytest = hydra.extra.pytest_plugin"]}). Maybe it has some kind of interaction here?
I think we should avoid disabling assertion rewriting if possible.

@omry
Copy link
Collaborator

omry commented Oct 14, 2021

From the issue description, it sounds like a regression in pytest. Is there an issue there?

@martenlienen
Copy link
Contributor Author

There is pytest-dev/pytest#2506 which reports a problem with pkgutil. Looking at the source, the root cause for both pkgutil and importlib is the same however. Seeing how that issue has been open for 4 years and the pytest docs say

However, if you are working with the import machinery yourself, the import hook may interfere.
If this is the case you have two options:

  • Disable rewriting for a specific module by adding the string PYTEST_DONT_REWRITE to its docstring.
  • Disable rewriting for all modules by using --assert=plain.

there might not be an easy fix.

@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 5, 2021

It's not just hydra.test_utils.configs that's being processed by pytest's assertion-rewriting logic; it's all of hydra:

import hydra
from _pytest.assertion.rewrite import AssertionRewritingHook

def test_rewriting():
    assert isinstance(hydra.__loader__, AssertionRewritingHook)
    # This test passes if you run it with pytest.
    # It fails if you run it manually by e.g. importing `test_rewriting` and calling it from a REPL.

If you modify our setup.py file by deleting the hydra_pytest entrypoint, Hydra is no longer re-written by pytest's assertion logic (i.e. the above test fails, and instead isinstance(hydra.__loader__, importlib.machinery.SourceFileLoader) is True). This is the case both for python3.10 and for earlier versions of python.

So what changed between python3.9 and python3.10? Hydra's ImportlibResourcesConfigSource class uses the importlib.resources.files API to decide whether a config source is available. In python3.10, with recent versions of pytest, this API fails when used with modules that have been assertion-rewritten.

The folks at pytest-dev have merged a PR that should resolve this issue (once the next version of pytest is released).

As an alternative to waiting for the next pytest version, we could try to restructure our code so that other parts of hydra (besides the hydra.extras.pytest_plugin module) are not subject to pytest's assertion rewriting.

@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 5, 2021

Thanks again @martenlienen

@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 10, 2021

Per offline discussion with Hydra team, we'll wait until the next version of pytest is released incorporating PR pytest-dev/pytest#9173, which should resolve this issue.

@Jasha10 Jasha10 added the blocked Can't make progress label Nov 10, 2021
@Jasha10 Jasha10 removed the blocked Can't make progress label Feb 22, 2022
@johnnynunez
Copy link

any new news about this?

@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 24, 2022

Hydra now uses pytest version 7 in our CI, which means that Hydra+python3.10+pytest7 should all work together.
As for support of older pytest versions (pytest<7), this issue still applies.

We plan to support Python3.10 in the upcoming Hydra1.2 release (which is scheduled for mid-April).

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

No branches or pull requests

5 participants