Skip to content

Comments

Use shortest module name for importlib imports#11931

Closed
flying-sheep wants to merge 7 commits intopytest-dev:mainfrom
flying-sheep:doctest-parent
Closed

Use shortest module name for importlib imports#11931
flying-sheep wants to merge 7 commits intopytest-dev:mainfrom
flying-sheep:doctest-parent

Conversation

@flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Feb 5, 2024

Closes #11475

bluetech
bluetech reviewed Feb 5, 2024
View reviewed changes
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.

I'll let @nicoddemus decide if this makes sense, just some comments on the code.

src/_pytest/pathlib.py Outdated
Comment on lines 616 to 620
candidates = (
_module_name_from_path(path, dir)
for dir in itertools.chain([root], map(Path, sys.path))
)
return ".".join(min(candidates, key=len)) # type: ignore[arg-type]
Copy link
Member

@bluetech bluetech Feb 5, 2024

Choose a reason for hiding this comment

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

This is going to be somewhat slow I reckon. Pathlib is a hog (particularly relative_to) and sys.path can have many entries in some circumstances, and this is running for every import.

Copy link
Contributor Author

@flying-sheep flying-sheep Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I know, I just did the bare functional minimum.

I think this could be improved

  1. maybe by pre-filtering sys.path somehow?
  2. by having a cache that
    1. stores all paths that are ever requested as long as sys.path stays the same,
    2. otherwise the cache gets cleared
  3. using some faster string algorithm for this instead of pathlib.
  4. Maybe there is an alternative to the whole approach, some way to get the correct module name using some API I’m not thinking of

But there’s no alternative to the functionality. The test case should pass in the end.

@nicoddemus
Copy link
Member

nicoddemus commented Feb 5, 2024

Thanks @flying-sheep for the PR.

Sorry, the original issue escaped my radar. I will set aside some time this week to understand the original issue and review this PR.

Thanks and sorry again for the delay on this. 👍

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Feb 6, 2024

No problem, I know how it is, and I’m hesitant to be too annoying with pings, so I hoped this PR might be a productive way to get this going 😄

@nicoddemus nicoddemus mentioned this pull request Feb 17, 2024
Merged
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Feb 17, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
c9ce821 
…chanism

As detailed in pytest-dev#11475 (comment), currently with `--import-mode=importlib` pytest will try to import every file by using a unique module name, regardless if that module could be imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path` (via other mechanism, such as being installed into a virtualenv, PYTHONPATH, etc) would end up being imported as standalone modules, instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c "import anndata.core"` works, but pytest with `importlib` mode would import that module as a standalone module named `".env.lib.site-packages.anndata.core"`, because importlib module was designed to import test files which are not reachable from `sys.path`, but now it is clear that normal modules should be imported using the standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally, without changing `sys.path`, and if that fails it falls back to importing the module as a standalone module.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Feb 18, 2024

Superseded by #11997

nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Feb 18, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
5d90d97 
…chanism

As detailed in pytest-dev#11475 (comment), currently with `--import-mode=importlib` pytest will try to import every file by using a unique module name, regardless if that module could be imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path` (via other mechanism, such as being installed into a virtualenv, PYTHONPATH, etc) would end up being imported as standalone modules, instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c "import anndata.core"` works, but pytest with `importlib` mode would import that module as a standalone module named `".env.lib.site-packages.anndata.core"`, because importlib module was designed to import test files which are not reachable from `sys.path`, but now it is clear that normal modules should be imported using the standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally, without changing `sys.path`, and if that fails it falls back to importing the module as a standalone module.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Feb 25, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
8ceb8fc 
…chanism

As detailed in pytest-dev#11475 (comment), currently with `--import-mode=importlib` pytest will try to import every file by using a unique module name, regardless if that module could be imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path` (via other mechanism, such as being installed into a virtualenv, PYTHONPATH, etc) would end up being imported as standalone modules, instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c "import anndata.core"` works, but pytest with `importlib` mode would import that module as a standalone module named `".env.lib.site-packages.anndata.core"`, because importlib module was designed to import test files which are not reachable from `sys.path`, but now it is clear that normal modules should be imported using the standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally, without changing `sys.path`, and if that fails it falls back to importing the module as a standalone module.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Feb 25, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
8afcc12 
…chanism

As detailed in pytest-dev#11475 (comment), currently with `--import-mode=importlib` pytest will try to import every file by using a unique module name, regardless if that module could be imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path` (via other mechanism, such as being installed into a virtualenv, PYTHONPATH, etc) would end up being imported as standalone modules, instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c "import anndata.core"` works, but pytest with `importlib` mode would import that module as a standalone module named `".env.lib.site-packages.anndata.core"`, because importlib module was designed to import test files which are not reachable from `sys.path`, but now it is clear that normal modules should be imported using the standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally, without changing `sys.path`, and if that fails it falls back to importing the module as a standalone module.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Feb 25, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
d8ad203 
…chanism

As detailed in pytest-dev#11475 (comment), currently with `--import-mode=importlib` pytest will try to import every file by using a unique module name, regardless if that module could be imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path` (via other mechanism, such as being installed into a virtualenv, PYTHONPATH, etc) would end up being imported as standalone modules, instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c "import anndata.core"` works, but pytest with `importlib` mode would import that module as a standalone module named `".env.lib.site-packages.anndata.core"`, because importlib module was designed to import test files which are not reachable from `sys.path`, but now it is clear that normal modules should be imported using the standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally, without changing `sys.path`, and if that fails it falls back to importing the module as a standalone module.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
bluetech pushed a commit to bluetech/pytest that referenced this pull request Feb 27, 2024
@nicoddemus @bluetech
Change importlib to first try to import modules using the standard me…
3fcfd0b 
…chanism

As detailed in
pytest-dev#11475 (comment),
currently with `--import-mode=importlib` pytest will try to import every
file by using a unique module name, regardless if that module could be
imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path`
(via other mechanism, such as being installed into a virtualenv,
PYTHONPATH, etc) would end up being imported as standalone modules,
instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c
"import anndata.core"` works, but pytest with `importlib` mode would
import that module as a standalone module named
`".env.lib.site-packages.anndata.core"`, because importlib module was
designed to import test files which are not reachable from `sys.path`,
but now it is clear that normal modules should be imported using the
standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally,
without changing `sys.path`, and if that fails it falls back to
importing the module as a standalone module.

This also makes `importlib` respect namespace packages.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 2, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
14de5df 
…chanism

As detailed in
pytest-dev#11475 (comment),
currently with `--import-mode=importlib` pytest will try to import every
file by using a unique module name, regardless if that module could be
imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path`
(via other mechanism, such as being installed into a virtualenv,
PYTHONPATH, etc) would end up being imported as standalone modules,
instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c
"import anndata.core"` works, but pytest with `importlib` mode would
import that module as a standalone module named
`".env.lib.site-packages.anndata.core"`, because importlib module was
designed to import test files which are not reachable from `sys.path`,
but now it is clear that normal modules should be imported using the
standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally,
without changing `sys.path`, and if that fails it falls back to
importing the module as a standalone module.

This also makes `importlib` respect namespace packages.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 2, 2024
@nicoddemus
Change importlib to first try to import modules using the standard me…
c85fce3 
…chanism

As detailed in
pytest-dev#11475 (comment),
currently with `--import-mode=importlib` pytest will try to import every
file by using a unique module name, regardless if that module could be
imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path`
(via other mechanism, such as being installed into a virtualenv,
PYTHONPATH, etc) would end up being imported as standalone modules,
instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c
"import anndata.core"` works, but pytest with `importlib` mode would
import that module as a standalone module named
`".env.lib.site-packages.anndata.core"`, because importlib module was
designed to import test files which are not reachable from `sys.path`,
but now it is clear that normal modules should be imported using the
standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally,
without changing `sys.path`, and if that fails it falls back to
importing the module as a standalone module.

This also makes `importlib` respect namespace packages.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
flying-sheep pushed a commit to flying-sheep/pytest that referenced this pull request Apr 9, 2024
@nicoddemus @flying-sheep
Change importlib to first try to import modules using the standard me…
731f679 
…chanism

As detailed in
pytest-dev#11475 (comment),
currently with `--import-mode=importlib` pytest will try to import every
file by using a unique module name, regardless if that module could be
imported using the normal import mechanism without touching `sys.path`.

This has the consequence that non-test modules available in `sys.path`
(via other mechanism, such as being installed into a virtualenv,
PYTHONPATH, etc) would end up being imported as standalone modules,
instead of imported with their expected module names.

To illustrate:

```
.env/
  lib/
    site-packages/
      anndata/
        core.py
```

Given `anndata` is installed into the virtual environment, `python -c
"import anndata.core"` works, but pytest with `importlib` mode would
import that module as a standalone module named
`".env.lib.site-packages.anndata.core"`, because importlib module was
designed to import test files which are not reachable from `sys.path`,
but now it is clear that normal modules should be imported using the
standard mechanisms if possible.

Now `imporlib` mode will first try to import the module normally,
without changing `sys.path`, and if that fails it falls back to
importing the module as a standalone module.

This also makes `importlib` respect namespace packages.

This supersedes pytest-dev#11931.

Fix pytest-dev#11475
Close pytest-dev#11931
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluetech bluetech bluetech left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ModuleNotFoundError when using --doctest-modules, a src/ layout, and --import-mode=importlib (and no editable mode install)

3 participants

@flying-sheep @nicoddemus @bluetech