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

AttributeError with import-mode importlib and doctest modules #12194

Closed
jaraco opened this issue Apr 8, 2024 · 7 comments · Fixed by #12208
Closed

AttributeError with import-mode importlib and doctest modules #12194

jaraco opened this issue Apr 8, 2024 · 7 comments · Fixed by #12208
Assignees
Labels
topic: collection related to the collection phase

Comments

@jaraco
Copy link
Contributor

jaraco commented Apr 8, 2024

Likely related to #12112, discovered in jaraco/keyring#676, a new regression appears in pytest 8.1 relating to collection. Consider this minimal example:

 draft @ cat > test_something.py
import foo
from foo.bar import baz
foo.bar.baz
 draft @ mkdir -p foo/bar/baz
 draft @ touch foo/__init__.py
 draft @ touch foo/bar/__init__.py
 draft @ touch foo/bar/baz/__init__.py
 draft @ pip-run pytest -- -m pytest --import-mode importlib --doctest-modules
============================================================== test session starts ===============================================================
platform darwin -- Python 3.12.2, pytest-8.1.1, pluggy-1.4.0
rootdir: /Users/jaraco/draft
collected 0 items / 1 error                                                                                                                      

===================================================================== ERRORS =====================================================================
_______________________________________________________ ERROR collecting test_something.py _______________________________________________________
test_something.py:3: in <module>
    foo.bar.baz
E   AttributeError: module 'foo' has no attribute 'bar'
============================================================ short test summary info =============================================================
ERROR test_something.py - AttributeError: module 'foo' has no attribute 'bar'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================================ 1 error in 0.03s ================================================================

It seems that once the foo/bar/__init__.py has been collected for doctesting, the collection of the tests in test_something will start to fail when foo has no attribute bar. Any of the following tweaks will bypass the failure:

  • disable doctests
  • pin to pytest<8.1
  • use a different import-mode

Something about the pytest discovery and import machinery is breaking the Python convention that importing a submodule causes the submodule to be added as an attribute of the parent module.

@jaraco
Copy link
Contributor Author

jaraco commented Apr 8, 2024

It occurred to me that the fix in #12169 might also address this issue, but it does not:

 draft @ pip-run git+https://github.com/pytest-dev/pytest@refs/pull/12169/head -- -m pytest --import-mode importlib --doctest-modules
  WARNING: Did not find branch or tag 'refs/pull/12169/head', assuming revision or ref.
============================================================== test session starts ===============================================================
platform darwin -- Python 3.12.2, pytest-8.2.0.dev95+g1e22e0f70, pluggy-1.4.0
rootdir: /Users/jaraco/draft
collected 0 items / 1 error                                                                                                                      

===================================================================== ERRORS =====================================================================
_______________________________________________________ ERROR collecting test_something.py _______________________________________________________
test_something.py:4: in <module>
    foo.bar.baz
E   AttributeError: module 'foo' has no attribute 'bar'
============================================================ short test summary info =============================================================
ERROR test_something.py - AttributeError: module 'foo' has no attribute 'bar'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================================ 1 error in 0.03s ================================================================

@nicoddemus
Copy link
Member

Thanks @jaraco, I can reproduce the issue (also verified that #12169 does not fix this).

Will investigate which incantation is necessary for this to work.

Currently we import modules in --importmode=importlib using this snippet:

if spec is not None:
mod = importlib.util.module_from_spec(spec)
sys.modules[module_name] = mod
spec.loader.exec_module(mod) # type: ignore[union-attr]
if insert_modules:
insert_missing_modules(sys.modules, module_name)
return mod

@nicoddemus nicoddemus self-assigned this Apr 8, 2024
@flying-sheep
Copy link
Contributor

flying-sheep commented Apr 9, 2024

I found an issue that might be related. If not, please direct me in the right direction.

My problem so far is that I haven’t figured out a minimal reproducer yet: Somehow the _frozen_importlib_external.PathFinder finds the a namespace module for our real life example but not in the test suite.

Minimal (not yet) reproducer:
$ mkdir bar
$ mkdir foo
$ touch foo/bar.py
$ tree 
+ foo/
|  + bar.py
+ bar/

Now running a pytest test that imports foo.bar leads to this traceback (simplified, oldest to newest)

mod = import_path('foo/bar.py', mode='importlib', root='.')
→
mod = _import_module_using_spec('foo.bar', 'foo/bar.py', '.')
→
spec = meta_importer.find_spec('foo.bar', ['.'])

where meta_importer is a _frozen_importlib_external.PathFinder instance, and the module created from the spec returned by the last expression is

>>> importlib.util.module_from_spec(spec)
<module 'foo.bar' (namespace) from ['./bar']>

instead of

>>> importlib.util.module_from_spec(spec)
<module 'foo.bar' from ['./foo/bar.py']>

The documentation of MetaPathFinder.find_spec says:

If this is a top-level import, path will be None.

and since _import_module_using_spec says it “imports a module by its canonical name”, that means path should be none and you should call

meta_importer.find_spec('foo.bar', None)

right?

@jaraco
Copy link
Contributor Author

jaraco commented Apr 10, 2024

Thanks for the tip. I did find that adding this patch works around the issue (at least in the repro):

diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py
index 254d9d946..bb26a433f 100644
--- a/src/_pytest/pathlib.py
+++ b/src/_pytest/pathlib.py
@@ -638,6 +638,7 @@ def _import_module_using_spec(
         assert spec is not None
         mod = importlib.util.module_from_spec(spec)
         sys.modules[module_name] = mod
+        _set_name_in_parent(mod)
         spec.loader.exec_module(mod)  # type: ignore[union-attr]
         if insert_modules:
             insert_missing_modules(sys.modules, module_name)
@@ -646,6 +647,13 @@ def _import_module_using_spec(
     return None
 
 
+def _set_name_in_parent(mod):
+    parent, sep, name = mod.__name__.rpartition(".")
+    if not sep:
+        return
+    setattr(sys.modules[parent], name, mod)
+
+
 def spec_matches_module_path(
     module_spec: Optional[ModuleSpec], module_path: Path
 ) -> bool:

I wonder if there's similar logic in CPython's importlib logic that should be modeled.

@jaraco
Copy link
Contributor Author

jaraco commented Apr 10, 2024

Here's where that happens in CPython. Doesn't seem to be particularly re-usable, but at least shows how that behavior is implemented there.

@flying-sheep
Copy link
Contributor

My issue is fixed with #12169, but I found that the change in #12207 avoids the wrong loader altogether.

@jaraco does that one fix your problem?

@jaraco
Copy link
Contributor Author

jaraco commented Apr 13, 2024

My issue is fixed with #12169, but I found that the change in #12207 avoids the wrong loader altogether.

@jaraco does that one fix your problem?

Just checked and it does not.

jaraco added a commit to jaraco/skeleton that referenced this issue Apr 16, 2024
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Apr 20, 2024
nicoddemus added a commit that referenced this issue Apr 20, 2024
Now `importlib` mode will correctly set the imported modules as an attribute of their parent modules.

As helpfully posted on #12194, that's how the Python import module works so we should follow suit.

In addition, we also try to import the parent modules as part of the process of importing a child module, again mirroring how Python importing works.

Fix #12194
clrpackages pushed a commit to clearlinux-pkgs/pypi-jaraco.functools that referenced this issue Apr 22, 2024
…0 to version 4.0.1

Avasam (1):
      Allow mypy on PyPy (jaraco/skeleton#111)

Bartosz Sławecki (3):
      Tweak coverage configuration for type checking (jaraco/skeleton#97)
      Add link to blog entry from jaraco/skeleton#115 above CI build matrix.
      Move project metadata to `pyproject.toml` (jaraco/skeleton#122)

Christian Clauss (2):
      Upgrade GitHub Actions checkout (jaraco/skeleton#94)
      GitHub Actions: Combine tox jobs diffcov and docs (jaraco/skeleton#95)

Dimitri Papadopoulos Orfanos (2):
      Use the ruff formatter (jaraco/skeleton#99)
      ruff: extended-ignore → ignore (jaraco/skeleton#105)

Jason R. Coombs (24):
      Limit sphinxlint jobs to 1. Workaround for sphinx-contrib/sphinx-lint#83.
      Remove news fragment after allowing time to be processed downstream.
      Suppress deprecation warning in dateutil. Workaround for dateutil/dateutil#1284.
      Update Github Actions badge per actions/starter-workflows#1525.
      Separate collateral jobs on different lines for easier override/extension.
      Drop minimum requirement on pytest-mypy as most environments are already running much later. Closes jaraco/skeleton#96.
      Remove sole entry for branches-ignore. Workaround for and closes jaraco/skeleton#103.
      Bump year on badge
      Remove build and dist from excludes. It appears they are not needed and their presence blocks the names of packages like 'builder' and 'distutils'. Ref pypa/distutils#224.
      Exclude docs and tests directories properly per Setuptools behavior.
      Rely on default discovery for good heuristics for finding packages.
      Enable preview to enable preserving quotes.
      Use latest versions in RTD boilerplate.
      Remove Sphinx pin. Ref sphinx-doc/sphinx#11662.
      Include deps from the base config in diffcov.
      Enable complexity check and pycodestyle warnings. Closes jaraco/skeleton#110.
      Use 'extend-select' to avoid disabling the default config. Ref jaraco/skeleton#110.
      Re-enable ignoring of temporary merge queue branches. Closes jaraco/skeleton#103.
      Fetch unshallow clones in readthedocs. Closes jaraco/skeleton#114.
      Move Python 3.11 out of the test matrix.
      Configure pytest to support namespace packages. Ref pytest-dev/pytest#12112.
      Pin against pytest 8.1.x due to pytest-dev/pytest#12194.
      Migrated config to pyproject.toml using jaraco.develop.migrate-config and ini2toml.
      Finalize

Sviatoslav Sydorenko (1):
      Enable testing merge queues @ GitHub Actions CI/CD (jaraco/skeleton#93)

bswck (1):
      Remove deprecated `call_aside` from __init__.pyi
clrpackages pushed a commit to clearlinux-pkgs/pypi-inflect that referenced this issue Apr 26, 2024
…ion 7.2.1

Bartosz Sławecki (2):
      Add link to blog entry from jaraco/skeleton#115 above CI build matrix.
      Move project metadata to `pyproject.toml` (jaraco/skeleton#122)

Jason R. Coombs (25):
      Configure pytest to support namespace packages. Ref pytest-dev/pytest#12112.
      Pin against pytest 8.1.x due to pytest-dev/pytest#12194.
      Migrated config to pyproject.toml using jaraco.develop.migrate-config and ini2toml.
      Extract _handle_chunk function, one small step toward simplification.
      Simplify a bit by using booleans instead of ints.
      Extract _sub_ord function.
      Re-use _sub_ord where the same pattern appears.
      Remove unnecessary variable and type assignment
      Implemented _sub_ord as one regex operation.
      Prefer expression algebra
      Remove comment that's redundant to the docstring.
      Extract _chunk_num and _remove_last_blank functions.
      Avoid repetition in call and assignment and vary on the parameter.
      Extract function for _get_sign
      Prefer None for tri-state variable
      Remove remnant comment
      Refactor signout handling to consolidate some behavior and limit interacting branches.
      Re-write first as a single assignment of a boolean expression.
      Extract _render method for rendering the chunks.
      Simplify logic by yielding the comma separately.
      Consolidate returns across group and non-group.
      Reformat
      Add news fragment.
      Finalize
      Restore Python 3.8 compatibility in annotations.
clrpackages pushed a commit to clearlinux-pkgs/pypi-keyring that referenced this issue Apr 30, 2024
…sion 25.2.0

BakerNet (9):
      add getcreds to cli for interface to get_credentials
      Syntax
      ruff
      fix _check_args
      format
      switch to --get-mode instead of separate operation
      Add output format argument for get operation
      typos
      change --get-mode to --mode

Bartosz Sławecki (2):
      Add link to blog entry from jaraco/skeleton#115 above CI build matrix.
      Move project metadata to `pyproject.toml` (jaraco/skeleton#122)

Jason R. Coombs (11):
      Pin against pytest 8.1.x due to pytest-dev/pytest#12194.
      Migrated config to pyproject.toml using jaraco.develop.migrate-config and ini2toml.
      Allow macos on Python 3.8 to fail as GitHub CI has dropped support.
      Move project.urls to appear in the order that ini2toml generates it. Remove project.scripts.
      Extract methods for getting the credential or password.
      Extract methods for emitting a credential.
      Re-write do_get to re-use credential objects.
      Move checks for 'no result' back into do_get.
      Rewrite _check_args using rules for required params. Consolidates logic and reduces cyclomatic complexity to 2.
      Add news fragment.
      Finalize
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: collection related to the collection phase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants