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

Add deptry to check for issues with the project's dependencies. #679

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fpgmaas
Copy link

@fpgmaas fpgmaas commented Mar 25, 2024

Description

This proposes to add deptry to the project, which would automatically detect issues with the project's dependencies, such as unused, transitive, or missing dependencies, similar to what was handled in this PR.

Disclaimer; I am the author of deptry :)

The output of a run of deptry on the project is shown below:

Initial pyproject.toml config:

[tool.deptry]
pep621_dev_dependency_groups = ["development", "tests", "rtd", "deps"]
extend_exclude = ["doc", "conftest.py", "versioneer.py", "scripts"]
deptry .
   
Assuming the corresponding module name of package 'pydot2' is 'pydot2'. Install the package or configure a package_module_name_map entry to override this behaviour.
Scanning 189 files...

pyproject.toml: DEP002 'jaxlib' defined as a dependency but not used in the codebase
pytensor/breakpoint.py:121:24: DEP001 'pudb' imported but missing from the dependency definitions
pytensor/breakpoint.py:126:28: DEP001 'ipdb' imported but missing from the dependency definitions
pytensor/link/c/cmodule.py:2669:16: DEP001 'mkl' imported but missing from the dependency definitions
pytensor/link/c/cutils.py:103:9: DEP001 'cutils_ext' imported but missing from the dependency definitions
pytensor/link/c/cutils.py:113:17: DEP001 'cutils_ext' imported but missing from the dependency definitions
pytensor/link/c/cutils.py:116:17: DEP001 'cutils_ext' imported but missing from the dependency definitions
pytensor/link/c/lazylinker_c.py:26:12: DEP001 'lazylinker_ext' imported but missing from the dependency definitions
pytensor/link/c/lazylinker_c.py:159:13: DEP001 'lazylinker_ext' imported but missing from the dependency definitions
pytensor/link/c/lazylinker_c.py:167:1: DEP001 'lazylinker_ext' imported but missing from the dependency definitions
pytensor/link/c/lazylinker_c.py:168:1: DEP001 'lazylinker_ext' imported but missing from the dependency definitions
pytensor/link/jax/dispatch/random.py:17:12: DEP001 'numpyro' imported but missing from the dependency definitions
pytensor/link/jax/dispatch/random.py:311:5: DEP001 'numpyro' imported but missing from the dependency definitions
pytensor/link/jax/dispatch/random.py:334:5: DEP001 'numpyro' imported but missing from the dependency definitions
pytensor/link/jax/dispatch/random.py:357:5: DEP001 'numpyro' imported but missing from the dependency definitions
pytensor/link/jax/dispatch/scalar.py:42:16: DEP001 'tensorflow_probability' imported but missing from the dependency definitions
pytensor/printing.py:33:12: DEP004 'pydot_ng' imported but declared as a dev dependency
pytensor/printing.py:42:16: DEP004 'pydot' imported but declared as a dev dependency
Found 18 dependency issues.

For more information, see the documentation: https://deptry.com/

In this PR, I added config to ignore these specific errors in the pyproject.toml, since it is difficult for me to judge if they are correct or not. At first glance, it seems that the error for jaxlib is correct; that does not seem to be used. Also, pydot, pydot2 and pydot_ng are declared as 'docs' dependencies, but they are used within the pytensor directory.

If you think it does not add enough value to the project, feel free to close it.

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94 ricardoV94 requested a review from maresb March 25, 2024 09:38
@maresb
Copy link
Contributor

maresb commented Mar 25, 2024

This looks really interesting but I'm slammed at the moment. Could you ping me in a few weeks so I can give this a proper look?

@fpgmaas
Copy link
Author

fpgmaas commented Jun 9, 2024

@maresb ping ;) updated the PR, it seems typing_extensions is now also no longer used and can be removed as a dependency. Don't feel pressured to review though, if you are short on time I can always make a separate PR that just removes typing_extensions from the dependencies.

@maresb maresb force-pushed the deptry branch 3 times, most recently from 7c71c66 to 0ae6a90 Compare June 9, 2024 15:37
@maresb
Copy link
Contributor

maresb commented Jun 9, 2024

Thanks a lot @fpgmaas, this is a really cool project. I like that it pushed me to document several quirks of our dependencies in the ignores. Moreover I opened #812 and #813 to fix some issues revealed here.

I can also explain some minor pain points:

  1. As I close the aforementioned issues, it would be nice to enforce the removal of the corresponding rules. For this feature request, I opened Add yesqa-style rule against unused ignores fpgmaas/deptry#728.

  2. I'd like to add deptry to pre-commit, but it the pre-commit hook doesn't include the binary. I was going to suggest emulating ruff's pre-commit hook, but then I realized that you set language="system", so the base assumption is that you're running deptry from a particular venv. Thus my suggestion wouldn't actually gain much.

    As a followup, my mental model of these tools like Ruff is that they should ideally run independently of the code's environment. From this perspective ruff succeeds while mypy fails. It would be very interesting to see if you could get by without using a venv but instead using PyPI's metadata service.

  3. I'd like to add deptry to our development environment, but we use conda-forge. I created a staged recipe for adding deptry to conda-forge. I'd be happy to add you as a maintainer, but you need to write a note on that PR to give consent.

  4. I wonder if you could improve heuristics for your imports. In particular, anyone who is using deptry will be very likely to use import sorting that encourages most imports to be placed at the top of the file. Therefore, I'd expect to good approximation that any imports not at the top are conditional imports of optional dependencies rather than required dependencies.

I hope this feedback is useful. As for the future of this PR, I think it may make sense to add deptry to our pytest workflow after it becomes available on conda-forge. Thanks again for taking the time to share your awesome tool!

@fpgmaas
Copy link
Author

fpgmaas commented Jun 10, 2024

Thanks a bunch for the feedback!

  1. I think this is a great suggestion, will work on implementing this. I was aware of this problem, but I never thought of this solution.
  2. Yup, that has been a problem with deptry since the beginning; (link to issue). But we never got around to solving it. I see some new suggestions have popped up in the thread in the past months, so maybe time to revisit and it give it another attempt. I am not too sure about relying on PyPi's metadata service though; this will cause issues if the users works in an environment where access to PyPi is blocked and they use a private artifact registry instead.
  3. done
  4. I like the idea, but in my opinion this would make deptry a bit opinionated which might cause issues. See for example here. This is not strictly at the top of the file, so deptry might then say that tomllib can be removed, which will then break the code for users who use Python 3.11 or newer. I can imagine other examples where this can go wrong.

@maresb maresb requested a review from ricardoV94 June 13, 2024 15:04
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.87%. Comparing base (0e29d76) to head (ad6645f).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #679   +/-   ##
=======================================
  Coverage   80.87%   80.87%           
=======================================
  Files         163      163           
  Lines       46847    46847           
  Branches    11463    11463           
=======================================
  Hits        37887    37887           
+ Misses       6750     6747    -3     
- Partials     2210     2213    +3     

see 3 files with indirect coverage changes

@ricardoV94
Copy link
Member

Needs a rebase?

@maresb
Copy link
Contributor

maresb commented Jun 13, 2024

Squished.

@maresb maresb marked this pull request as draft June 13, 2024 16:17
@maresb
Copy link
Contributor

maresb commented Jun 13, 2024

Just realized that currently deptry only exists for Linux and AMD Mac on conda-forge. That might break the workflows of developers. Will remove it from the environment.yaml

@maresb maresb marked this pull request as ready for review June 13, 2024 16:23
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.

3 participants