Try to make virtualenv metadata findable when invoked globally

[kwentine]

PR Checklist

• A description of the changes is added to the description of this PR.
• If there is a related issue, make sure it is linked to this PR.
• If you've fixed a bug or added code that should be tested, add tests!
• Documentation in docs is updated

Description of changes

This PR is an attempt to fix #92 by:

1. Detecting if deptry is likely run outside of the project's virtualenv
2. If so, try to find the project's site-packages by checking common locations, and make it findable by importlib.metadata

To test the changes on the deptry project itself:

# Simulate a global deptry install
VENV=~/.virtualenvs/globalinstall
python -m venv $VENV
$VENV/bin/python -m pip install deptry
cd /path/to/deptry
# Should work
$VENV/bin/deptry .

[fpgmaas]

Nice work, this seems to be a very good solution approach. I ran some tests locally and it seems to find the metadata in the virtual environment in all cases!

I will need some time to look into your proposed changes and see if I can provide some feedback, I will start now but will probably have to continue later this weekend.

The first thing I notice is the following:

➜  example-project git:(main) ✗ . /Users/florian/git/example-project/.venv/bin/activate
(example-project-py3.10) ➜  example-project git:(main) ✗ deptry .
example-project
Assuming project virtualenv /Users/florian/git/example-project/.venv/lib/python3.10/site-packages
Consider explicitly activating before running deptry
Scanning 2 files...
Success! No dependency issues found.
(example-project-py3.10) ➜  example-project git:(main) ✗ 

Here, I installed deptry globally. This error does not seem appropriate; I already activated my environment. Maybe we can try and detect the difference between a non-activated virtual environment, and a globally installed deptry version?

[fpgmaas]

Need to sign off for now since my train just arrived at the station, but just wanted to let you know I am impressed with your work! I hope I did not discourage you with my feedback. I look forward to merging this after one or two iterations :)

[kwentine]

Thanks for your very kind and spot-on feedback :) I made an iteration during the limited time I had this morning, factoring out the logic in a dedicated virtualenv_finder module that exposes a simplified entrypoint to cli. Haven't addressed the argument cardinality and suggestion to refactor into classes yet.

Regarding your main comment concerning the warning message you were totally right: the issue is not whether the project virtualenv is active or not, but rather if we are being run by its interpreter.

Have a nice day :)

[kwentine]

I'm not very knowledgeable with pre-commit, but as I understand it (and I think you mention it in the documentation), deptry installed as a hook may be the most common occurence of running outside of a project's virtualenv. For this use case the changes of this PR would be beneficial, since package metadata is likely to be found in standard layouts. But I remember that pre-commit allows to specify arguments to pass to hooks, so maybe supporting a --project-virtualenv argument in the CLI could be useful to users, and when provided, allow us to avoid the guesswork.

-   repo: https://github.com/fpgmaas/deptry.git
    rev: <tag>
    hooks:
    - id: deptry
      args:
        - "--skip-missing"
        - "--project-virtualenv path/to/virtualenv"

But that might be something for another PR :)

[mkniewallner]

While I think it's a nice option to be able to run deptry from a global installation, I must say I'm not a fan of the part that dynamically tries to guess where the virtual environment is, as the location is not standard, so the virtual environment can be created in various locations depending on the tool used.

For instance:

• Poetry will by default create them in ~/.cache/pypoetry/virtualenvs on Linux
• pyenv-virtualenv will by default create them in ~/.pyenv/versions on Linux

A more flexible choice might be to add a custom option to define the path to the virtual environment, like virtual-environment-path, since this would made the logic more simple and less magical, while allowing the flexibility that some users might need.

[mkniewallner]

A more flexible choice might be to add a custom option to define the path to the virtual environment, like virtual-environment-path, since this would made the logic more simple and less magical, while allowing the flexibility that some users might need.

mypy allows to do that through --python-executable option, where the user can pass the path to a Python executable (for instance mypy --python-executable ~/Library/Caches/pypoetry/virtualenvs/foobar-7GcI82l9-py3.10/bin/python) .

[kwentine]

Thanks for your remarks @mkniewallner. I do agree that the guessing is brittle. One could argue that failing to find the virtualenv (because the candidate location list is incomplete) leaves the user no worse off than before. But maybe more worrisome is the risk to wrongly guess a virtualenv...

So @fpgmaas do you think should I directly work in this PR to add --python-site-packages CLI argument ? And maybe just emit a warning message if we suspect that deptry is being run globally without this argument provided ?

[fpgmaas]

@kwentine Yes, I think @mkniewallner point's are valid and his suggestion is a good one. Thanks mathieu for sharing your feedback! So then it would make sense to add the --python-site-packages CLI argument in this PR. I also agree with your suggestion to emit a message, I think that would be a clean solution.

[kwentine]

Hi 👋🏻 Had shot at supporting --python-site-packages this morning. However I had some doubts: if users are accustomed to --python-executable from other tools as @mkniewallner remarked, maybe we should stick to that ? (And it is less cumbersome to specify since the python version is not in the path).

[kwentine]

I also wanted to share an alternative, more explicit way of searching for metadata in a custom path, that occurred to me while perusing importlib.metadata source.

from importlib.metadata import Distribution
def distribution(name: str, path: str | None):
    kwargs = {'name': name}
    if path is not None:
        kwargs['path'] = [path] # or `[path, *sys.path]` ?
    try:
        return next(Distribution.discover(**kwargs))
    except StopIteration:
        raise PackageNotFoundError(name)

Using this function instead of metadata.distribution should do the trick, with the possible upsides of narrowing the search only to site packages (if that is wished) and being easier to test. At the cost of replacing all calls to importlib.metadata.distribution. Just an idea to toy with maybe :)

[fpgmaas]

Hi 👋🏻 Had shot at supporting --python-site-packages this morning. However I had some doubts: if users are accustomed to --python-executable from other tools as @mkniewallner remarked, maybe we should stick to that ? (And it is less cumbersome to specify since the python version is not in the path).

I think since we explicitly looking for site-packages, what you have currently implemented looks good to me. Otherwise, we still need to do some searching for the corresponding site-packages directory.

[pmav99]

typo in project

[fpgmaas]

Closing due to inactivity.