-
Notifications
You must be signed in to change notification settings - Fork 62
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
Reintroduce the preresolved dependency optimization #610
Comments
In our projects, we fixed pip-audit to version 2.5.2 because the later versions are definitely so much slower. Version 2.5.2$ time python3 -m pip_audit --require-hashes --requirement requirements/remote.txt
No known vulnerabilities found
real 0m0,624s
user 0m0,563s
sys 0m0,062s Version 2.5.5$ time python3 -m pip_audit --require-hashes --requirement requirements/remote.txt
No known vulnerabilities found
real 0m15,915s
user 0m11,808s
sys 0m1,037s |
@woodruffw I wonder whether this would be solved by us passing I need to do a bit of testing with |
@pauloxnet If you run that command with |
I confirm that we create hashed requirements file with python3 -m piptools compile --generate-hashes --no-header --quiet --resolver=backtracking --upgrade --output-file requirements/test.txt requirements/test.in Using The main difference in |
@woodruffw Based on that, we might have to just bring that logic back. I don't think it's that big a deal as long as it's not the default behaviour when someone uses |
@tetsuo-cpp Agreed; I'll update this issue. |
Hi everyone, I would like to confirm what @pauloxnet has reported regarding the drop in performance observed in versions after 2.5.2 of the software. Specifically, this issue arises when a requirement.txt file, built with pip compile and containing hashes, is passed to pip audit. The problem stems from the fact that starting from version 2.5.3, all the dependencies parsed by pip audit are installed in a temporary virtual environment, and there is no option to exclude this behavior. Upon examining the code of pip audit, I have come to the conclusion that it is logically correct to enforce the installation of packages in a temporary virtual environment in order to ensure that the audit includes all subdependencies. The only, or perhaps the primary, exception to this would be when the requirements are compiled with pip compile, without necessarily including the hashes. This is because certain Python libraries generate the hashes in the requirement.txt file, but these hashes may not guarantee the inclusion of all the subdependencies. Therefore, in general, I believe it is appropriate that when the arguments --require-hashes or --no-deps are passed, the installation occurs in the temporary virtual environment. This is because the presence of hashes does not guarantee that all the dependencies listed in the requirements.txt file will actually be installed. Personally, I would suggest implementing a new code flow to provide pip audit with information about whether the requirements.txt file was compiled with pip compile or not. Perhaps this could be achieved through a new argument for pip audit, such as Alternatively, another approach could be to parse the content of the requirements.txt file itself. If the file contains a reference to |
@woodruffw I thought of opening a pull request with the proposal to introduce a new option to optimize dependency resolution in case they are compiled with pip-compile ☝️ |
Thanks to @woodruffw and @trottomv for the new version. |
## [2.6.0] ### Added * Added option to skip dependency resolution via `pip` with the `--disable-pip` flag. This option can only be used with hashed requirements files or when the `--no-deps` flag has been provided ([#610](pypa/pip-audit#610))
We removed this in #540 for correctness reasons.
However, doing so seems to have caused some bad performance regressions on
brew-pip-audit
's automation: audits that were previously only bound by vulnerability API lookup time are now much slower, as they call intopip
and do dependency machinery work.We should consider reintroducing a variant of these, potentially stuffed behind a scary flag.
CC @tetsuo-cpp for thoughts.
CC @alex for visiblity.
The text was updated successfully, but these errors were encountered: