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

Do not import duplicated modules with --importmode=importlib #12074

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

nicoddemus
Copy link
Member

Regression brought up by #11475.

@nicoddemus
Copy link
Member Author

@flying-sheep I could not reproduce the issue using your reproducer, in 7.4.4, 8.1.0 and this branch I get the same error:

======================================================= ERRORS ========================================================
________________________________ ERROR collecting scanpy/_utils/compute/is_constant.py ________________________________
ImportError while importing test module 'c:\Users\bruno\projects\scanpy\scanpy\_utils\compute\is_constant.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
scanpy\_utils\compute\is_constant.py:12: in <module>
    from ..._compat import DaskArray
.env\lib\site-packages\scanpy\__init__.py:19: in <module>
    from ._utils import check_versions
.env\lib\site-packages\scanpy\_utils\__init__.py:33: in <module>
    from .compute.is_constant import is_constant  # noqa: F401
E   ImportError: cannot import name 'is_constant' from 'scanpy._utils.compute.is_constant' (c:\Users\bruno\projects\scanpy\scanpy\_utils\compute\is_constant.py)

Perhaps this is due to me running Windows? Can you try this branch yourself perhaps?

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of whether it fixes @flying-sheep's issue, the change LGTM

@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Mar 4, 2024
@flying-sheep
Copy link
Contributor

flying-sheep commented Mar 4, 2024

Ah, sorry, the reproducer didn’t do what I thought it did.

This one works. I’ll check if this PR fixes things:

git clone https://github.com/scverse/anndata.git -b src2
cd anndata
python -m venv .venv
.venv/bin/pip install .[test] pytest@https://github.com/pytest-dev/pytest/releases/download/8.1.0/pytest-8.1.0-py3-none-any.whl
.venv/bin/pytest anndata src/anndata/tests/test_awkward.py::test_view src/anndata/tests/test_backed_sparse.py::test_empty_backed_indexing
FAILED src/anndata/tests/test_awkward.py::test_view[obsm] - Failed: DID NOT WARN. No warnings of type (<class 'anndata._warnings.ImplicitModificationWarning'>,) were emitted.
FAILED src/anndata/tests/test_awkward.py::test_view[varm] - Failed: DID NOT WARN. No warnings of type (<class 'anndata._warnings.ImplicitModificationWarning'>,) were emitted.
ERROR src/anndata/tests/test_backed_sparse.py::test_empty_backed_indexing[zarr-empty_list] - ImportError: cannot import name 'write_zarr' from 'anndata._io.zarr' (/home/phil/Dev/Python/_reproducers/anndata/.venv/lib/python3.11/site-packages/zarr/__init__.py)
ERROR src/anndata/tests/test_backed_sparse.py::test_empty_backed_indexing[zarr-empty_bool_mask] - ImportError: cannot import name 'write_zarr' from 'anndata._io.zarr' (/home/phil/Dev/Python/_reproducers/anndata/.venv/lib/python3.11/site-packages/zarr/__init__.py)

@flying-sheep
Copy link
Contributor

flying-sheep commented Mar 4, 2024

OK, I tried

git clone https://github.com/scverse/anndata.git -b src2
cd anndata
python -m venv .venv
.venv/bin/pip install .[test] 'pytest@git+https://github.com/nicoddemus/pytest.git@11475-duplicated-imports'
.venv/bin/pytest anndata src/anndata/tests/test_awkward.py::test_view src/anndata/tests/test_backed_sparse.py::test_empty_backed_indexing

and it worked!

/edit: running all tests via .venv/bin/pytest also worked, so this PR probably covers all cases.

🥳

@nicoddemus
Copy link
Member Author

Awesome, thanks @flying-sheep!

@nicoddemus nicoddemus merged commit 03e5471 into pytest-dev:main Mar 4, 2024
25 checks passed
@nicoddemus nicoddemus added backport 8.1.x and removed needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch labels Mar 4, 2024
@nicoddemus nicoddemus deleted the 11475-duplicated-imports branch March 4, 2024 15:45
@ksunden ksunden mentioned this pull request Mar 4, 2024
5 tasks
@aaraney
Copy link

aaraney commented Mar 11, 2024

I think this may have introduced a different regression. Consider the following project structure with two packages under the ns namespace:

ns
└── python
    ├── bar
    │   ├── ns
    │   │   ├── bar
    │   │   └── test
    │   │       ├── __init__.py
    │   │       ├── bar.py
    │   │       └── test_bar.py
    │   └── pyproject.toml
    └── foo
        ├── ns
        │   ├── foo
        │   └── test
        │       ├── __init__.py
        │       ├── foo.py
        │       └── test_foo.py
        └── pyproject.toml

In pytest==8.0.2, python -m pytest --import-mode=importlib correctly runs the tests from the top level ns directory. In pytest==8.1.1, python -m pytest --import-mode=importlib -o "consider_namespace_packaes=true", results in the following error:

========================== test session starts ===========================
platform darwin -- Python 3.9.16, pytest-8.1.1, pluggy-1.4.0
rootdir: /home/user/pytest-12074
collected 0 items / 2 errors

================================= ERRORS =================================
____________ ERROR collecting python/bar/ns/test/test_bar.py _____________
ImportError while importing test module '/home/user/pytest-12074/python/bar/ns/test/test_bar.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
python/bar/ns/test/test_bar.py:1: in <module>
    from .bar import value
E   ModuleNotFoundError: No module named 'test.bar'
____________ ERROR collecting python/foo/ns/test/test_foo.py _____________
ImportError while importing test module '/home/user/pytest-12074/python/foo/ns/test/test_foo.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
python/foo/ns/test/test_foo.py:1: in <module>
    from .foo import value
E   ModuleNotFoundError: No module named 'test.foo'
======================== short test summary info =========================
ERROR python/bar/ns/test/test_bar.py
ERROR python/foo/ns/test/test_foo.py
!!!!!!!!!!!!!!!! Interrupted: 2 errors during collection !!!!!!!!!!!!!!!!!
=========================== 2 errors in 0.04s ============================

@aaraney
Copy link

aaraney commented Mar 12, 2024

Just a little bit of a follow up, because I realize that my original comment wasn't the most clear. Below are the contents of test_foo.py and foo.py. test_bar.py and bar.py look nearly identical.

# python/foo/ns/test/test_foo.py
from .foo import value

def test_foo():
    assert value == "foo"
# python/foo/ns/test/foo.py
value = "foo"

@nicoddemus
Copy link
Member Author

@aaraney thanks for the report, but could you open a new issue please (you can refer to this one there)?

I will take a look at it ASAP. 👍

@aaraney
Copy link

aaraney commented Mar 12, 2024

@nicoddemus, for sure! Thanks for taking a look at this!

@tacaswell
Copy link

This appears to have broken assertion re-writing with editable installs.

@tacaswell
Copy link

This is fixed by #12313

@@ -539,6 +539,10 @@ def import_path(
except CouldNotResolvePathError:
pass
else:
# If the given module name is already in sys.modules, do not import it again.
with contextlib.suppress(KeyError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just taking a look again, i believe this should trigger a import mismatch if the file of the loaded module and the current path are different

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants