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

Ability to disable support for PEP 420 due to performance degradation #797

Closed
jdimmerman opened this issue Aug 7, 2024 · 4 comments · Fixed by #798
Closed

Ability to disable support for PEP 420 due to performance degradation #797

jdimmerman opened this issue Aug 7, 2024 · 4 comments · Fixed by #798
Labels
enhancement New feature or request

Comments

@jdimmerman
Copy link

Is your feature request related to a problem? Please describe.

The upgrade from 0.16.1 to 0.16.2 caused a significant performance degradation for our use case. The deptry execution went from ~1 second to ~30 seconds on my machine and my entire team is seeing similar impact. We believe that this is due to the use of rglob introduced in #753 but that is an "informed" guess. We use deptry as a precommit hook, so this impacts our workflow somewhat heavily.

Describe the solution you would like

Our use case does not benefit from this change given how our modules are structured, so we would like to be able to opt-in disable the recursive search, understanding the implications. There may of course also be a way to improve performance, which would be the ideal solution.

Additional context

None, just a thanks!

@jdimmerman jdimmerman added the enhancement New feature or request label Aug 7, 2024
@DArtagan
Copy link

DArtagan commented Aug 7, 2024

From an initial glance at the code:

or (path.is_dir() and list(path.rglob("*.py")))

I read it as "or if there's a python file anywhere in the recursive directory structure". In cases where there are a bunch of deeply nested directories (I know we have a node_modules folder in with the Python), gathering that full list of Python files could take a beat. And it's likely that if there will be Python files, for local modules or whathave you, they're likely to be found at the top.

Speculation: a breadth-first search that bails on the first python file might make for a sizeable performance uplift.

@mkniewallner
Copy link
Collaborator

Hi, thanks to both of you for the report and investigation, and sorry about the regression.

Since the change was meant to solve #740, but only partially solved the issue in the end, it might be best to revert the change for now, and see if we can come up with a better solution if we do want to properly support PEP 420.

For the long-term, we should also look into having performance tests at some point to ensure that we don't introduce huge performance regressions when doing code changes.

@mkniewallner
Copy link
Collaborator

The change that should have caused the regression has been reverted in 0.19.0. I would appreciate it if you can test again on this version to confirm that it fully resolves the regression.

@jdimmerman
Copy link
Author

@mkniewallner Thanks for the quick response and action. I can confirm that the latest version fixed our performance issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants