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.

Sign up for GitHub

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

Jump to bottom

Use shortest module name for importlib imports #11931

Closed
wants to merge 7 commits into from
Closed

Use shortest module name for importlib imports #11931

wants to merge 7 commits into from

Conversation

flying-sheep
Copy link
Contributor

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

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

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.

src/_pytest/pathlib.py Outdated Show resolved Hide resolved
testing/test_pathlib.py Outdated Show resolved Hide resolved
@nicoddemus
Copy link
Member

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

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

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
3 participants
@flying-sheep @nicoddemus @bluetech